Refactor build user update params #2657
Merged
Conversation
This file contains 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
chrislo
force-pushed
the
refactor_build_user_update_params
branch
2 times, most recently
from
5d44e19
to
a034b98
Compare
This method is effectively duplicated across three controllers. I'd like to extract it to remove the duplication. Parameterizing the arguments rather than relying on shared state will help me do that.
This method is effectively duplicated across three controllers. I'd like to extract it to remove the duplication. Parameterizing the arguments rather than relying on shared state will help me do that.
This method is effectively duplicated across three controllers. I'd like to extract it to remove the duplication. Parameterizing the arguments rather than relying on shared state will help me do that.
This makes the call to UserUpdate.new easier to follow and makes the return value of build_user_update_params less tightly coupled to how it is eventually used.
This commit moves the identical build_user_update_params method from each of the three controllers into a class. I think this is an improvement as it ensures that the business logic is defined in a single place and can't drift out of sync. The implementation of this method is currently covered by the controller tests but in a subsequent commit I'll introduce some unit tests for this class to help reduce the duplication in the controller tests.
We test these two scenarios multiple times in three different controller tests. Now the underlying logic has been extracted to UserUpdatePermissionBuilder I think they can be replaced with simpler and more comprehensive tests of `UserUpdatePermissionBuilder#build`. Note the remaining tests of the update method in the three permissions controllers are still useful - they effectively test that the correct values are set by the `set_application` and `set_permissions` methods on those controllers. I considered refactoring these tests to `expect` that `UserUpdatePermissionBuilder` is called correctly, but decided to keep things as they are for now. This change still feels like an improvement.
|
Thanks @chrisroos - rebasing before merging. |
chrislo
force-pushed
the
refactor_build_user_update_params
branch
from
a034b98
to
beb5d26
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Trello: https://trello.com/c/yDM25WlZ
This PR is an attempt to remove some of the duplication we added in c723644, edf2e83 and 92c5f5c with the
build_user_update_paramscontroller method. I did consider folding some of this logic intoUserUpdatebut as we're currently thinking about simplifying or removing that altogether (e.g. #2623) I think for now it's an improvement to have the code shared between the three controllers via a simple Ruby object. It'll at least make it easier to move somewhere else in the future.