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

Improvements to time syncing #2493

Conversation

corrideat
Copy link
Member

@corrideat corrideat commented Jan 3, 2025

Closes #2485.

@corrideat corrideat self-assigned this Jan 3, 2025
Copy link
Member

@taoeffect taoeffect left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left one comment. Also consider, is there any useful console.info (or higher level) debugging that should be added anywhere to help diagnose the issue?

Copy link

cypress bot commented Jan 5, 2025

group-income    Run #3654

Run Properties:  status check passed Passed #3654  •  git commit fab129db05 ℹ️: Merge 15546731b752c0ff62823127337f544e482f2e89 into 160bb6231efbcee1834b96106dc0...
Project group-income
Branch Review 2485-group-inactivity-still-shows-the-wrong-inactive-members-lastloggedin-bug
Run status status check passed Passed #3654
Run duration 10m 40s
Commit git commit fab129db05 ℹ️: Merge 15546731b752c0ff62823127337f544e482f2e89 into 160bb6231efbcee1834b96106dc0...
Committer Ricardo Iván Vieitez Parra
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 10
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 112
View all changes introduced in this branch ↗︎

@corrideat corrideat marked this pull request as ready for review January 7, 2025 20:16
@corrideat
Copy link
Member Author

corrideat commented Jan 7, 2025

is there any useful console.info (or higher level) debugging that should be added anywhere to help diagnose the issue?

I don't think so, but I also think this issue is resolved. If you want to test these changes locally, you're gonna need two devices (or a VM). You should carry out these steps on master to reproduce the issue and on this branch.

I recommend using Firefox on the device that will be suspended (device B) and running the server on device A. The reason to use Firefox is that you can follow these instructions to test locally without HTTPS.

  1. Change LAST_LOGGED_IN_THROTTLE_WINDOW in constants.js to a small value, like 2 minutes. Also, change lastLoggedIn to type: PERIODIC_NOTIFICATION_TYPE.MIN1 in mainNotificationMixin.js. This is not strictly necessary but will make testing a lot faster.
  2. Register on device A and create a group. Add income details (not necessary, but you can see the activity tab in the UI).
  3. Join the group on device B as a new user. Also add income details.
  4. Close the app in device A.
  5. Put device B to sleep for at least the time you set in LAST_LOGGED_IN_THROTTLE_WINDOW. You may need to do this a few times.
  6. On master, you should eventually miss the entry for the user created in device A. On this branch, you shouldn't miss any entries.

Copy link
Member

@taoeffect taoeffect left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Congrats @corrideat !

@taoeffect taoeffect merged commit 8d7d9a1 into master Jan 7, 2025
4 checks passed
@taoeffect taoeffect deleted the 2485-group-inactivity-still-shows-the-wrong-inactive-members-lastloggedin-bug branch January 7, 2025 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Group inactivity still shows the wrong inactive members (lastLoggedIn bug)
2 participants