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

Subscribe to correct LoginEvent name #507

Merged
merged 6 commits into from
Nov 7, 2023
Merged

Conversation

doobry-systemli
Copy link
Contributor

This fixes the last_login_time record being updated when authenticating through the app:users:checkpassword command.

@doobry-systemli doobry-systemli added the bug Something isn't working label Nov 6, 2023
This fixes the `last_login_time` record being updated when authenticating
through the `app:users:checkpassword` command.
Copy link

codecov bot commented Nov 6, 2023

Codecov Report

Merging #507 (ce1d943) into main (9268491) will decrease coverage by 0.01%.
The diff coverage is 33.33%.

@@             Coverage Diff              @@
##               main     #507      +/-   ##
============================================
- Coverage     36.02%   36.01%   -0.01%     
  Complexity     1137     1137              
============================================
  Files           185      185              
  Lines          4583     4584       +1     
============================================
  Hits           1651     1651              
- Misses         2932     2933       +1     
Files Coverage Δ
src/Event/RecoveryProcessEvent.php 0.00% <ø> (ø)
src/EventListener/LoginListener.php 89.65% <100.00%> (ø)
src/Command/UsersRegistrationMailCommand.php 0.00% <0.00%> (ø)

$mailLocation);
$commandTester = new CommandTester($command);

$commandTester->execute([]);
Copy link
Member

Choose a reason for hiding this comment

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

I do not see any assertation here. It would be good to assert that the command returns successfully and the mocked services were called as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the code logic now so that $this->loginListener->expects(self::once())->method('onLogin') is in the test function itself. Probably it's easier to follow the logic of the test that way.

I also added an assertion to make sure the execute() call succeeded, but that's not what we want to test here. We want to test whether LoginListener->onLogin() got triggered.

Copy link
Contributor

@t2d t2d left a comment

Choose a reason for hiding this comment

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

Thank you for adding the test case. Maybe you can adjust the changelog and we release a patch version afterwards?

@doobry-systemli doobry-systemli merged commit 321a83f into main Nov 7, 2023
10 of 12 checks passed
@doobry-systemli doobry-systemli deleted the fix/login_event branch November 7, 2023 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants