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

[Editor]: Warn user if draft has been updated #1051

Merged
merged 16 commits into from
Dec 30, 2024

Conversation

cmoinier
Copy link
Collaborator

@cmoinier cmoinier commented Dec 2, 2024

Description

This PR introduces a warning when the user starts editing one of their drafts, if the record has been updated by another user since they created the draft.
Key features :

  • Added detection for record change in backend when a draft is opened
  • Added the update of recordUpdated field when the draft is created, to be able to compare the draft's date with the record's date in the backend.
  • Added a banner at the top of the edit page when the record has been updated by someone else
  • Added an overlay warning message when the user tries to publish their version on top of someone else's, with cancel/proceed choices
  • E2E tests : to mock 2 users editing the same record in e2e tests, it's been decided to clear and re-populate the localStorage between 2 editions by the same user

Screenshots

image

image

Quality Assurance Checklist

  • Commit history is devoid of any merge commits and readable to facilitate reviews
  • If new logic ⚙️ is introduced: unit tests were added
  • If new user stories 🤏 are introduced: E2E tests were added
  • If new UI components 🕹️ are introduced: corresponding stories in Storybook were created
  • If breaking changes 🪚 are introduced: add the breaking change label
  • If bugs 🐞 are fixed: add the backport <release branch> label
  • The documentation website 📚 has received the love it deserves

@cmoinier cmoinier force-pushed the ME-warn-user-newer-version branch from ca43e77 to 0e01294 Compare December 2, 2024 12:10
Copy link
Contributor

github-actions bot commented Dec 2, 2024

Affected libs: api-repository, feature-catalog, feature-record, feature-router, feature-editor, feature-search, feature-map, feature-auth, common-domain, api-metadata-converter, ui-search, feature-dataviz, common-fixtures, ui-elements, feature-notifications, ui-catalog, util-shared, ui-widgets, ui-inputs, ui-layout, ui-map, ui-dataviz,
Affected apps: metadata-editor, datahub, demo, webcomponents, map-viewer, search, datafeeder, metadata-converter, data-platform,

  • 🚀 Build and deploy storybook and demo on GitHub Pages
  • 📦 Build and push affected docker images

Copy link
Contributor

github-actions bot commented Dec 2, 2024

📷 Screenshots are here!

@cmoinier cmoinier force-pushed the ME-warn-user-newer-version branch 2 times, most recently from b2b998f to 49ab5fd Compare December 2, 2024 12:33
@coveralls
Copy link

coveralls commented Dec 2, 2024

Coverage Status

coverage: 84.201% (-0.2%) from 84.438%
when pulling b12f2c0 on ME-warn-user-newer-version
into 8c98ae0 on main.

@CLAassistant
Copy link

CLAassistant commented Dec 8, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@tkohr tkohr left a comment

Choose a reason for hiding this comment

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

Thanks, @cmoinier ! Congrats for the clean PR on this essential feature ! I left a couple of comments with ideas in-line.

Almost everything works really well. I just discovered two egde/minor issues:

  • When editing the same record (that has not been saved as a draft yet) at the same and submitting it on both sides, the one submitting last can still overwrite the changes of the one submitting first without being warned.
    => This might actually be out of scope for this ticket/PR, not sure.
  • The popup does not close when accepting to overwrite changes.

Also, we should remember to clarify why we get a 400 error for non-admin users when submitting changes.

@cmoinier cmoinier force-pushed the ME-warn-user-newer-version branch from f53e1b8 to 19ff46b Compare December 20, 2024 09:44
Copy link
Collaborator

@tkohr tkohr 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 the adaptions @cmoinier !

The issues mentioned above are fixed. There is a small glitch when aborting changes after the warning and reverting them, they are not applied in the open form. This may be out of scope for this PR.

warning

Clicking cancel and back arrow to revert changes =>

change

There is still an issue with the date formatting in the tests, potentially because the CI runs in a different timezone. I think you could rely on the angular pipe for formatting here and get rid of the test. I let you fix this and approve.

@tkohr
Copy link
Collaborator

tkohr commented Dec 20, 2024

Just had a look at the e2e error log and I think the following errors are actually related to this PR, the issue on reverting changes I mentioned earlier:

@cmoinier cmoinier force-pushed the ME-warn-user-newer-version branch from 183a90b to 0fb05f5 Compare December 24, 2024 08:07
@LHBruneton-C2C LHBruneton-C2C merged commit 16f1367 into main Dec 30, 2024
14 checks passed
@LHBruneton-C2C LHBruneton-C2C deleted the ME-warn-user-newer-version branch December 30, 2024 07: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.

5 participants