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

Story Locking: Autosave on takeover #12373

Merged
merged 8 commits into from
Oct 4, 2022
Merged

Conversation

spacedmonkey
Copy link
Contributor

@spacedmonkey spacedmonkey commented Sep 26, 2022

Context

Summary

Trigger an autosave when user has their story taken over.

Relevant Technical Choices

Only trigger if they are changes in the story. Otherwise, it will trigger a REST API call that returns a 400 error.

To-do

User-facing changes

Testing Instructions

  • This is a non-user-facing change and requires no QA

This PR can be tested by following these steps:
0. Enable autosaving and take over post locking experiment.

  1. Create a story.
  2. Add some content.
  3. Save as draft.
  4. Open another browser.
  5. Login, as a differnet user.
  6. Open story ( from previous ) in editor.
  7. Click Take over story.
  8. In first browser, make changes to story. DO NOT SAVE.
  9. Wait 1 minute 30 seconds.
  10. Go back to second browser.
  11. Refresh browser.
  12. See message that there is a newer autosave.

Reviews

Does this PR have a security-related impact?

Does this PR change what data or activity we track or use?

Does this PR have a legal-related impact?

Checklist

  • This PR addresses an existing issue and I have linked this PR to it in ZenHub
  • I have tested this code to the best of my abilities
  • I have verified accessibility to the best of my abilities (docs)
  • I have verified i18n and l10n (translation, right-to-left layout) to the best of my abilities
  • This code is covered by automated tests (unit, integration, and/or e2e) to verify it works as intended (docs)
  • I have added documentation where necessary
  • I have added a matching Type: XYZ label to the PR

Fixes #12325

@spacedmonkey spacedmonkey added P1 High priority, must do soon Group: WordPress Changes related to WordPress or Gutenberg integration Pod: WP Group: Story Locking aka post locking labels Sep 26, 2022
@spacedmonkey spacedmonkey self-assigned this Sep 26, 2022
@spacedmonkey spacedmonkey marked this pull request as ready for review September 26, 2022 14:20
@googleforcreators-bot
Copy link
Collaborator

googleforcreators-bot commented Sep 26, 2022

Plugin builds for a480b6c are ready 🛎️!

@github-actions
Copy link
Contributor

github-actions bot commented Sep 26, 2022

Size Change: +61 B (0%)

Total Size: 2.71 MB

ℹ️ View Unchanged
Filename Size Change
assets/css/carousel-view-rtl.css 702 B 0 B
assets/css/carousel-view.css 701 B 0 B
assets/css/web-stories-block-rtl.css 4.52 kB 0 B
assets/css/web-stories-block.css 4.56 kB 0 B
assets/css/web-stories-embed-rtl.css 318 B 0 B
assets/css/web-stories-embed.css 317 B 0 B
assets/css/web-stories-list-styles-rtl.css 2.36 kB 0 B
assets/css/web-stories-list-styles.css 2.39 kB 0 B
assets/css/web-stories-theme-style-twentyeleven-rtl.css 102 B 0 B
assets/css/web-stories-theme-style-twentyeleven.css 102 B 0 B
assets/css/web-stories-theme-style-twentyfifteen-rtl.css 251 B 0 B
assets/css/web-stories-theme-style-twentyfifteen.css 251 B 0 B
assets/css/web-stories-theme-style-twentyfourteen-rtl.css 287 B 0 B
assets/css/web-stories-theme-style-twentyfourteen.css 287 B 0 B
assets/css/web-stories-theme-style-twentyseventeen-rtl.css 288 B 0 B
assets/css/web-stories-theme-style-twentyseventeen.css 288 B 0 B
assets/css/web-stories-theme-style-twentysixteen-rtl.css 224 B 0 B
assets/css/web-stories-theme-style-twentysixteen.css 224 B 0 B
assets/css/web-stories-theme-style-twentyten-rtl.css 143 B 0 B
assets/css/web-stories-theme-style-twentyten.css 143 B 0 B
assets/css/web-stories-theme-style-twentytwelve-rtl.css 256 B 0 B
assets/css/web-stories-theme-style-twentytwelve.css 256 B 0 B
assets/css/web-stories-theme-style-twentytwenty-rtl.css 86 B 0 B
assets/css/web-stories-theme-style-twentytwenty.css 86 B 0 B
assets/css/web-stories-theme-style-twentytwentyone-rtl.css 326 B 0 B
assets/css/web-stories-theme-style-twentytwentyone.css 326 B 0 B
assets/css/web-stories-widget-rtl.css 482 B 0 B
assets/css/web-stories-widget.css 482 B 0 B
assets/css/wp-dashboard-rtl.css 657 B 0 B
assets/css/wp-dashboard.css 659 B 0 B
assets/css/wp-story-editor-rtl.css 737 B 0 B
assets/css/wp-story-editor.css 738 B 0 B
assets/js/1814.js 7.48 kB 0 B
assets/js/4422.js 49.3 kB 0 B
assets/js/5369.js 90.4 kB 0 B
assets/js/5918.js 34.7 kB 0 B
assets/js/81.js 203 kB 0 B
assets/js/9750.js 12.8 kB 0 B
assets/js/carousel-view.js 3.41 kB 0 B
assets/js/chunk-colorthief.js 2.64 kB 0 B
assets/js/chunk-ffmpeg.js 5.89 kB 0 B
assets/js/chunk-focus-visible.js 1.01 kB 0 B
assets/js/chunk-html-to-image.js 4.5 kB 0 B
assets/js/chunk-opentype.js 96 B 0 B
assets/js/chunk-react-calendar.js 12.5 kB 0 B
assets/js/chunk-react-color.js 44.3 kB 0 B
assets/js/chunk-web-animations-js.js 14.6 kB 0 B
assets/js/chunk-web-stories-template-0-metaData.js 545 B 0 B
assets/js/chunk-web-stories-template-0.js 11.4 kB 0 B
assets/js/chunk-web-stories-template-1-metaData.js 539 B 0 B
assets/js/chunk-web-stories-template-1.js 9.61 kB 0 B
assets/js/chunk-web-stories-template-10-metaData.js 533 B 0 B
assets/js/chunk-web-stories-template-10.js 7.37 kB 0 B
assets/js/chunk-web-stories-template-11-metaData.js 539 B 0 B
assets/js/chunk-web-stories-template-11.js 9.09 kB 0 B
assets/js/chunk-web-stories-template-12-metaData.js 497 B 0 B
assets/js/chunk-web-stories-template-12.js 9.7 kB 0 B
assets/js/chunk-web-stories-template-13-metaData.js 525 B 0 B
assets/js/chunk-web-stories-template-13.js 7.4 kB 0 B
assets/js/chunk-web-stories-template-14-metaData.js 582 B 0 B
assets/js/chunk-web-stories-template-14.js 7.37 kB 0 B
assets/js/chunk-web-stories-template-15-metaData.js 544 B 0 B
assets/js/chunk-web-stories-template-15.js 9 kB 0 B
assets/js/chunk-web-stories-template-16-metaData.js 588 B 0 B
assets/js/chunk-web-stories-template-16.js 10.9 kB 0 B
assets/js/chunk-web-stories-template-17-metaData.js 539 B 0 B
assets/js/chunk-web-stories-template-17.js 9.2 kB 0 B
assets/js/chunk-web-stories-template-18-metaData.js 585 B 0 B
assets/js/chunk-web-stories-template-18.js 9.91 kB 0 B
assets/js/chunk-web-stories-template-19-metaData.js 501 B 0 B
assets/js/chunk-web-stories-template-19.js 10.8 kB 0 B
assets/js/chunk-web-stories-template-2-metaData.js 586 B 0 B
assets/js/chunk-web-stories-template-2.js 9.3 kB 0 B
assets/js/chunk-web-stories-template-20-metaData.js 548 B 0 B
assets/js/chunk-web-stories-template-20.js 9.01 kB 0 B
assets/js/chunk-web-stories-template-21-metaData.js 535 B 0 B
assets/js/chunk-web-stories-template-21.js 9.85 kB 0 B
assets/js/chunk-web-stories-template-22-metaData.js 525 B 0 B
assets/js/chunk-web-stories-template-22.js 7.83 kB 0 B
assets/js/chunk-web-stories-template-23-metaData.js 605 B 0 B
assets/js/chunk-web-stories-template-23.js 7.48 kB 0 B
assets/js/chunk-web-stories-template-24-metaData.js 516 B 0 B
assets/js/chunk-web-stories-template-24.js 11.7 kB 0 B
assets/js/chunk-web-stories-template-25-metaData.js 544 B 0 B
assets/js/chunk-web-stories-template-25.js 7.06 kB 0 B
assets/js/chunk-web-stories-template-26-metaData.js 601 B 0 B
assets/js/chunk-web-stories-template-26.js 7.27 kB 0 B
assets/js/chunk-web-stories-template-27-metaData.js 542 B 0 B
assets/js/chunk-web-stories-template-27.js 7.82 kB 0 B
assets/js/chunk-web-stories-template-28-metaData.js 532 B 0 B
assets/js/chunk-web-stories-template-28.js 9.07 kB 0 B
assets/js/chunk-web-stories-template-29-metaData.js 562 B 0 B
assets/js/chunk-web-stories-template-29.js 9.25 kB 0 B
assets/js/chunk-web-stories-template-3-metaData.js 539 B 0 B
assets/js/chunk-web-stories-template-3.js 8.42 kB 0 B
assets/js/chunk-web-stories-template-30-metaData.js 576 B 0 B
assets/js/chunk-web-stories-template-30.js 7.89 kB 0 B
assets/js/chunk-web-stories-template-31-metaData.js 502 B 0 B
assets/js/chunk-web-stories-template-31.js 10.3 kB 0 B
assets/js/chunk-web-stories-template-32-metaData.js 552 B 0 B
assets/js/chunk-web-stories-template-32.js 13.3 kB 0 B
assets/js/chunk-web-stories-template-33-metaData.js 492 B 0 B
assets/js/chunk-web-stories-template-33.js 9.07 kB 0 B
assets/js/chunk-web-stories-template-34-metaData.js 571 B 0 B
assets/js/chunk-web-stories-template-34.js 7.58 kB 0 B
assets/js/chunk-web-stories-template-35-metaData.js 566 B 0 B
assets/js/chunk-web-stories-template-35.js 8.91 kB 0 B
assets/js/chunk-web-stories-template-36-metaData.js 577 B 0 B
assets/js/chunk-web-stories-template-36.js 12.7 kB 0 B
assets/js/chunk-web-stories-template-37-metaData.js 528 B 0 B
assets/js/chunk-web-stories-template-37.js 6.71 kB 0 B
assets/js/chunk-web-stories-template-38-metaData.js 572 B 0 B
assets/js/chunk-web-stories-template-38.js 7.94 kB 0 B
assets/js/chunk-web-stories-template-39-metaData.js 588 B 0 B
assets/js/chunk-web-stories-template-39.js 8.08 kB 0 B
assets/js/chunk-web-stories-template-4-metaData.js 564 B 0 B
assets/js/chunk-web-stories-template-4.js 12.7 kB 0 B
assets/js/chunk-web-stories-template-40-metaData.js 557 B 0 B
assets/js/chunk-web-stories-template-40.js 10.2 kB 0 B
assets/js/chunk-web-stories-template-41-metaData.js 572 B 0 B
assets/js/chunk-web-stories-template-41.js 7.75 kB 0 B
assets/js/chunk-web-stories-template-42-metaData.js 521 B 0 B
assets/js/chunk-web-stories-template-42.js 7 kB 0 B
assets/js/chunk-web-stories-template-43-metaData.js 557 B 0 B
assets/js/chunk-web-stories-template-43.js 8.76 kB 0 B
assets/js/chunk-web-stories-template-44-metaData.js 584 B 0 B
assets/js/chunk-web-stories-template-44.js 11.1 kB 0 B
assets/js/chunk-web-stories-template-45-metaData.js 565 B 0 B
assets/js/chunk-web-stories-template-45.js 7.52 kB 0 B
assets/js/chunk-web-stories-template-46-metaData.js 531 B 0 B
assets/js/chunk-web-stories-template-46.js 5.22 kB 0 B
assets/js/chunk-web-stories-template-47-metaData.js 592 B 0 B
assets/js/chunk-web-stories-template-47.js 9.42 kB 0 B
assets/js/chunk-web-stories-template-48-metaData.js 555 B 0 B
assets/js/chunk-web-stories-template-48.js 9.09 kB 0 B
assets/js/chunk-web-stories-template-49-metaData.js 518 B 0 B
assets/js/chunk-web-stories-template-49.js 9.69 kB 0 B
assets/js/chunk-web-stories-template-5-metaData.js 556 B 0 B
assets/js/chunk-web-stories-template-5.js 9.94 kB 0 B
assets/js/chunk-web-stories-template-50-metaData.js 503 B 0 B
assets/js/chunk-web-stories-template-50.js 9.15 kB 0 B
assets/js/chunk-web-stories-template-51-metaData.js 526 B 0 B
assets/js/chunk-web-stories-template-51.js 10.4 kB 0 B
assets/js/chunk-web-stories-template-52-metaData.js 602 B 0 B
assets/js/chunk-web-stories-template-52.js 10.4 kB 0 B
assets/js/chunk-web-stories-template-53-metaData.js 553 B 0 B
assets/js/chunk-web-stories-template-53.js 5.78 kB 0 B
assets/js/chunk-web-stories-template-54-metaData.js 547 B 0 B
assets/js/chunk-web-stories-template-54.js 7.67 kB 0 B
assets/js/chunk-web-stories-template-55-metaData.js 573 B 0 B
assets/js/chunk-web-stories-template-55.js 7.13 kB 0 B
assets/js/chunk-web-stories-template-56-metaData.js 542 B 0 B
assets/js/chunk-web-stories-template-56.js 9.87 kB 0 B
assets/js/chunk-web-stories-template-57-metaData.js 527 B 0 B
assets/js/chunk-web-stories-template-57.js 14.9 kB 0 B
assets/js/chunk-web-stories-template-58-metaData.js 555 B 0 B
assets/js/chunk-web-stories-template-58.js 5.74 kB 0 B
assets/js/chunk-web-stories-template-59-metaData.js 590 B 0 B
assets/js/chunk-web-stories-template-59.js 8.96 kB 0 B
assets/js/chunk-web-stories-template-6-metaData.js 569 B 0 B
assets/js/chunk-web-stories-template-6.js 7.07 kB 0 B
assets/js/chunk-web-stories-template-60-metaData.js 510 B 0 B
assets/js/chunk-web-stories-template-60.js 9.51 kB 0 B
assets/js/chunk-web-stories-template-7-metaData.js 569 B 0 B
assets/js/chunk-web-stories-template-7.js 7.46 kB 0 B
assets/js/chunk-web-stories-template-8-metaData.js 569 B 0 B
assets/js/chunk-web-stories-template-8.js 8.93 kB 0 B
assets/js/chunk-web-stories-template-9-metaData.js 581 B 0 B
assets/js/chunk-web-stories-template-9.js 8.46 kB 0 B
assets/js/chunk-web-stories-templates.js 1.17 kB 0 B
assets/js/chunk-web-stories-textset-0.js 5.06 kB 0 B
assets/js/chunk-web-stories-textset-1.js 6.65 kB 0 B
assets/js/chunk-web-stories-textset-2.js 7.65 kB 0 B
assets/js/chunk-web-stories-textset-3.js 15.1 kB 0 B
assets/js/chunk-web-stories-textset-4.js 4.15 kB 0 B
assets/js/chunk-web-stories-textset-5.js 5.47 kB 0 B
assets/js/chunk-web-stories-textset-6.js 5.28 kB 0 B
assets/js/chunk-web-stories-textset-7.js 10.2 kB 0 B
assets/js/generateBlurhash.worker.worker.js 1.1 kB 0 B
assets/js/imgareaselect.js 3.77 kB 0 B
assets/js/lightbox.js 550 B 0 B
assets/js/tinymce-button.js 2.85 kB 0 B
assets/js/web-stories-activation-notice.js 27.1 kB 0 B
assets/js/web-stories-block.js 22.6 kB 0 B
assets/js/web-stories-embed.js 20 B 0 B
assets/js/web-stories-widget.js 587 B 0 B
assets/js/wp-dashboard.js 63.7 kB 0 B
assets/js/wp-story-editor.js 1.44 MB +61 B (0%)

compressed-size-action

@spacedmonkey
Copy link
Contributor Author

Should I remove references to improvedAutosaves, considering #12357. CC @timarney

@swissspidy swissspidy added the Type: Enhancement New feature or improvement of an existing feature label Sep 28, 2022
Copy link
Collaborator

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

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

Change looks OK but eyes from Prometheus pod would be great

@timarney
Copy link
Contributor

Should I remove references to improvedAutosaves, considering #12357. CC @timarney

Re-tested my other PR further last night --- seems to be working as expected.

showLockedDialog &&
user &&
hasNewChanges &&
!isFirstTime
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why only when !isFirstTime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The take over message should show until this is not the first time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To Miina's point, the variable wording is not the most intuitive, but at this point I guess it is what it is :-)

@swissspidy
Copy link
Collaborator

#12357 is merged now, so this can be updated here

@spacedmonkey
Copy link
Contributor Author

#12357 is merged now, so this can be updated here

Done in 6ceb693

@kkalarickal
Copy link

Tested this feature by logging in as an admin user and an editor user on two different browsers , and within the same browser, via two different sessions.

Confirmed that after take over by either admin or editor user, when the previous session is updated, but NOT saved, there is an auto-draft that is made. This is reflected in the other browser/session upon refresh so that changes made in the taken-over editor now reflect on the new owner of the editor session.

