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

remove /projects/ #6228

Merged
merged 2 commits into from Oct 7, 2019
Merged

remove /projects/ #6228

merged 2 commits into from Oct 7, 2019

Conversation

@iambenzo
Copy link
Contributor

@iambenzo iambenzo commented Oct 1, 2019

Fixes #6221

I am relatively new to development with Django but to remove the publicly accessible /projects path, only a small amount of removals seemed to be required.

I did not remove the ProjectIndex class, as it seems that the tags functionality is dependant on it. If you're happy for the tags to go, I will be happy to update my PR.

I did not remove the project_urls section, as clicking on a project as a logged in user takes you to a /projects/<project_name> route.

Please let me know how I did in meeting your expectations and let me know if I can do any better.

@stsewd
Copy link
Member

@stsewd stsewd commented Oct 1, 2019

Thanks for the pull request!

I did not remove the ProjectIndex class, as it seems that the tags functionality is dependant on it. If you're happy for the tags to go, I will be happy to update my PR.

I'd say that section can go too.

Looks like it links from https://readthedocs.org/projects/docs/ (the tags section) https://readthedocs.org/projects/docs/

Maybe @davidfischer has another opinion here

But if the section stays, you shouldn't remove the template, as that view still uses it.

@stsewd stsewd closed this Oct 1, 2019
@stsewd
Copy link
Member

@stsewd stsewd commented Oct 1, 2019

Sorry, wrong button

@stsewd stsewd reopened this Oct 1, 2019
@iambenzo
Copy link
Contributor Author

@iambenzo iambenzo commented Oct 1, 2019

Okay, thanks - I'll wait for input from @davidfischer before I make any further updates.

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Oct 1, 2019

Maybe @davidfischer has another opinion here

At least right now I don't think we should remove browsing projects by tag (eg. https://readthedocs.org/projects/tags/python/). Compared with listing all projects, at least tags have a use which is finding similar projects.

I also wouldn't mind if /projects/ redirected to /dashboard/ but that can come in a separate PR.

@iambenzo
Copy link
Contributor Author

@iambenzo iambenzo commented Oct 1, 2019

Okay, thanks. In that case, I think my course of action is to reinstate the template I removed and all should be well.

@stsewd
Copy link
Member

@stsewd stsewd commented Oct 2, 2019

@iambenzo can you please merge master into this branch? Test on travis have been fixed

@iambenzo
Copy link
Contributor Author

@iambenzo iambenzo commented Oct 3, 2019

Sure thing.

@iambenzo iambenzo force-pushed the remove-projects-page branch from f172382 to 8ce612e Oct 3, 2019
@stsewd
Copy link
Member

@stsewd stsewd commented Oct 3, 2019

We can now remove this conditional, as we always expect a tag now, and probably rename the class

if self.kwargs.get('tag'):

@iambenzo
Copy link
Contributor Author

@iambenzo iambenzo commented Oct 4, 2019

Okie dokie - I'll get on that a little later

stsewd
stsewd approved these changes Oct 7, 2019
Copy link
Member

@stsewd stsewd left a comment

Thanks!

@stsewd stsewd requested a review from davidfischer Oct 7, 2019
Copy link
Contributor

@davidfischer davidfischer left a comment

This looks good to me. I noted a few things that separately have to happen but nothing that should stop merging.

We should definitely redirect /projects/ -> /dashboard/ but as I mentioned, that can come in a separate PR.

@@ -37,7 +37,7 @@
mimetypes.add_type('application/epub+zip', '.epub')


class ProjectIndex(ListView):
class ProjectTagIndex(ListView):
Copy link
Contributor

@davidfischer davidfischer Oct 7, 2019

Choose a reason for hiding this comment

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

I'm just noting that this change will require a corresponding change to the private Read the Docs corporate repository.

else:
self.tag = None
self.tag = get_object_or_404(Tag, slug=self.kwargs.get('tag'))
queryset = queryset.filter(tags__slug__in=[self.tag.slug])

if self.kwargs.get('username'):
Copy link
Contributor

@davidfischer davidfischer Oct 7, 2019

Choose a reason for hiding this comment

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

It doesn't seem like this is used anymore. As a separate change in a separate PR, this could probably be removed.

Copy link
Member

@stsewd stsewd Oct 7, 2019

Choose a reason for hiding this comment

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

@stsewd stsewd merged commit 0c69e7a into readthedocs:master Oct 7, 2019
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants