Skip to content
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

Project: merge basic and advanced forms #11169

Merged
merged 9 commits into from
Feb 29, 2024
Merged

Project: merge basic and advanced forms #11169

merged 9 commits into from
Feb 29, 2024

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Feb 28, 2024

  • ProjectExtraForm wasn't being used by itself, it was just inherited from one form, so I merged it with that one instead.
  • ProjectAdvancedForm was merged into UpdateProjectForm.
  • Sorted the settings by trying to group them in related groups
  • The old advanced settings page now redirects to the basic/normal settings page
  • References in UI to the old settings page were deleted (the screenshot was done before I realized about it)
  • Old links were changed
  • Documentation was updated

I'm still missing checking how this looks on ext-theme.

Screenshot 2024-02-28 at 16-35-16 Edit Project Read the Docs

Closes #11084

@stsewd stsewd marked this pull request as ready for review February 28, 2024 21:55
@stsewd stsewd requested review from a team as code owners February 28, 2024 21:55
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

Looks great to me! 💯

I would use a 302 redirect instead of a 301 just in case.

docs/user/guides/pull-requests.rst Outdated Show resolved Hide resolved
@@ -84,7 +83,9 @@
),
re_path(
r"^(?P<project_slug>[-\w]+)/advanced/$",
ProjectAdvancedUpdate.as_view(),
login_required(
RedirectView.as_view(pattern_name="projects_edit", permanent=True),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
RedirectView.as_view(pattern_name="projects_edit", permanent=True),
RedirectView.as_view(pattern_name="projects_edit"),

Just in case we want to use /advanced/ URL for a different thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thoughts @agjohnson?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question. I lean slightly towards non-permanent just because it won't be a heavily used redirect anyways. I don't see us reusing the URL either though, so permanent does seem safe.

@@ -251,6 +251,7 @@ class PublicProjectMixin(ProjectMixin):
"/projects/pip/badge/": {"status_code": 200},
"/projects/invalid_slug/": {"status_code": 302},
"/projects/pip/search/": {"status_code": 302},
"/dashboard/pip/advanced/": {"status_code": 301},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"/dashboard/pip/advanced/": {"status_code": 301},
"/dashboard/pip/advanced/": {"status_code": 302},
stsewd added a commit to readthedocs/ext-theme that referenced this pull request Feb 29, 2024
@stsewd stsewd merged commit 1517e12 into main Feb 29, 2024
7 checks passed
@stsewd stsewd deleted the merge-project-forms branch February 29, 2024 21:00
stsewd added a commit to readthedocs/ext-theme that referenced this pull request Feb 29, 2024
* Project: merge basic and advanced forms

Matches readthedocs/readthedocs.org#11169

* Update URL
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants