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

Add project/build filters #8142

Merged
merged 4 commits into from Jun 11, 2021
Merged

Add project/build filters #8142

merged 4 commits into from Jun 11, 2021

Conversation

agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Apr 28, 2021

Most of these filters are currently only used in views when the new
templates are enabled. The project dashboard view context data was
replaced with a filter, which does enable sorting on the dashboard view
using request arguments (like ?sort=built_last), but this isn't
exposed in the UI.

@agjohnson agjohnson marked this pull request as ready for review April 28, 2021 21:42
@agjohnson agjohnson requested a review from a team April 28, 2021 21:42
@agjohnson
Copy link
Contributor Author

I found a missing commit that was supposed to be part of this branch. Going back to WIP.

@agjohnson agjohnson added the PR: work in progress Pull request is not ready for full review label Apr 30, 2021
Most of these filters are currently only used in views when the new
templates are enabled. The project dashboard view context data was
replaced with a filter, which does enable sorting on the dashboard view
using request arguments (like `?sort=built_last`), but this isn't
exposed in the UI.
@agjohnson agjohnson force-pushed the agj/add-project-build-filters branch from f420412 to 57d6fbb Compare May 26, 2021 23:34
@agjohnson agjohnson removed the PR: work in progress Pull request is not ready for full review label May 26, 2021
@agjohnson agjohnson requested review from a team and humitos and removed request for a team May 26, 2021 23:35
@agjohnson
Copy link
Contributor Author

Ready for review!

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.

The code looks good to me. However, I haven't QAed this yet because it seems it's not ready from the UI. This is not used yet, so I think it's fine to merge and leave it ready for the new templates to make usage of it.

readthedocs/projects/filters.py Outdated Show resolved Hide resolved
readthedocs/projects/filters.py Show resolved Hide resolved
readthedocs/projects/views/private.py Outdated Show resolved Hide resolved
@agjohnson
Copy link
Contributor Author

I fixed some goofs, but that probably means I should check the application again with the new templates because I'm sure I've changed something incorrectly. I will test this today.

@agjohnson agjohnson added the Status: blocked Issue is blocked on another issue label May 27, 2021
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This looks like a good start. I think we should just always use the filter, and that will let us figure out if there's performance issues with it.

Seems like this also needs some tests. I'm a little worried about performance here, so it might be good to make sure the queries we're generating aren't going to kill the DB, especially around the Build & Version tables.

readthedocs/builds/filters.py Show resolved Hide resolved
readthedocs/builds/views.py Show resolved Hide resolved
readthedocs/projects/filters.py Outdated Show resolved Hide resolved
readthedocs/projects/filters.py Show resolved Hide resolved
@agjohnson
Copy link
Contributor Author

agjohnson commented Jun 4, 2021

Seems like this also needs some tests.

Perhaps. I'm probably not the best person to write these tests however. It would probably be better if someone else wants to dive into test and/or we revisit this later. My major concern is continuing to block this work, it's holding up my progress on templates.

In general, I'd like to find a way to hand off application work so that I'm just working on frontend pieces with the templates. This piece in particular was rather annoying to develop however, and took back and forth between front end and backend changes to tweak this to make the UI correct.

There is maybe a good amount of value in having someone else jump in to this work to expand test coverage afterwards. It's going to take me a lot longer to write tests and having a second set of hands on the code after would be a benefit to QA.

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

I'm fine merging and adding a TODO somewhere to test this code, as long as we actually get back to it :)

I'd love to have you working w/ @humitos as a small team, with him doing backend & API changes needed. I'd generally like to see us doing this more, but we all have close to the same skillsets which makes it hard to find space for it.

@agjohnson
Copy link
Contributor Author

Yup, that's the workflow I was describing.

I've already pushed off API changes, which are easier to collaborate on, though differences in priority slow progress a good deal.

Changes like this PR are hard, and require experimentation on multiple repositories to nail down the UI. This process would have to be more of a pairing process, as there was experimentation on multiple repositories to get this UI correct. It would have been an awful amount of back and forth to collaborate on this work. This is where I'm looking for process improvements. We will want to figure out how to split up backend and frontend development into digestible work.

@agjohnson
Copy link
Contributor Author

Moved test coverage to #8238

@agjohnson
Copy link
Contributor Author

I have tested these changes and they look correct. Build/version/project filters are all working on new templates. I squeezed a small fix for unpacking the dictionary, I didn't get a chance to test the previous commit because my dev environment was broken again. Looks good now however.

@agjohnson agjohnson merged commit 0e89c2e into master Jun 11, 2021
@agjohnson agjohnson deleted the agj/add-project-build-filters branch June 11, 2021 01:37
@humitos humitos removed the Status: blocked Issue is blocked on another issue label Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants