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

Refactor notification handling for password_policy #40

Open
jvillafanez opened this issue Jul 12, 2018 · 2 comments
Open

Refactor notification handling for password_policy #40

jvillafanez opened this issue Jul 12, 2018 · 2 comments

Comments

@jvillafanez
Copy link
Member

Unittests revealed several complexities around the notification handling area. There are also some loose ends such as depending on storing the old password id as a marker to know if a notification has been sent for the user (logic to properly handling this is spread into several classes)

Refactor plan

  1. Create a class to wrap the notification sending.
    • The class will have a sendNotificationFor(OldPassword $passInfo) method and will return true if the notification is "claimed" to be sent (core doesn't return anything for this)
    • The class will also have a removeNotificationFor(OldPassword $passInfo) to remove the notification.
    • The class will depend on the notification manager and the UserNotificationConfigHandler classes
  2. This will have the following simplications:
    • The HookHandler class will depend on this new class instead of both notification manager and UserNotificationConfigHandler (code should be easier)
    • The only minor problem is that the HookHandler will need to retrieve the last old password before inserting the new one, but it's expected that the new class handles the rest (getting and resetting the marks, and remove the notifications) via the removeNotificationFor() method
    • Unittest for the HookHandler will be greatly simplified.
    • Something similar is expected for the background job because the problems are similar to the ones of the HookHandler
  3. Tests for this new class will be slightly problematic due to how the notification manager is designed.
    • There won't be any check about the notification that has to be sent (way too complex unless we use an specific implementation). Tests in this regard will be mostly about checking whether a notification is sent or processed, but not about the contents of the notification.
    • The rest of the things to test in the class should be manageable
@PVince81
Copy link
Contributor

@jvillafanez do you have an estimate for the changes ?

@jvillafanez
Copy link
Member Author

I'd say 2-3md

@PVince81 PVince81 added this to the backlog milestone Jul 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants