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

update current user info during sync #1135

Merged
merged 1 commit into from
Sep 1, 2020

Conversation

sssoleileraaa
Copy link
Contributor

Description

Towards #76

Next step (after this PR): update gui auth widget to show updated name

Test Plan

  1. log in
  2. change first and last name of the account you're logged into from the admin UI
  3. wait until next sync then check local client db
    • verify local db shows updated first and last name
  4. change first and last name to empty string
  5. wait until next sync then check local client db
    • verify local db shows updated first and last name
  6. change first and last name from journalist UI (not admin UI)
  7. wait until next sync then check local client db
    • verify local db shows updated first and last name
  8. change username from admin UI (can't do this from journalist UI)
    • verify local db shows updated username

@sssoleileraaa sssoleileraaa force-pushed the update-current-user-info-during-session branch 2 times, most recently from 1f7e659 to 583dfd2 Compare August 6, 2020 06:34
@sssoleileraaa sssoleileraaa marked this pull request as draft August 6, 2020 17:17
@sssoleileraaa
Copy link
Contributor Author

It appears that adding the api.get_current_user call to the metadata sync job breaks functional tests in a somewhat mysterious way that has to with cassettes perhaps. If we need to generate new cassettes when we modify some parts of client code, we should document a) how to generate new cassettes, e.g. server setup - do we want NUM_SOURCES to be 5 or some other value, and b) in what cases do we need to generate new cassettes, e.g. adding a new api job or adding a new api call to the client.

(To be continued)

@sssoleileraaa sssoleileraaa force-pushed the update-current-user-info-during-session branch 2 times, most recently from cbca77c to 6e6865f Compare August 13, 2020 22:58
@sssoleileraaa
Copy link
Contributor Author

After #1138 is merged, I can do a bit of conflict resolution, then tests should pass, and i'll move this from draft to ready-for-review.

@sssoleileraaa sssoleileraaa force-pushed the update-current-user-info-during-session branch from 6e6865f to 5c6a868 Compare August 25, 2020 17:30
@sssoleileraaa
Copy link
Contributor Author

Now that #1138 has been merged, cassette generation in the client was a breeze~~ and this can be reviewed now.

@sssoleileraaa sssoleileraaa marked this pull request as ready for review August 25, 2020 17:31
Copy link
Contributor

@rmol rmol left a comment

Choose a reason for hiding this comment

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

All good:

  1. log in
  2. change first and last name of the account you're logged into from the admin UI
  3. wait until next sync then check local client db
    • verify local db shows updated first and last name
  4. change first and last name to empty string
  5. wait until next sync then check local client db
    • verify local db shows updated first and last name
  6. change first and last name from journalist UI (not admin UI)
  7. wait until next sync then check local client db
    • verify local db shows updated first and last name
  8. change username from admin UI (can't do this from journalist UI)
    • verify local db shows updated username

Looking at this, I'm starting to think storage.find_or_create_user and storage.update_and_get_user could be merged, but if so, it should be done in another change.

@rmol rmol merged commit cf872e8 into main Sep 1, 2020
@rmol rmol deleted the update-current-user-info-during-session branch September 1, 2020 20:51
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.

2 participants