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

Conflict fixes #3972

Merged
merged 7 commits into from
Mar 24, 2023
Merged

Conflict fixes #3972

merged 7 commits into from
Mar 24, 2023

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Mar 21, 2023

Couple of fixes around conflict handling and file save, split into individual commits for easier review:

  • test: Add cypress tests for resolving conflicts
  • fix: store actually used mtime
  • fix: Properly restart sync after conflict resolution
  • fix: Avoid writing file content if it has not changed
  • fix: Fix styling of collision dialog and move it out of the editor
  • fix: Do not disconnect during conflict to be able to catch up on a resolution

There is still some work left that probably involves design work to come up with better handling of the conflict display on mobile devices, but that is left out for now to have a more easily back portable change.

📝 Summary

🖼️ Screenshots

🏚️ Before 🏡 After
Screenshot 2023-03-22 at 18 44 30 Screenshot 2023-03-22 at 18 44 09

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

@cypress
Copy link

cypress bot commented Mar 21, 2023

3 flaky tests on run #9199 ↗︎

0 142 1 0 Flakiness 3

Details:

Conflict fixes
Project: Text Commit: ed977b5303
Status: Passed Duration: 03:44 💡
Started: Mar 24, 2023 7:17 AM Ended: Mar 24, 2023 7:21 AM
Flakiness  sync.spec.js • 1 flaky test

View Output Video

Test Artifacts
Sync > recovers from a lost connection Output Screenshots
Flakiness  share.spec.js • 1 flaky test

View Output Video

Test Artifacts
Open test.md in viewer > Share a file with download disabled shows an error Output Screenshots
Flakiness  conflict.spec.js • 1 flaky test

View Output Video

Test Artifacts
Open test.md in viewer > resolves conflict using current editing session Output Screenshots

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@@ -561,10 +563,6 @@ export default {
},

onChange({ document, sessions }) {
if (this.document.baseVersionEtag !== '' && document.baseVersionEtag !== this.document.baseVersionEtag) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This behaviour is no longer valid and should be handled more gracefully as per #3963

@juliusknorr juliusknorr added the bug Something isn't working label Mar 21, 2023
@juliusknorr juliusknorr added this to the Nextcloud 27 milestone Mar 21, 2023
@juliusknorr juliusknorr force-pushed the bugfix/noid/conflict branch from 9d3694a to 9bfac57 Compare March 22, 2023 17:39
@juliusknorr juliusknorr force-pushed the bugfix/noid/conflict branch from 9bfac57 to 538d05b Compare March 22, 2023 17:46
@juliusknorr
Copy link
Member Author

/backport to stable26

@juliusknorr
Copy link
Member Author

Will check the cypress test tomorrow, seems we should await the sync request after pushing the state as it passed before my squashing push.

Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

Nice, thanks for tackling this! Code looks good to me 👌

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
…solution

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr force-pushed the bugfix/noid/conflict branch from e879ad0 to eeca470 Compare March 24, 2023 06:52
@juliusknorr
Copy link
Member Author

/compile

Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release 26 feedback bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Conflict resolution is broken
3 participants