-
Notifications
You must be signed in to change notification settings - Fork 32
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
Conversation
ca43e77
to
0e01294
Compare
Affected libs:
|
📷 Screenshots are here! |
b2b998f
to
49ab5fd
Compare
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.
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.
apps/metadata-editor/src/app/edit/components/publish-button/publish-button.component.html
Outdated
Show resolved
Hide resolved
apps/metadata-editor/src/app/edit/components/publish-button/publish-button.component.ts
Outdated
Show resolved
Hide resolved
f53e1b8
to
19ff46b
Compare
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.
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.
Clicking cancel and back arrow to revert changes =>
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.
apps/metadata-editor/src/app/edit/components/publish-button/publish-button.component.html
Show resolved
Hide resolved
apps/metadata-editor/src/app/edit/components/publish-button/publish-button.component.ts
Outdated
Show resolved
Hide resolved
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: |
183a90b
to
0fb05f5
Compare
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 :
recordUpdated
field when the draft is created, to be able to compare the draft's date with the record's date in the backend.Screenshots
Quality Assurance Checklist
breaking change
labelbackport <release branch>
label