-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
Make it possible to disable tenants #5050
Conversation
04e5804
to
6afe487
Compare
6afe487
to
60ffc56
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @javierm, I left just a single comment. Let me know what you think!
Hi again @javierm, While testing locally, I found disabling subdomains tenants works fine but disabling domain tenants is not working. I tested it in the development environment by adding the domain I gave to the tenant in my machine After digging, I found the cause could be in the |
60ffc56
to
67328f2
Compare
@Senen Great! I've updated the pull request so we raise an exception when accessing a hidden tenant with a domain, just like we do when accessing a hidden tenant with a subdomain. |
67328f2
to
b334f20
Compare
This is consistent with the way we use separate actions to hide and restore records, which is similar to enabling and disabling a record. We might do something similar with the `toggle_selection` actions in the future. For now, we're only doing it with budget phases because we're going to add a similar switch control to hide and restore tenants. We're also making these actions idempotent, so sending many requests to the same action will get the same result, which wasn't the case with the `toggle` action. Although it's a low probability case, the `toggle` action could result in disabling a phase when trying to enable it if someone else has enabled it between the time the page loaded and the time the admin clicked on the "enable" button.
We're going to use it in other places.
We were returning `nil` in this case, meaning we would be accessing the main tenant in this situation instead of returning "Not Found".
Note we could use `acts_as_paranoid` with the `without_default_scope` option, but we aren't doing so because it isn't possible to consider deleted records in uniqueness validations with the paranoia gem [1]. I've added tests for these cases so we don't accidentally add `acts_as_paranoid` in the future. Also note we're extracting a `RowComponent` because, when enabling/disabling a tenant, we're also enabling/disabling the link pointing to its URL, and so we need to update the URL column after the AJAX call. [1] See issues 285 and 319 in https://github.com/rubysherpas/paranoia/
b334f20
to
25435b0
Compare
References
Background
We don't allow removing a tenant because it would remove all its data, which would be a dangerous operation. However, there might be cases where we don't want a tenant to be accessible, and there's currently no way to do so.
Objectives
Visual Changes
Before these changes
After these changes