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.

Sign up for GitHub

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

Jump to bottom

display subject when we have sync issues like network errors #5768

Merged
merged 6 commits into from
Jun 7, 2023
Merged

display subject when we have sync issues like network errors #5768

merged 6 commits into from
Jun 7, 2023

Conversation

mgallien
Copy link
Collaborator

@mgallien mgallien commented Jun 6, 2023

No description provided.

@mgallien
Copy link
Collaborator Author

mgallien commented Jun 6, 2023

before
image

after
image

@codecov
Copy link

codecov bot commented Jun 6, 2023 •
edited
Loading

Codecov Report

Merging #5768 (51b2fd9) into master (d63eb6a) will decrease coverage by 0.04%.
The diff coverage is n/a.

❗ Current head 51b2fd9 differs from pull request most recent head 71fbc86. Consider uploading reports for the commit 71fbc86 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #5768      +/-   ##
==========================================
- Coverage   60.37%   60.33%   -0.04%     
==========================================
  Files         143      143              
  Lines       18518    18518              
==========================================
- Hits        11180    11173       -7     
- Misses       7338     7345       +7

see 5 files with indirect coverage changes

@mgallien
Copy link
Collaborator Author

mgallien commented Jun 6, 2023

@nimishavijay @jancborchardt @karlitschek
any feedback on the exact message ?
we want to tell the user that the error is a network error
I may be able to fix the missing folder information but it may be hard to tell in a precise/reliable way why there is a network issue

@tobiasKaminsky
Copy link
Member

I would use a more generic error message, like: Network problem while syncing xyz. Client will retry automatically again.

This way user knows what the problem is and that they do not have to do anything.

@mgallien mgallien force-pushed the bugfix/errorMessageConnectionIssues branch from 2b90806 to 51b2fd9 Compare June 6, 2023 15:07
@mgallien
Copy link
Collaborator Author

mgallien commented Jun 6, 2023

new version
image

@mgallien mgallien force-pushed the bugfix/errorMessageConnectionIssues branch from 1ed39c5 to 3c9c87a Compare June 7, 2023 07:41
camilasan
camilasan requested changes Jun 7, 2023
View reviewed changes
src/gui/tray/activitylistmodel.cpp Outdated Show resolved Hide resolved
claucambra
claucambra reviewed Jun 7, 2023
View reviewed changes
Copy link
Collaborator

@claucambra claucambra left a comment

Choose a reason for hiding this comment

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

Just comments on constifying method parameters

src/gui/tray/usermodel.cpp Outdated Show resolved Hide resolved
src/gui/tray/activitylistmodel.h Outdated Show resolved Hide resolved
src/gui/tray/activitylistmodel.cpp Outdated Show resolved Hide resolved
src/gui/tray/usermodel.cpp Outdated Show resolved Hide resolved
src/gui/tray/usermodel.cpp Outdated Show resolved Hide resolved
src/libsync/syncengine.h Outdated Show resolved Hide resolved
src/libsync/syncengine.h Outdated Show resolved Hide resolved
src/libsync/syncengine.h Outdated Show resolved Hide resolved
src/libsync/syncengine.h Outdated Show resolved Hide resolved
src/libsync/syncengine.h Outdated Show resolved Hide resolved
@camilasan
Copy link
Member

/backport to stable-3.9

@mgallien mgallien force-pushed the bugfix/errorMessageConnectionIssues branch 2 times, most recently from 4886962 to 879f5ef Compare June 7, 2023 12:53
@mgallien mgallien requested review from camilasan and claucambra June 7, 2023 12:53
@mgallien
Copy link
Collaborator Author

mgallien commented Jun 7, 2023

latest state
image

@mgallien mgallien enabled auto-merge June 7, 2023 13:30
mgallien added 3 commits June 7, 2023 17:25
@mgallien
add more metadata to sync errors to allow filtering
5372276 
in order to be able to filter some errors when showing them into the
main dialog activity list, add some more info about the error to know
the origin (like a network issue or a sync issue)

Signed-off-by: Matthieu Gallien <[email protected]>
@mgallien
use a better user message for network errors
fe748fa 
tell users this is a network error and the client wll retry
automatically

Signed-off-by: Matthieu Gallien <[email protected]>
@mgallien
disable one definition rule sanitizer check
2a41b81 
Signed-off-by: Matthieu Gallien <[email protected]>
@mgallien mgallien force-pushed the bugfix/errorMessageConnectionIssues branch from 879f5ef to 2a41b81 Compare June 7, 2023 15:25
@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-5768-71fbc8620dce95215748c43d329000d75482fc28-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 7, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 10 Code Smells

34.2% 34.2% Coverage
0.0% 0.0% Duplication

@mgallien mgallien merged commit 3e4dc5d into master Jun 7, 2023
@mgallien mgallien deleted the bugfix/errorMessageConnectionIssues branch June 7, 2023 16:22
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Jun 7, 2023
Merged
@mgallien mgallien added this to the 3.10.0 milestone Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camilasan camilasan camilasan approved these changes

@claucambra claucambra claucambra approved these changes

@allexzander allexzander Awaiting requested review from allexzander

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.10.0
Development

Successfully merging this pull request may close these issues.
5 participants
@mgallien @tobiasKaminsky @camilasan @nextcloud-desktop-bot @claucambra