Add CompositeLogoutHandler#3904
Conversation
|
Thanks for the PR @eddumelendez! In general this looks pretty good. I provided some comments inline. Additionally, can we get unit tests for CompsiteLogoutHandler added? |
There was a problem hiding this comment.
Please change the type to LogoutHandler
|
@rwinch PR is updated. Comments have been fixed and new test was added to cover |
|
@eddumelendez Please sign the Contributor License Agreement! |
|
@eddumelendez Thank you for signing the Contributor License Agreement! |
|
@eddumelendez Thank you for signing the Contributor License Agreement! |
|
@rwinch any feedback about this? |
There was a problem hiding this comment.
Can we add a test that ensures that the logoutHandlers that are injected are invoked when CompositeLogoutHandler.logout is invoked? This would likely work well with a mock.
We should also document what happens if an Exception happens in one of the LogoutHandlers (the remaining LogoutHandlers won't be invoked) and have a test to verify the documentation is correct.
|
@eddumelendez Bump |
|
@rwinch sorry for the delay. PR updated! |
@eddumelendez No problem...just was not like you since you are always on top of things :) This is now merged into master via 1effc18 I also applied a bit of polish via 70787fc NOTE: I modified the tests so that they weren't dependent on the name of the private variable. I'd prefer to keep tests verifying behavior rather than if a member variable is set or not. This is something that is no consistent w/ the codebase, but I'd like to keep it this way going forward. |
Fixes gh-3895