Separate MFA methods from User to its own concern #3108
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
889fcab to
cbfd737
Compare
|
Generally I'm in favour of this refactoring. Regarding the review questions:
|
|
Nice change. Regarding the naming and dummy class testing I personally do prefer to keep it as close as possible to real usage. This new module is not meant to be reused in multiple models. I think we can also just call it like |
simi
approved these changes
Jul 5, 2022
573c417 to
d0b88e5
Compare
d0b88e5 to
bc82bcb
Compare
|
Sounds good, that makes sense 👍 I ended up going with your suggestions and renamed the concern to |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6165
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Solves: #3012
Currently, there is a code climate violation suggesting that there were too many methods in
User.rbwhen working on the MFA work (#2994 (comment)). Before we move on with implementing requiring MFA, this issue should be addressed.This PR moves any mfa related methods from user into its own concern called
MultifactorAuthableand includes it in the User model.Reviewers should focus on
MultifactorAuthablemake sense? Better suggestions for names?