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.

Sign up for GitHub

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

Jump to bottom

Created component for editing account settings #627

Merged
merged 5 commits into from
Mar 20, 2024
Merged

Created component for editing account settings #627

merged 5 commits into from
Mar 20, 2024

Conversation

BenjaminCharmes
Copy link
Member

@BenjaminCharmes BenjaminCharmes commented Mar 8, 2024 •
edited by jdbocarsly
Loading

Working:

  • Displaying current user display_name
  • Users routes created with one route for updating display_name
  • The buttons for connecting/disconnecting from Github and ORCHID exist but are not currently working either

Issue to fix:

  • When closing the form without submitting but modifying the display_name, it'll keep the modification in the form until page get refreshed
  • When submitting, the form doesn't close and the new display_name need the page to be refreshed manually to be displayed correctly

Closes #623

ml-evs
ml-evs reviewed Mar 11, 2024
View reviewed changes
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

Thanks for this @BenjaminCharmes! Looks good so far, just a couple of comments on the API endpoints:

pydatalab/pydatalab/routes/v0_1/users.py Outdated Show resolved Hide resolved
pydatalab/pydatalab/routes/v0_1/users.py Outdated Show resolved Hide resolved
ml-evs
ml-evs reviewed Mar 11, 2024
View reviewed changes
Copy link
Member

@ml-evs ml-evs left a comment •
edited by BenjaminCharmes
Loading

Choose a reason for hiding this comment

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

Nearly finished, as discussed couple of things remain:

  • Show and update contact email
    • Potentially allow empty email addresses to mean -> delete my email address (set email to None in db)
    • Update route to handle providing both display name and contact email
  • Show GitHub username (and write ORCID so its ready in future)
  • Test authentication

@ml-evs ml-evs mentioned this pull request Mar 12, 2024
Closed
@BenjaminCharmes
Copy link
Member Author

Everything should be working now, to sum up:

  • Updated LoginDetails component: now using the store to update dynamically the user's display_name

  • Account settings is now enabled with the EditAccountSettingsModal component displaying:

    • Name = editable user's display_name (required)
    • Contact email = editable user's contact_email (can be None)
    • Link to user's github page || Login via Github
    • Link to user's orcid page || Login via ORCID (disabled)

  • New users routes in pydatalad to save_user (currently supporting updating display_name and contact_email only)

  • And the saveUser fetch in server_fetch_utils.js

@ml-evs ml-evs force-pushed the bc/account-settings branch 2 times, most recently from f36b7c5 to b3925e7 Compare March 13, 2024 16:44
@ml-evs ml-evs mentioned this pull request Mar 13, 2024
Merged
@ml-evs
Copy link
Member

ml-evs commented Mar 13, 2024

For some reason all the e2e tests are failing here @BenjaminCharmes, would you be able to take a look when you get a chance before we merge?

jdbocarsly
jdbocarsly previously requested changes Mar 14, 2024
View reviewed changes
Copy link
Member

@jdbocarsly jdbocarsly left a comment

Choose a reason for hiding this comment

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

Looks great! Nice clean code. Just two little suggestions in the js.

webapp/src/components/EditAccountSettingsModal.vue Outdated Show resolved Hide resolved
webapp/src/components/EditAccountSettingsModal.vue Outdated Show resolved Hide resolved
@BenjaminCharmes BenjaminCharmes changed the title [WIP] Created component for editing account settings Created component for editing account settings Mar 18, 2024
@ml-evs ml-evs self-requested a review March 19, 2024 12:37
ml-evs
ml-evs requested changes Mar 19, 2024
View reviewed changes
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

Just a couple more requests then this is good to go!

pydatalab/pydatalab/routes/v0_1/users.py Outdated Show resolved Hide resolved
pydatalab/pydatalab/routes/v0_1/users.py Outdated Show resolved Hide resolved
webapp/src/components/EditAccountSettingsModal.vue Outdated Show resolved Hide resolved
webapp/src/components/EditAccountSettingsModal.vue Outdated Show resolved Hide resolved
ml-evs
ml-evs previously approved these changes Mar 20, 2024
View reviewed changes
BenjaminCharmes and others added 2 commits March 20, 2024 14:07
@BenjaminCharmes @ml-evs
Created component for editing account settings
2e5195b 
Removed the 'disabled' class from the account settings link in LoginDetails

Added logic for opening the editAccountSettings modal

Added a saveUser function to update display_name

Created users.py in pydatalab route to update users

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Updated EditAccountSettingsModal with backend

Add update:modalValue to close on submit

Update pydatalab/pydatalab/routes/v0_1/users.py

Co-authored-by: Matthew Evans <[email protected]>

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Added store and and API endpoint

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Last change: Replace orcid, add contact_email, show github username

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Remove .them in the submitForm

Modification to e2e/sampleTablePage.cy test failing due to duplicate name entry

Modification to e2e/sampleTablePage.cy test failing due to duplicate submit button

Modification on e2e test failing due to duplicate submit button

Update cypress commands createSample to selects right input

Same for e2e/editPage.cy

Same for e2e/batchSampleFeature.cy

Update findByLabelText for name inside a sample

Wrong modification using id rather than findBy
@BenjaminCharmes @ml-evs
Update error details in save_user route
7d8c9f4 
Co-authored-by: Matthew Evans <[email protected]>
BenjaminCharmes and others added 3 commits March 20, 2024 14:07
@BenjaminCharmes @ml-evs
Code review
3203011 
Replace "Login via ORCID" with "Connect your ORCiD"

Co-authored-by: Matthew Evans <[email protected]>

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Replace "Login via GitHub" with "Connect your GitHub account"

Co-authored-by: Matthew Evans <[email protected]>

Replace "account-name" with "account-email"

Co-authored-by: Matthew Evans <[email protected]>
@BenjaminCharmes @ml-evs
Update method for processing data to update user
eff7c7d
@ml-evs
Final tweaks from code review
095f0e3 
[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Fix my own linting error

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Update webapp/src/server_fetch_utils.js
ml-evs
ml-evs approved these changes Mar 20, 2024
View reviewed changes
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

Thank you @BenjaminCharmes!

@ml-evs ml-evs added enhancement New feature or request webapp For issues/PRs pertaining to the web interface server labels Mar 20, 2024
@ml-evs ml-evs merged commit 0d1e12c into the-grey-group:main Mar 20, 2024
6 of 7 checks passed
@ml-evs ml-evs mentioned this pull request Mar 21, 2024
Merged
BenjaminCharmes added a commit to BenjaminCharmes/datalab that referenced this pull request Mar 27, 2024
@BenjaminCharmes @ml-evs
Created component for editing account settings (the-grey-group#627)
1611154 
* Created component for editing account settings

Removed the 'disabled' class from the account settings link in LoginDetails

Added logic for opening the editAccountSettings modal

Added a saveUser function to update display_name

Created users.py in pydatalab route to update users

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Updated EditAccountSettingsModal with backend

Add update:modalValue to close on submit

Update pydatalab/pydatalab/routes/v0_1/users.py

Co-authored-by: Matthew Evans <[email protected]>

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Added store and and API endpoint

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Last change: Replace orcid, add contact_email, show github username

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Remove .them in the submitForm

Modification to e2e/sampleTablePage.cy test failing due to duplicate name entry

Modification to e2e/sampleTablePage.cy test failing due to duplicate submit button

Modification on e2e test failing due to duplicate submit button

Update cypress commands createSample to selects right input

Same for e2e/editPage.cy

Same for e2e/batchSampleFeature.cy

Update findByLabelText for name inside a sample

Wrong modification using id rather than findBy

* Update error details in save_user route

Co-authored-by: Matthew Evans <[email protected]>

* Code review

Replace "Login via ORCID" with "Connect your ORCiD"

Co-authored-by: Matthew Evans <[email protected]>

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Replace "Login via GitHub" with "Connect your GitHub account"

Co-authored-by: Matthew Evans <[email protected]>

Replace "account-name" with "account-email"

Co-authored-by: Matthew Evans <[email protected]>

* Update method for processing data to update user

* Final tweaks from code review

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Fix my own linting error

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

Update webapp/src/server_fetch_utils.js

---------

Co-authored-by: Matthew Evans <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ml-evs ml-evs ml-evs approved these changes

@jdbocarsly jdbocarsly jdbocarsly left review comments

Assignees
No one assigned
Labels
enhancement New feature or request server webapp For issues/PRs pertaining to the web interface
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Account settings UI
3 participants
@BenjaminCharmes @ml-evs @jdbocarsly