Skip to content

Conversation

@solebared
Copy link
Collaborator

@solebared solebared commented Apr 6, 2021

Why

Discovered that every organization currently created is defaulted to is_instance_owner: true.
IIUC, only one org should be the instance owner.

Also renames current_organization because that name connotes something transient, eg current_user.

What

  • Migration to change organizations.is_instance_owner default to false
  • Model validation to ensure no two organizations can be instance owners
  • Rename Organization.current_user -> .instance_owner
  • Change Organization.instance_owner from a scope to a class method since it doesn't return a relation

Testing

  • Added a model spec
  • Manually tested org form

Next steps

Outstanding Questions, Concerns and Other Notes

It's quite possible i'm completely misunderstanding the intention of current_organization!
@maebeale, how does this align with your initial/current thinking?

Accessibility

This does make it harder for admins to change which org is the instance owner. If that becomes a needed feature, we can add a 'transfer ownership' feature.

Pre-Merge Checklist

  • Security & accessibility have been considered
  • Tests have been added, or an explanation has been given why the features cannot be tested
  • Documentation and comments have been added to the codebase where required
  • Entry added to CHANGELOG.md if appropriate
  • Outstanding questions and concerns have been resolved
  • Any next steps have been turned into Issues or Discussions as appropriate

@solebared
Copy link
Collaborator Author

Whelp, i just noticed #240, where @maebeale explicitly says this should not be renamed :)
Let's discuss!

@solebared solebared force-pushed the org-instance-owner branch from be8500e to 9762e39 Compare April 8, 2021 15:25
@solebared solebared mentioned this pull request Apr 8, 2021
15 tasks
@maebeale
Copy link
Collaborator

maebeale commented Apr 8, 2021

Hey! I'm ok with this!

It doesn't return a Relation and when the find_by doesn't match, scopes
will return `.all`, which is quite surprising in this case.
`current` connotes something that can change (eg between requests),
whereas the instance_owner is a fixed org.
@solebared solebared force-pushed the org-instance-owner branch from 9762e39 to 901a630 Compare April 9, 2021 19:41
@solebared solebared merged commit 2ee89cb into main Apr 9, 2021
@solebared solebared deleted the org-instance-owner branch April 9, 2021 21:27
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.

3 participants