Skip to content

Split sync of ApiService to sync and save#4424

Merged
blizzz merged 3 commits into
mainfrom
enh/noid/split-sync-save
Jul 31, 2023
Merged

Split sync of ApiService to sync and save#4424
blizzz merged 3 commits into
mainfrom
enh/noid/split-sync-save

Conversation

@blizzz

@blizzz blizzz commented Jun 29, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

📝 Summary

As discussed with @juliushaertl when talking about #3899 it might be worthwhile to split the sync route into dedicated sync and save endpoints.

🚧 TODO

  • probably have to adjust cypress
  • take care of PublicSessionController

🏁 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

@blizzz blizzz added enhancement New feature or request 2. developing labels Jun 29, 2023
@blizzz blizzz added this to the Nextcloud 28 milestone Jun 29, 2023
@cypress

cypress Bot commented Jun 29, 2023 •
edited
Loading

Copy link
Copy Markdown

Passing run #11441 ↗︎

0 149 2 0 Flakiness 0

Details:

Split sync of ApiService to sync and save
Project: Text Commit: 0dd749bbcf
Status: Passed Duration: 04:17 💡
Started: Jul 31, 2023 9:24 PM Ended: Jul 31, 2023 9:28 PM

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

juliusknorr
juliusknorr reviewed Jun 30, 2023
View reviewed changes
Comment thread src/services/SyncService.js
@@ -242,7 +242,7 @@ class SyncService {
async save({ force = false, manualSave = true } = {}) {

@juliusknorr juliusknorr Jun 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We may still want to trigger the autosave somehow regular. I think this is no longer the case as only sync is called in an interval. Might be something to check with @max-nextcloud next week on how to best achieve that.

@blizzz blizzz Jun 30, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's actually still being triggered regularly. At least yesterday it was 🙈

@blizzz blizzz Jun 30, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

And that's the 30s autosave:

text/src/services/SyncService.js

Line 88 in 0d088d6

this.autosave = debounce(this._autosave.bind(this), AUTOSAVE_INTERVAL)

@blizzz

This comment was marked as resolved.

Sign in to view
max-nextcloud
max-nextcloud requested changes Jul 11, 2023
View reviewed changes

@max-nextcloud max-nextcloud left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks a lot for tackling this!
I have only one small remark.
Just went over the code did not test locally or go through untouched code to see if further changes would be needed.

Comment thread lib/Service/ApiService.php Outdated
@max-nextcloud max-nextcloud self-requested a review July 11, 2023 11:47
@blizzz blizzz force-pushed the enh/noid/split-sync-save branch from bdf34f6 to 76f9f33 Compare July 14, 2023 18:20
@blizzz

blizzz commented Jul 14, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

squahsed and rebased

@blizzz blizzz force-pushed the enh/noid/split-sync-save branch 2 times, most recently from 80837f6 to f7107a5 Compare July 16, 2023 20:41
@blizzz

blizzz commented Jul 16, 2023

Copy link
Copy Markdown
Member Author

The remaining cypress errors about conflicts are rooted that this information was not transported on /sync anymore. This is fixed now. It works locally, but Cypress is still not convinced.… will continue there.

The handler we have, reacts to this status only on polling! Saves would be ignored, you could see the failures only on the console (before and after the changes). With the polling itself it's not a big deal.

@juliusknorr juliusknorr mentioned this pull request Jul 26, 2023
Merged
6 tasks
@blizzz blizzz force-pushed the enh/noid/split-sync-save branch from 75bdd5c to 4ec598e Compare July 31, 2023 15:57
@blizzz
refactor(api): split sync into sync and save endpoints
2c52b83 
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz force-pushed the enh/noid/split-sync-save branch 3 times, most recently from 2436346 to ecb1932 Compare July 31, 2023 20:30
blizzz added 2 commits July 31, 2023 23:21
@blizzz
fix(backend): conflicts were not detected on sync anymore
a6afd1d 
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
build(frontend): js assets
0dd749b 
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz blizzz force-pushed the enh/noid/split-sync-save branch from ecb1932 to 0dd749b Compare July 31, 2023 21:21
@blizzz

blizzz commented Jul 31, 2023

Copy link
Copy Markdown
Member Author

The remaining issue with cypress conflicts tests was an accidentally inverted condition, so that the proper exception was not thrown.

@blizzz blizzz merged commit cc0cb38 into main Jul 31, 2023
@blizzz blizzz deleted the enh/noid/split-sync-save branch July 31, 2023 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@juliusknorr juliusknorr juliusknorr left review comments

@max-nextcloud max-nextcloud max-nextcloud approved these changes

Assignees

No one assigned

Labels

2. developing enhancement New feature or request

Projects

None yet

Milestone

Nextcloud 28

Development

Successfully merging this pull request may close these issues.

3 participants

@blizzz @juliusknorr @max-nextcloud