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

Add method to clear notification when user is deletd #32260

Closed
wants to merge 1 commit into from

Conversation

sharidas
Copy link
Contributor

@sharidas sharidas commented Aug 7, 2018

Added a method to clear notification entry of user
when the user is deleted from oC.

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

User entries from notification table should be deleted, once the user is deleted from oC.

Related Issue

  • Fixes <issue_link>

Motivation and Context

User entries from notification table should be deleted, once the user is deleted from oC.

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)

@sharidas sharidas self-assigned this Aug 7, 2018
@sharidas sharidas force-pushed the clear-notifications-after-user-deleted branch from e2aa77e to 032d6d8 Compare August 7, 2018 07:25
@sharidas
Copy link
Contributor Author

sharidas commented Aug 7, 2018

Fixes -> owncloud/password_policy#69

@sharidas sharidas force-pushed the clear-notifications-after-user-deleted branch from 032d6d8 to b42084e Compare August 7, 2018 08:28
@codecov
Copy link

codecov bot commented Aug 7, 2018

Codecov Report

Merging #32260 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #32260      +/-   ##
============================================
+ Coverage     64.01%   64.01%   +<.01%     
- Complexity    18559    18562       +3     
============================================
  Files          1171     1171              
  Lines         69834    69841       +7     
  Branches       1267     1267              
============================================
+ Hits          44704    44711       +7     
  Misses        24761    24761              
  Partials        369      369
Flag Coverage Δ Complexity Δ
#javascript 52.81% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 65.29% <100%> (ø) 18562 <3> (+3) ⬆️
Impacted Files Coverage Δ Complexity Δ
lib/private/Notification/Manager.php 98.18% <100%> (+0.12%) 45 <3> (+3) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13b3d02...fefb2ca. Read the comment docs.

/**
* @param $uid
* @return null
* @since 10.1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can put 10.0.10 here

Copy link
Contributor

Choose a reason for hiding this comment

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

PHPDoc is incomplete

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

see comment

Added a method to clear notification entry of user
when the user is deleted from oC.

Signed-off-by: Sujith H <sharidasan@owncloud.com>
@sharidas sharidas force-pushed the clear-notifications-after-user-deleted branch from b42084e to fefb2ca Compare August 7, 2018 11:59
@jvillafanez
Copy link
Member

What are the reasons to provide the solution here?

This implies that the core's notification manager knows that the notification is stored by the apps, which implies that is expected that the apps store notifications somewhere, which leads to store duplicate information (by different apps).

Core doesn't need to know whether the notifications are being stored. If the notifications app or any other provider stores notifications it's their own responsability to clean them.

I'd prefer to move the solution to the notification app: just listen the delete event there and remove the stored notifications. No need to change the core's interface.

@sharidas
Copy link
Contributor Author

sharidas commented Aug 7, 2018

@jvillafanez

I'd prefer to move the solution to the notification app: just listen the delete event there and remove the stored notifications. No need to change the core's interface.

In fact you are right, the core should not bother about the changes I made in this PR. The notification app should listen to the event and then do the action, i.e, to remove the entry of the user from the table.
Thanks for the suggestion.

@PVince81
Copy link
Contributor

PVince81 commented Aug 8, 2018

@sharidas please link to your new PR in the notifications app then close

@sharidas
Copy link
Contributor Author

sharidas commented Aug 9, 2018

The notification PR owncloud/notifications#221 is sufficient to achieve the result. Hence closing this PR.

@sharidas sharidas closed this Aug 9, 2018
@sharidas sharidas deleted the clear-notifications-after-user-deleted branch August 9, 2018 07:44
@lock lock bot locked as resolved and limited conversation to collaborators Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants