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 two factor authentication notification #4967

Merged
merged 2 commits into from Sep 27, 2022
Merged

Fix two factor authentication notification #4967

merged 2 commits into from Sep 27, 2022

Conversation

camilasan
Copy link
Member

@camilasan camilasan commented Sep 21, 2022 •
edited

Fix for #4841

@camilasan camilasan changed the title Fix issue Fix two factor authentication notification Sep 21, 2022
@camilasan
Copy link
Member Author

/backport to stable-3.6

1 similar comment
@camilasan
Copy link
Member Author

/backport to stable-3.6

@jancborchardt
Copy link
Member

@camilasan nice! :) Do you mind posting a screenshot for a much quicker design review? Thank you!

jancborchardt
jancborchardt approved these changes Sep 26, 2022
View reviewed changes
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

If it’s the screenshot from #4841 (comment), looks good! :)

The message does mention "Approve or deny" but the buttons are only "Approve" or "Cancel", so maybe they should be named accordingly?

@camilasan
Copy link
Member Author

camilasan commented Sep 26, 2022 •
edited

The message does mention "Approve or deny" but the buttons are only "Approve" or "Cancel", so maybe they should be named accordingly?

Yes, the message mentions it but the button in the response from the server do not have these names. It is Approve and Cancel.
That is why I said that this could be changed in the server side.

@camilasan
Copy link
Member Author

If it’s the screenshot from #4841 (comment), looks good! :)

Yes, that is the screenshot.

claucambra
claucambra reviewed Sep 26, 2022
View reviewed changes
src/gui/tray/ActivityItemActions.qml Outdated Show resolved Hide resolved
src/gui/tray/ActivityItemActions.qml Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 26, 2022 •
edited

Codecov Report

Merging #4967 (0dfd213) into master (9f952e3) will increase coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 0dfd213 differs from pull request most recent head 348c9ea. Consider uploading reports for the commit 348c9ea to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #4967      +/-   ##
==========================================
+ Coverage   56.98%   57.00%   +0.02%     
==========================================
  Files         138      138              
  Lines       17232    17232              
==========================================
+ Hits         9819     9823       +4     
+ Misses       7413     7409       -4
Impacted Files Coverage Δ
src/libsync/propagatedownload.cpp 64.75% <0.00%> (+0.14%) ⬆️
src/libsync/discovery.cpp 84.29% <0.00%> (+0.29%) ⬆️

claucambra
claucambra requested changes Sep 26, 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 couple of small details

src/gui/tray/ActivityActionButton.qml Outdated Show resolved Hide resolved
src/gui/tray/ActivityItemActions.qml Outdated Show resolved Hide resolved
@claucambra
Copy link
Collaborator

@camilasan It seems to work great for most notifications except missed call notifications, where we now have two primary buttons:

Screenshot 2022-09-26 at 18 22 09

I am guessing this is not intentional?

@camilasan
Copy link
Member Author

I am guessing this is not intentional?

Turns out 'call back' is a primary action but we also initially wanted 'Reply' to look like that. 'Reply' it is a desktop client only action, does not come from the server... wdyt @jancborchardt @nimishavijay?

@jancborchardt
Copy link
Member

At some point, "Reply" would also be good to have in call notifications for server. cc @nickvergessen

In the case of call notifications, "Call back" would be primary, "Reply" secondary.
For messages, "Reply" should stay primary of course.

@nickvergessen
Copy link
Member

At some point, "Reply" would also be good to have in call notifications for server.

nextcloud/notifications#637

@camilasan
Copy link
Member Author

In the case of call notifications, "Call back" would be primary, "Reply" secondary.
For messages, "Reply" should stay primary of course.

I will do this in another PR. @claucambra could you just check this PR one last time? Then I will clean up the git commit history.

@claucambra
Copy link
Collaborator

looks good now

@camilasan
Copy link
Member Author

/rebase

Camila added 2 commits September 27, 2022 14:49
Fix two factor auth notification: 'Approve' link is a primary action.
f3513aa 
Signed-off-by: Camila <hello@camila.codes>
Fix persistent two factor auth notification.
348c9ea 
The server app returns a 202 code when the request succeeded, which was
not taken into account.

Signed-off-by: Camila <hello@camila.codes>
@camilasan camilasan merged commit 71032ca into master Sep 27, 2022
@camilasan camilasan deleted the bugfix/issue-4841 branch September 27, 2022 14:52
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Sep 27, 2022
Merged
@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-4967-348c9ea91552cdbed2ef2505b7309dade5e3c833-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 Sep 27, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

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

@camilasan camilasan mentioned this pull request Sep 29, 2022
Merged
@mgallien mgallien added this to the 3.7.0 milestone Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jancborchardt jancborchardt jancborchardt approved these changes

@allexzander allexzander allexzander approved these changes

@claucambra claucambra claucambra approved these changes

@mgallien mgallien Awaiting requested review from mgallien

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
7 participants
@camilasan @jancborchardt @claucambra @nickvergessen @nextcloud-desktop-bot @allexzander @mgallien