image.png

Test Variations:

  • use Chrome-Firefox, Chrome-Edge, Chrome-Safari as the old and new sessions
  • use Chrome on win10-OSX

@kkalarickal
Copy link

@spacedmonkey - wanted to run some scenarios by you to confirm this is expected:

Scenario 1:

When Admin user creates a story, but its taken over by Editor user, there is a notification sent to Admin user saying someone else has taken over.

But the Admin user is able to click outside that notification and continue editing the story. After some time, if the Admin user now reloads the editor, there are two overlays one-on-top-of-the-other

image.png

  • one is a backup notification
  • another is the take over notification

Isn't it confusing that the user has to deal with two notifications one on top of the other ?

Cc: @swissspidy

@kkalarickal
Copy link

Scenario 2:

After Editor user takes over a story Admin is editing, because the Admin can click outside the notification about someone taking over the story, and she can continue to edit her version of the story.

Later, if the Editor refreshes the open story that was taken over, the story is updated with the changes that the Admin made after takeover !

There seems to be a delay between when the story take over notice is sent to previous editor sessions, and how the local edits are auto-saved. As a result, both parties can to continue editing by skipping over the take over notification, leading to auto-saved edits being transferred between the two open editor instances.

@swissspidy
Copy link
Collaborator

But the Admin user is able to click outside that notification and continue editing the story.

This should not happen.

After some time, if the Admin user now reloads the editor, there are two overlays one-on-top-of-the-other

This should not happen.

Both would require new tickets to be opened.


@kkalarickal Could you test the same scenarios in the WordPress post editor so that we have something to compare against?

@timarney timarney mentioned this pull request Oct 4, 2022
8 tasks
@swissspidy swissspidy merged commit c65fc66 into main Oct 4, 2022
@swissspidy swissspidy deleted the fix/autosave-on-take-ver branch October 4, 2022 15:36
@kkalarickal
Copy link

@swissspidy - i created #12430 to track the issue with "user is able to click outside that notification and continue editing the story". This ticket also has the double overlay in it. I was not able to re-create the double overlay consistently, but thought I would include it in the ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Group: Story Locking aka post locking Group: WordPress Changes related to WordPress or Gutenberg integration P1 High priority, must do soon Type: Enhancement New feature or improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Story Locking: Autosave on takeover
6 participants