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.

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

Dismiss notifications #5687

Merged
merged 1 commit into from
May 16, 2023
Merged

Dismiss notifications #5687

merged 1 commit into from
May 16, 2023

Conversation

camilasan
Copy link
Member

@camilasan camilasan commented May 12, 2023

Any notification missing a dismiss link will get one like in the server:

2fa-and-dismiss

@camilasan camilasan force-pushed the bugfix/notifications branch from fa333ef to dcd7783 Compare May 12, 2023 16:38
@camilasan camilasan changed the title Fix for 2FA notifications Fix for (2FA) notifications May 12, 2023
@camilasan camilasan force-pushed the bugfix/notifications branch from dcd7783 to 3b9cc7c Compare May 12, 2023 16:43
@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Merging #5687 (d6db8fc) into master (6d6f9a8) will decrease coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5687      +/-   ##
==========================================
- Coverage   59.28%   59.27%   -0.01%     
==========================================
  Files         143      143              
  Lines       18463    18463              
==========================================
- Hits        10945    10944       -1     
- Misses       7518     7519       +1     

see 1 file with indirect coverage changes

@camilasan camilasan marked this pull request as draft May 12, 2023 19:11
@camilasan camilasan force-pushed the bugfix/notifications branch from 3b9cc7c to dae575e Compare May 15, 2023 13:07
@camilasan camilasan marked this pull request as ready for review May 15, 2023 13:08
@camilasan camilasan force-pushed the bugfix/notifications branch from dae575e to 3ab72ab Compare May 15, 2023 13:08
@camilasan camilasan changed the title Fix for (2FA) notifications Dismiss notifications May 15, 2023
Copy link
Collaborator

@mgallien mgallien left a comment

Choose a reason for hiding this comment

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

see my question on the comment

test/testactivitylistmodel.cpp Outdated Show resolved Hide resolved
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 good to me, though Matthieu's comment would be good to fix

@camilasan camilasan force-pushed the bugfix/notifications branch from 3ab72ab to 0931c1b Compare May 16, 2023 06:42
- Fix for #5606 and #5585.
- That is how it is done in the server and other clients.
- Update tests.

Signed-off-by: Camila <[email protected]>
@mgallien mgallien force-pushed the bugfix/notifications branch from 0931c1b to d6db8fc Compare May 16, 2023 09:17
@sonarqubecloud
Copy link

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

@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-5687-d6db8fcbd6247909feb3fb7508a56f5d2ce36562-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.

@camilasan camilasan merged commit 2d27e4a into master May 16, 2023
@camilasan camilasan deleted the bugfix/notifications branch May 16, 2023 10:12
@mgallien mgallien added this to the 3.9.0 milestone Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants