Skip to content

Don't allow adding domains on subprojects #663

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

Open
stsewd wants to merge 1 commit into
base: main
Choose a base branch
from
+18 −2
Open

Don't allow adding domains on subprojects #663

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions readthedocsext/theme/templates/projects/domain_list.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{% extends "projects/project_edit_base.html" %}

{% load trans from i18n %}
{% load trans blocktrans from i18n %}

{% block title %}
{{ project.name }} - {% trans "Domains" %}
Expand All @@ -23,8 +23,24 @@
{% endcomment %}
{% if not enabled %}
{% include "organizations/includes/feature_disabled.html" with project=project plan="Advanced" %}
{% elif project.superproject %}
<div class="ui icon message">
<i class="fa-duotone fa-circle-exclamation icon"></i>
<div class="content">
<div class="header">{% trans "Custom domains managed by superproject" %}</div>
<p>
{% url "projects_detail" project.superproject.slug as superproject_url %}
{% url "projects_domains" project.superproject.slug as superproject_domains_url %}
{% blocktrans trimmed with superproject_url=superproject_url superproject_domains_url=superproject_domains_url superproject_name=project.superproject.name %}
This project is a subproject of
<a href="{{ superproject_url }}">{{ superproject_name }}</a>.
Custom domains must be <a href="{{ superproject_domains_url }}">added on the superproject</a>.
{% endblocktrans %}
</p>
</div>
</div>
{% 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 %}
Copy link
Contributor

@agjohnson agjohnson Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ext-theme/readthedocsext/theme/templates/projects/projectrelationship_list.html

Lines 10 to 36 in 1831b18

{% block project_edit_content %}
{% if superproject %}
<div class="ui icon message">
<i class="fa-duotone fa-circle-exclamation icon"></i>
<div class="content">
<div class="header">
{% trans "Nested subprojects are not supported" %}
</div>
<p>
{% blocktrans trimmed with project=superproject.name %}
This project is already configured as a subproject of {{ project }}.
{% endblocktrans %}
</p>
<p>
<a href="{% url 'projects_subprojects' project_slug=superproject.slug %}">
{% blocktrans trimmed with project=superproject.name %}
View all subprojects of {{ project }}
{% endblocktrans %}
</a>
</p>
</div>
</div>
{% else %}
{% include "projects/partials/edit/subproject_list.html" with objects=object_list %}
{% endif %}
{% endblock %}

Copy link
Member Author

@stsewd stsewd Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@agjohnson agjohnson Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@stsewd stsewd Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@agjohnson agjohnson Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@stsewd stsewd Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@agjohnson agjohnson Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_button block with no content to hide the button. Other views do this already.
  • Conditionally override create_button block to call include create_button with disabled=True to show the button in a disabled state.

</div>
{% endblock project_edit_content %}
Expand Down