Habitica: Member Counts Inaccurate in Parties

[SabreCat]

Description

Clean-up of #7454. You can visit that ticket to see the full history of discussion on this issue.

memberCount in group records is a stored computed value that we currently attempt to keep up to date by incrementing and decrementing as members join and leave. It frequently gets out of sync, leading to people with 6-member parties showing a member count of 7, etc.; this is a nuisance at best, and in some cases involving the 30-person party limit, can actually interfere with people joining or inviting members.

This is the desired fix:

change the group remove, leave and join routes to recompute memberCount each time it needs to be updated (i.e., count all existing members, rather than add or subtract one from the current value of memberCount)

...when Parties are involved. At this time, we're leaving Guilds alone for performance reasons. See #12286 for the status of that arm of the issue.

[Alys]

I'm changing this to suggestion-discussion because @paglias has raised load-related concerns about the desired fix (which was developed from an old comment in the old issue) and I don't want a contributor starting on it if it's decided we don't go ahead with it. :)

[paglias]

In the meantime if the issue affects mostly parties where the member count is more important (due to the limit of 30 members) we could recalculate the number of members just for them excluding guilds

[SabreCat]

OK, I'm updating this ticket to relate to Parties only, and will spin off a separate ticket for Guilds that we'll leave on hold pending the ability to use transactions.

[Alys]

Following on from an initial question about this issue in Aspiring Blacksmiths:

From memory, there's already code for recomputing memberCount (git grep memberCount to find it), so it may be pretty simple to use that code when a party's membership change.

[JalanshMunshi]

Hi, can someone please assign this issue to me. I do have general programming experience and I would like to work on this as my first issue. I will go through the issues mentioned here, start working on this issue and post any doubts here. Also, will it possible to inform where do I need to look in the code for this change?

[JalanshMunshi]

As per my understanding, we have to change how the number of members is calculated. I was unable to determine which section of code recomputes it (as pointed out by @Alys ).

There is not separate end-point like url: '/groups' for parties and hence, whether a group is a party or not is checked in the groups.js file (website/server/controllers/api-v3/groups.js).

The memberCount attribute is retrieved after making the following call -

const group = await Group.getGroup({user, groupId: req.params.groupId, optionalMembership, fields: '-chat',});

getGroup() executes a query and fetches the group details. Do we have to make a change in how the query is executed?

Can someone please guide me in a correct direction?

[Alys]

@JalanshMunshi I'm sorry about the delay! I've marked this as in progress for you.

The issue is that when people join or leave a group, the group's memberCount attribute is adjusted by merely adding or subtracting 1 from the current value. This would work perfectly well if database transactions were in place, but without them it's unreliable. For example, when a player joins a group, we need to do two updates: one to the player's own account (to add the party or guild's ID to the player's party._id or guilds attribute); and the other to the group's memberCount attribute. If one of those updates fails and the other succeeds, the memberCount attribute gets out of sync with the real number of members.

To minimise that problem for parties, we want to change how memberCount is updated when someone joins or leaves. Instead of adding/subtracting 1, we want to recompute memberCount from scratch. We'd do that by querying the database to count how many members there are (not by querying it to merely retrieve the value of memberCount).

Does that clarify the issue? Please post again if not!

[SabreCat]

@JalanshMunshi Look for uses of memberCount += 1 or memberCount -= 1! example: https://github.com/HabitRPG/habitica/blob/develop/website/server/controllers/api-v3/groups.js#L606

[JalanshMunshi]

Thanks for the help, @Alys, and @SabreCat !

Can you please review my draft PR? I get the existing memberCount of the group and add/subtract 1 to it. Since the draft PR is for changing the way how memberCount is incremented/decremented, I don't think I need to change any existing test cases or add any new ones.

However, please confirm this from your end as well. Please point out if I am missing anything or making any mistake in the draft PR.

Some doubts from my end:
memberCount is also decremented in the following 2 places -

1. https://github.com/HabitRPG/habitica/blob/92dc332ab908416750c1ca5671dad62837123e31/website/server/models/challenge.js#L296
2. https://github.com/HabitRPG/habitica/blob/92dc332ab908416750c1ca5671dad62837123e31/website/client/src/components/groups/membersModal.vue#L589

The first one is related to challenges. As per my understanding, we do not have to make any changes there. However, I am not sure about the second one. Can you please provide confirmation for both my doubts?

[Alys]

Good questions!

My personal opinion about the tests is that you're probably correct that they don't need to be updated, but if Habitica's staff say otherwise, listen to them not me. :)

You're right that the code for challenges doesn't need to be modified for this issue. Challenges do suffer from the same memberCount problem as groups but the consequences are less severe, so there's no point us trying to fix Challenges until database transactions are available.

The *.vue files such as membersModal.vue contain only client-side code, so any modifications to memberCount made in them are only to give the player instant feedback for their action to add/remove a member. It doesn't matter if that code is inaccurate because the next time the player loads the page, the correct value that's stored in the database will be displayed.

[paglias]

@Alys is right, I would not update tests as there are already tests covering the changes in group.memberCound, we could add a test where we manually set the database value for memberCount to a wrong number and check that it corrects itself (for parties) after a new user joins but it would become obsolete as soon as transactions are enabled so I would say we can skip it