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

SMS prove possession route not found #288

Merged
merged 2 commits into from
Apr 13, 2023

Conversation

MKodde
Copy link
Member

@MKodde MKodde commented Apr 13, 2023

The ss_registration_sms_prove_possession might have been skipped by the
SMS RT proof op posession where it was removed. During that action this
route was also removed. But it was still used in the token registration
of an SMS token.

The other commit states that: When the max attempts are expired, the view vars did not include the
verifyEmail property. That was fixed in this boy scout commit.

See: https://www.pivotaltracker.com/story/show/184750138

When the max attempts are expired, the view vars did not include the
verifyEmail property. That was fixed in this boy scout commit.
The ss_registration_sms_prove_possession might have been skipped by the
SMS RT proof op posession where it was removed. During that action this
route was also removed. But it was still used in the token registration
of an SMS token.

See: https://www.pivotaltracker.com/story/show/184750138
@MKodde MKodde force-pushed the bugfix/sms-registration-route-not-found branch from 907afc2 to 40e49a6 Compare April 13, 2023 10:10
@MKodde
Copy link
Member Author

MKodde commented Apr 13, 2023

Short summary of our IRL review session:

  1. Ruben noticed the lack of automated tests in this PR, and I explained we do not have a very practical way to write tests for controller logic. The one possibility is to add behat tests in the openconext deploy repository. And to my shame I have to admit we run them seldom.
  2. We discussed different test types and how to utilize them
  3. Looking at the controller logic we touched on the SOLID design principles and measured the controller logic against them. Ouch..

@MKodde MKodde merged commit 42ba348 into develop Apr 13, 2023
@MKodde MKodde deleted the bugfix/sms-registration-route-not-found branch April 13, 2023 12:52
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.

None yet

2 participants