trafficstars

Fix for #4841

Sep 21 '22 15:09 camilasan

/backport to stable-3.6

Sep 21 '22 15:09 camilasan

/backport to stable-3.6

Sep 26 '22 11:09 camilasan

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

Sep 26 '22 11:09 jancborchardt

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.

Sep 26 '22 12:09 camilasan

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

Yes, that is the screenshot.

Sep 26 '22 12:09 camilasan

Codecov Report

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

:exclamation: 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%)`	:arrow_up:
src/libsync/discovery.cpp	`84.29% <0.00%> (+0.29%)`	:arrow_up:

Sep 26 '22 15:09 codecov[bot]

@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?

Sep 26 '22 16:09 claucambra

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?

Sep 27 '22 09:09 camilasan

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.

Sep 27 '22 09:09 jancborchardt

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

https://github.com/nextcloud/notifications/issues/637

Sep 27 '22 10:09 nickvergessen

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.

Sep 27 '22 11:09 camilasan

looks good now

Sep 27 '22 12:09 claucambra

/rebase

Sep 27 '22 14:09 camilasan

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.