Conversation
3 failed and 2 flaky tests on run #12080 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
||||||||||||||||||||
mejo-
force-pushed
the
fix/editor_api
branch
from
ac6c23b to
1527f9a
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
src/services/SyncService.js
Outdated
| this.autosave.clear() | ||
| } catch (e) { | ||
| logger.error('Failed to save document.', { error: e }) | ||
| throw e |
There was a problem hiding this comment.
I thought I commented that already, but i feel this might be a risky change if the save fails during other code parts calling this method. Can we safeguard that other callers catch this error then and handle it gracefully?
There was a problem hiding this comment.
Good point. On the other hand for the API usage it would be really handy to be able to detect failures when saving 🤔
There was a problem hiding this comment.
If we can get this in and backported more quickly without the throw e change, I'm fine with postponing this one 😬
There was a problem hiding this comment.
I had a look at all occasions of .save(), .autosave() and .forceSave(). Seems to be mostly places where throwing an error should be save. But maybe it's better to remove this potentially dangerous change from this PR that is supposed to be backported and move it to a seperate PR that doesn't get backported.
juliusknorr
left a comment
juliusknorr
left a comment
There was a problem hiding this comment.
Looks good besides the one comment
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
mejo-
force-pushed
the
fix/editor_api
branch
2 times, most recently
from
90f71d5 to
fb1f4e0
Compare
max-nextcloud
left a comment
max-nextcloud
left a comment
There was a problem hiding this comment.
reviewed and looks good to me.
Did not investigate julius' comment on the trow in SyncService.js yet. Is that needed / related?
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: Jonas <jonas@freesources.org>
|
Ready for review from my side now. |
|
/backport to stable27 |
|
/backport to stable26 |
3 participants
📝 Summary
This is required for Collectives to migrate to the editor API. Changes are separated by commits.
🏁 Checklist
npm run lint/npm run stylelint/composer run cs:check)