Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Challenge member count can be wrong #8337

Open
Alys opened this issue Dec 30, 2016 · 9 comments · May be fixed by #15253
Open

Challenge member count can be wrong #8337

Alys opened this issue Dec 30, 2016 · 9 comments · May be fixed by #15253

Comments

@Alys
Copy link
Contributor

Alys commented Dec 30, 2016

The member count listed on a challenge's page (which is taken from the challenge's document in the database) can be incorrect.

For example, for challenge 4d596684-ad74-416e-9abb-8195638247ea created on 2016-11-01, the memberCount in the document is 58, but a search for all players participating in the challenge shows only 53

This has also been noticed in other challenges (e.g., ccba7258-bac9-41e2-b9b5-e23ff0a50bf9, b2f1dff2-d5e0-4589-8100-5fcd0049d035, 4eb1f160-29bf-4036-bc0f-9d77bcb791e3).

I'm marking this as for suggestion-discussion so that we can discuss whether we should change the code to make updating the memberCount more reliable or do away with the memberCount attribute and take the count from the members retrieved.


With PR #12335 the Habitica codebase has been updated to support MongoDB 4.2 and transactions making it possible to fix this issue.

Transactions allow operations on multiple documents to be executed ensuring that either all of them are executed correctly or none, making it possible to fix this issue.

An example on using transactions can be found at https://mongoosejs.com/docs/transactions.html while more info at https://docs.mongodb.com/manual/core/transactions/. If you want to work on this issue and have any question please leave a comment!


EDIT 2020-11-20: I've removed some old comments that are no longer relevant, and added strike-through to some others that are also not as relevant for the new desired fix.
The desired fix is described in this comment: #8337 (comment)

@Alys
Copy link
Contributor Author

Alys commented Feb 21, 2018 via email

@SabreCat
Copy link
Member

SabreCat commented Mar 17, 2020

After a bit of staff discussion, I think the preferred solution here would be to obviate the need for a stored computed value by having the API return a count of Challenge participants at time of requesting Challenge data. We'd need to give careful consideration to performance especially on pages with lists of Challenges, but that's the way that will definitively solve this.

@Alys
Copy link
Contributor Author

Alys commented Mar 17, 2020

I know I was the person to log this issue, and I know there's already been staff discussion about it so feel free to completely ignore this disagreement if you wish 😄 but I'm wondering...
If you're concerned about performance (which certainly seems like a good concern), is it actually worth fixing this now? I don't see reports of the bug often so I don't think it's too disruptive. Once we're able to use database transactions, the current member count process could then be modified to use a transaction and so it would become reliable.

But it's just an idea. :) If any contributor wants to work on this, feel free to take it up since it's marked as help wanted!

@SabreCat
Copy link
Member

SabreCat commented Mar 19, 2020

It's a good point that it's not a high priority. If we were to tackle one of these stored-computed issues pre transactions, Guild/Party member counts would probably be more important as people run into that a lot more often!

EDIT: Based on that, I'm going to knock the priority tag down to minor, if that makes sense!

@veeeeeee
Copy link

veeeeeee commented Jun 23, 2020

May be related to #12286 and potentially solved in a similar fashion. This ticket will remain on hold until we can upgrade to a version that allows transactions, which will allow us to make a complete fix.

@paglias
Copy link
Contributor

paglias commented Jul 14, 2020

With PR #12335 the Habitica codebase has been updated to support MongoDB 4.2 and transactions making it possible to fix this issue.

Transactions allow operations on multiple documents to be executed ensuring that either all of them are executed correctly or none, making it possible to fix this issue.

An example on using transactions can be found at https://mongoosejs.com/docs/transactions.html while more info at https://docs.mongodb.com/manual/core/transactions/. If you want to work on this issue and have any question please leave a comment!

@SYN-tactic
Copy link

Hello 👋 - I'd love to help out with this issue if it's still needed. Am I correct in surmising that this line has the two database update commands that should be included in the same transaction?

Line 350 in website/server/controllers/api-v3/challenges.js

const results = await Promise.all([challenge.syncTasksToUser(user), challenge.save()]);

@CuriousMagpie
Copy link
Member

@SYN-tactic If you're willing to tackle this, you have our full permission to do so! Thank you!

@SYN-tactic
Copy link

Awesome, thanks @CuriousMagpie - I'm on it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants