-
Notifications
You must be signed in to change notification settings - Fork 4k
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
(Issue #11805) Removing duplicated user.pushDevices in cron task. #11825
Conversation
Also removed redundant blank one.
@SabreCat bump. |
Sorry for the delay but I won't have time to review PRs until the 15th |
Bump. |
@xrem sorry for the delay! The code is looking good (thanks for the tests!) but I'm wondering if at this point we should make something that can also be used to fix #11868 so instead of removing duplicates at cron we could do for both bugs as suggested here #11868 (comment) and clean anything wrong with push devices in a single function that is called when the push devices are modified |
After giving this some extra look I think the best place to fix duplicate devices would be to add a check that filters out duplicated entries in the user's pre save hook here https://github.com/HabitRPG/habitica/blob/develop/website/server/models/user/hooks.js#L260-L262 like it's similarly done with push notifications. You can reuse the code from here https://github.com/HabitRPG/habitica/pull/11825/files#diff-95ea51d6c9aaabaf272f7ff354e1d8a5R483 but it's important to wrap it into an if block like this to prevent the code from running when
Let me know if you have any question |
I'm not so sure that cron is a good place to fix the kind of bad pushDevices data described in #11868. One of the damaging aspects of that bad data is that it can affect other players when they interact with the account that has the bad data, and that includes accounts for people who are no longer using Habitica. The cron code won't be run for those players, so we need a fix in some other part of the code, so fixing it in cron too would be duplication. |
@Alys yeah my suggestion was to have the fix run every time the user is saved, but for those who don't login anymore we can run a migration |
Hi @xrem , thanks again for the PR, given this bug is closely related to #11868 I'm going to take over this PR. I'll try to incorporate your commits in my PR so that they remain available in Github, but if that becomes impossible you'll still get an Habitica contribution tier for this! Thanks for the understanding |
Fixes #11805
Changes
Cron task was modified to remove duplicated
user.pushDevices
So, the
pushDevices
array now contains only objects with uniqueregId
field.UUID: a9b92003-4817-4edd-8fa6-65fa995b2b1f