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

Split sync of ApiService to sync and save #4424

Merged
merged 3 commits into from Jul 31, 2023
Merged

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Jun 29, 2023

📝 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
Copy link

cypress bot commented Jun 29, 2023

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.

@@ -242,7 +242,7 @@ class SyncService {
async save({ force = false, manualSave = true } = {}) {
Copy link
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.

Copy link
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 🙈

Copy link
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:

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

@blizzz

This comment was marked as resolved.

Copy link
Collaborator

@max-nextcloud max-nextcloud left a comment

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.

lib/Service/ApiService.php Outdated Show resolved Hide resolved
@max-nextcloud max-nextcloud self-requested a review July 11, 2023 11:47
@blizzz
Copy link
Member Author

blizzz commented Jul 14, 2023

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
Copy link
Member Author

blizzz commented Jul 16, 2023

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.

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
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
Copy link
Member Author

blizzz commented Jul 31, 2023

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
41 checks passed
@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
Labels
2. developing enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants