Tidy up organisation-related code #3178
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
yndajas
force-pushed
the
simplify-organisation-selection
branch
3 times, most recently
from
9a7bc28
to
037f5b3
Compare
yndajas
changed the title
yndajas
force-pushed
the
simplify-organisation-selection
branch
from
037f5b3
to
cc6539d
Compare
yndajas
force-pushed
the
simplify-organisation-selection
branch
from
cc6539d
to
28b1e03
Compare
Gweaton
approved these changes
Oct 1, 2024
These aren't used anywhere and have no tests
We order them by name in a few places. In the organisations index they're in whatever order the database returns them in, which feels odd. This adds a sensible default for now, similar to other models (`ApiUser`, `Doorkeeper::Application`, and `SupportedPermission`)
The use of a non-standard dash here made some test failure output hard to read. I think this was using an em dash instead of en, or en instead of hyphen, but I think we can just use the standard dash and leave it to the browser to render appropriately
This wasn't being used in the organisations index. We had custom logic which resulted in organisations without abbreviations having empty brackets after their name, which looked odd. This method separates the name and abbreviation with a dash rather than putting the abbreviation in brackets, but there doesn't seem to be an obvious reason why both uses can't be the same The dash used in the method was a non-standard character, so that's fixed here
These are essentially doing the same thing, so there's no need to have multiple methods for them I've deleted a test from a controller spec that duplicates a helper test added here I had considered removing the `Organisation` policy scope. The scope gives GOV.UK admins access to all organisations and everyone else access to none, but is only used in places that are accessible only to GOV.UK admins (also based on the organisation policy), so it felt like a bit of a double-authorisation situation. However, George and I felt a bit uneasy about the potential for the `options_for_organisation_select` method being used elsewhere in future, and the authorisation therefore being skipped. As a result, we've retained the double authorisation, and I've included a scope-based context in the helper tests Another thing I spotted is that the `User` model has a `manageable_organisations` method, in turn relying on e.g. `Roles::Admin.manageable_organisations_for` or `Roles::SuperOrganisationAdmin.manageable_organisations_for`. This is used for filtering users by organisation in the users page/table, rather than populating a select field when creating/editing users, but the name makes the remits a little hard to differentiate without close inspection
yndajas
force-pushed
the
simplify-organisation-selection
branch
from
28b1e03
to
8075a72
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
This tidies up a bunch of organisation-related code, mostly stuff spotted while working on adding autocomplete to the organisation fields. In particular:
This application is owned by the publishing platform team. Please let us know in #govuk-publishing-platform when you raise any PRs.
Follow these steps if you are doing a Rails upgrade.