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.

Sign up for GitHub

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

Jump to bottom

Refactor build user update params #2657

Merged
merged 6 commits into from
Jan 25, 2024
Merged

Refactor build user update params #2657

merged 6 commits into from
Jan 25, 2024

Commits on Jan 25, 2024

  1. Parameterize user in build_user_update_params 

    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.
    @chrislo
    chrislo committed Jan 25, 2024
    Configuration menu
    Copy the full SHA
    fe58c8b View commit details
    Browse the repository at this point in the history

  2. Parameterize updatable_permission_ids in build_user_update_params 

    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.
    @chrislo
    chrislo committed Jan 25, 2024
    Configuration menu
    Copy the full SHA
    abe027a View commit details
    Browse the repository at this point in the history

  3. Parameterize selected_permission_ids in build_user_update_params 

    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.
    @chrislo
    chrislo committed Jan 25, 2024
    Configuration menu
    Copy the full SHA
    7682239 View commit details
    Browse the repository at this point in the history

  4. Extract supported_permission_ids variable 

    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.
    @chrislo
    chrislo committed Jan 25, 2024
    Configuration menu
    Copy the full SHA
    2ab9a6f View commit details
    Browse the repository at this point in the history

  5. Extract UserUpdatePermissionBuilder 

    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.
    @chrislo
    chrislo committed Jan 25, 2024
    Configuration menu
    Copy the full SHA
    f57a329 View commit details
    Browse the repository at this point in the history

  6. Remove some test duplication 

    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.
    @chrislo
    chrislo committed Jan 25, 2024
    Configuration menu
    Copy the full SHA
    beb5d26 View commit details
    Browse the repository at this point in the history