Don't allow adding domains on subprojects - #663
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a notification message to inform users when a project is a subproject that custom domains must be managed by the superproject, and disables the domain list form in this case.
- Added
blocktransto i18n imports to support translatable messages with variable interpolation - Added informational message with links when a project has a superproject
- Updated the disabled segment condition to include the superproject check
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {% endif %} | ||
| <div class="{% if not enabled %}ui basic fitted disabled segment{% endif %}"> | ||
| <div class="{% if not enabled or project.superproject %}ui basic fitted disabled segment{% endif %}"> | ||
| {% include "projects/partials/edit/domain_list.html" with objects=object_list %} |
There was a problem hiding this comment.
In the subproject listing UI, we only display the listing if it's possible to use the listing UI and create subprojects. We should probably do that here instead of just disabling the segment visually.
There was a problem hiding this comment.
Yeah, I opened an issue about this #665. I'm not sure if there is a case where we want to block the listing if there are objects. The create action should be disabled, but the listing should still be available to users.
There was a problem hiding this comment.
That seems like it should be a validation error on adding a subproject, we shouldn't allow a subproject to be added to a superproject if it has a custom domain. I doubt there would be a lot of projects in this edge case though.
If we're concerned about projects in this state, we should probably still show the domain list. We probably want the user to be able to remove the domain from the project, a disabled list won't allow that. The create button should be disabled either way though.
There was a problem hiding this comment.
That seems like it should be a validation error on adding a subproject, we shouldn't allow a subproject to be added to a superproject if it has a custom domain. I doubt there would be a lot of projects in this edge case though.
We already do this validation. We used to block the button in the frontend as well, but this was lost with the migration. We shouldn't allow that action if we know isn't allowed. There are several ways to end up in this state anyway, like when an existing project is converted to be a subproject.
There was a problem hiding this comment.
like when an existing project is converted to be a subproject.
I'm confused. If the subproject form throws a validation error when the target project has a custom domain, how is it possible to convert a project to a subproject when it has a custom domain?
Either way, my point above is still the same. If there is a custom domain, we shouldn't show a disabled list. The button should be disabled but the list should be usable.
There was a problem hiding this comment.
I'm confused. If the subproject form throws a validation error when the target project has a custom domain
There is an error when adding a subdomain to a project, not when converting a project to subproject.
Either way, my point above is still the same. If there is a custom domain, we shouldn't show a disabled list. The button should be disabled but the list should be usable.
I agree with that, and that's what I'm suggesting. But that behavior isn't implemented in our listings right now, that's why I opened the other issue.
There was a problem hiding this comment.
There is an error when adding a subdomain to a project, not when converting a project to subproject.
Okay so yeah, that sounds like what I described then. The project subproject form should throw a validation error when trying to add a subproject that has a domain already.
This is separate by the way, it doesn't help projects already in this state obviously, it only stops new instances of this.
But that behavior isn't implemented in our listings right now,
Other views do this already, you have two options:
- Conditionally override
create_buttonblock with no content to hide the button. Other views do this already. - Conditionally override
create_buttonblock to callinclude create_button with disabled=Trueto show the button in a disabled state.
|
It would be good to move forward with this PR and merge it. What are the next steps required here? |
|
Addressing my feedback about not disabling the section. |
This action isn't allowed at the form level, it gives a generic error, as it was expecting the UI to block this action, so users won't get this error.
I also noticed that we are blocking the whole list, but we only want to block the "add" action, as users could have had domains already created, we still want users to be able to delete those.
closes #666