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

Fix incorrect current user index when adding or removing a user account. Also fix incorrect user avatar lookup by id. #5092

Merged
merged 2 commits into from Nov 3, 2022
Merged

Fix incorrect current user index when adding or removing a user account. Also fix incorrect user avatar lookup by id. #5092

merged 2 commits into from Nov 3, 2022

Conversation

allexzander
Copy link
Collaborator

No description provided.

@allexzander allexzander changed the title Bugfix/incorrect currentuserid Fix incorrect current user index when adding or removing a user account. Also fix incorrect user avatar lookup by id. Oct 25, 2022
@allexzander allexzander force-pushed the bugfix/incorrect-currentuserid branch 5 times, most recently from 179bdcf to 319c760 Compare October 25, 2022 14:13
@codecov
Copy link

codecov bot commented Oct 25, 2022 •
edited

Codecov Report

Merging #5092 (c7e3518) into master (3584265) will decrease coverage by 0.16%.
The diff coverage is n/a.

❗ Current head c7e3518 differs from pull request most recent head 6cb988b. Consider uploading reports for the commit 6cb988b to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #5092      +/-   ##
==========================================
- Coverage   57.38%   57.22%   -0.17%     
==========================================
  Files         138      138              
  Lines       17444    17441       -3     
==========================================
- Hits        10011     9981      -30     
- Misses       7433     7460      +27
Impacted Files Coverage Δ
src/libsync/capabilities.cpp 37.66% <0.00%> (-15.16%) ⬇️
src/common/syncjournalfilerecord.cpp 91.66% <0.00%> (-8.34%) ⬇️
src/common/utility_win.cpp 42.98% <0.00%> (-3.62%) ⬇️
src/libsync/httplogger.cpp 87.50% <0.00%> (-1.57%) ⬇️
src/libsync/theme.cpp 12.90% <0.00%> (-1.23%) ⬇️
src/libsync/networkjobs.cpp 50.28% <0.00%> (-0.43%) ⬇️
src/libsync/discoveryphase.cpp 73.18% <0.00%> (-0.28%) ⬇️
src/libsync/clientsideencryption.cpp 26.33% <0.00%> (-0.08%) ⬇️
src/libsync/account.h 33.33% <0.00%> (ø)
src/libsync/clientsideencryption.h 38.46% <0.00%> (ø)
... and 6 more

claucambra
claucambra reviewed Oct 27, 2022
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.

Looks fine, just some small changes I would make to make things tidier

src/gui/tray/usermodel.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
@allexzander allexzander force-pushed the bugfix/incorrect-currentuserid branch 2 times, most recently from 59d02ff to 1eed3ba Compare October 27, 2022 15:37
@allexzander
Copy link
Collaborator Author

@claucambra fixed your comments and also had to add more conditions after I discovered an issue with the current account being switched when it shouldn't when removing another account, this all happens because of using ids as indices in the list, while the same id is set in the Account instance as a member. I guess best would be to refactor the setCurrentUser such that it would not assume the index in the list to correspond to the Account's id, but, that of course would require much more time and re-testing of everything

claucambra
claucambra reviewed Nov 2, 2022
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.

A few nitpicks

src/gui/tray/usermodel.cpp Show resolved Hide resolved
src/gui/tray/usermodel.cpp Outdated Show resolved Hide resolved
src/gui/tray/usermodel.cpp Outdated Show resolved Hide resolved
@allexzander
Fix incorrect current user index when removing and adding an account.
b00007e 
Signed-off-by: alex-z <blackslayer4@gmail.com>
@allexzander
Fix accounts avatar issue when it becomes completely white due to non…
6cb988b 
…-changing account id member with changing account index in the list.

Signed-off-by: allexzander <blackslayer4@gmail.com>
@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-5092-6cb988b66788e11cef439269959a06bf1c1278f0-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.

@sonarcloud
Copy link

sonarcloud bot commented Nov 3, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

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

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@allexzander allexzander merged commit 34a59fb into master Nov 3, 2022
@allexzander allexzander deleted the bugfix/incorrect-currentuserid branch November 3, 2022 13:00
@mgallien mgallien added this to the 3.7.0 milestone Nov 8, 2022
@allexzander
Copy link
Collaborator Author

/backport to stable-3.6

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Nov 9, 2022
Closed
@allexzander allexzander mentioned this pull request Nov 9, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@claucambra claucambra claucambra approved these changes

@mgallien mgallien Awaiting requested review from mgallien

@camilasan camilasan Awaiting requested review from camilasan

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@allexzander @nextcloud-desktop-bot @claucambra @mgallien