-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
Fix user last active updating #1105
Conversation
Test summaryRun details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
Thanks for adding this, I've been a bit busy recently but will do my best to review and respond properly about the backup functions this week. |
@@ -1,4 +1,21 @@ | |||
import { DBDoc, IDBDocChange } from '../models' | |||
import { IDBDocChange } from '../models' | |||
import { IUserDB } from 'src/models/user.models' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to a limitation with how the functions folder is organised, imports from the local 'src' folder won't actually be recognised on build (the way typescript resolves paths is fiddly at best, there's quite a bit of tooling out there which is better for working in these monorepo structures such as lerna or yarn workspaces (particularly with newer yarn 2.x), but haven't got round to updating.
So for now can you just change it to a relative import (../models
)
prevUser: IUserDB, | ||
updatedUser: IUserDB, | ||
): boolean => { | ||
return Object.keys(prevUser).some( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really like the implementation, efficient to just look for first key barring exclusions where data has changed. I also tested locally and working as intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, just one small import to fix which I've added a comment for.
Also, if you're updating this PR could you also include the code to run the user backup function? (I'm not sure why it was ever removed, I'm guessing by mistake during some code refactoring). It can be called from
I quickly revisited the current implementation for testing these backend functions. It still could do with a lot of improvement although I'm reluctant to spend too much time on this as I'm expecting/hoping that over the next year we will be able to replace firebase entirely in the project. I've sent an invite so you can receive edit access to the testing firebase site, and there is some general notes in More specifically it involves installing and logging in to the firebase cli, running the start script from the functions folder, and then making manual updates to the emulated database to see changes (e.g. I created a user document and tried updating different fields to check if the revision would be made) |
Visit the preview URL for this PR (updated for commit af7369e): https://onearmy-next--pr1105-984-fix-last-active-9y5wrlft.web.app (expires Thu, 08 Apr 2021 20:09:33 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for checking and addressing all the feedback points. Merging
PR Type
PR Checklist
master
branch mergedDescription
A login now updates the user's last active status to the current time, as discussed in #984. The update is done in the
userSignedIn
method in the store. I tested with a custom pin and the last active info is updating correctly now.Also, in order to not backup a user every time he logs in, I added a shallow comparator on a previous and updated user, to check if
lastActive
is the only field updated. This hasn't been tested though as I'm not sure how to test the backup function and see the backup data. If the test is desirable I'd be happy to test it, just will need some directions.Git Issues
Closes #984
Screenshots/Videos