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 series list view #575

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

miguelsaura
Copy link
Contributor

Add a series list view - as opposed to the default view which shows patches, this new view shows a list of all series. This is a much easier way to navigate and manage large repositories with 100+ patches (Some series alone contain more than 20-30 patches in a single series). This view will allow simple management of patch series. This version is MVP - and contains the basic view needed to navigate around.

Closes #509

Add a series list view - as opposed to the default view which shows
patches, this new view shows a list of all series. This is
a much easier way to navigate and manage large repositories with 100+
patches (Some series alone contain more than 20-30 patches in a single
series). This view will allow simple management of patch series.
This version is MVP - and contains the basic view needed to navigate
around.
@rossburton
Copy link
Contributor

As a user who missed the series view that presumably was in our ancient/horrible fork, this is great.

Can you make the list show the number of patches in the series somewhere?

@miguelsaura
Copy link
Contributor Author

As a user who missed the series view that presumably was in our ancient/horrible fork, this is great.

Can you make the list show the number of patches in the series somewhere?

Thanks, yes I'll work on it

@stephenfin
Copy link
Member

I've held of doing this because I wanted to add support for boolean Series states first, i.e. a series is either open/closed depending on the status of the state field of each associated Patch. I think this is still the priority, because without this the series page grows without end. This is also an issues with the /series APIs, fwiw.

I wonder if you'd be interested in working on that first? I suspect we can derive the value of an e.g. Series.is_open or Series.resolved field by inspecting the value of Patch.series.action_required. If action_required is False, the patch can be consider "closed". If all patches are "closed", then Series.is_open can be set to False. This can be done both in an initial migration and at runtime whenever we change the state of a patch (via the save method).

There are some other minor issues with the patch, which I'll leave comments for shortly, but I'd rather focus on closing the above first before we consider merging this.

</td>
</tr>
</table>
{% endif %}
Copy link
Member

Choose a reason for hiding this comment

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

This code is for bundles and doesn't do anything here. It can be removed.

e.preventDefault();
});
});
</script>
Copy link
Member

Choose a reason for hiding this comment

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

Are we doing anything with the checkboxes? Given we don't have any widgets for updating a series, I'd assume not? If so, do we need this?

@@ -0,0 +1,154 @@
{% load person %}
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't need to be a partial. The only reason we have a patch-list partial is because we re-use that partial for both the patch list view and the bundle view (bundles are just lists of patches). You can combine this into series.html.

@@ -36,6 +36,11 @@
patch_views.patch_list,
name='patch-list',
),
path(
'project/<project_id>/series-list/',
Copy link
Member

Choose a reason for hiding this comment

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

We should align this with the URL structure of the rest of the URLs. For example, project/<project_id>/series/.

We'll also eventually need a series detail view (and a new-style mbox and patch view), but it's okay to tackle that separately.

@@ -273,48 +275,65 @@ def generic_list(
else:
context['filters'].set_status(filterclass, setting)

if patches is None:
patches = Patch.objects.filter(project=project)
if series_view:
Copy link
Member

Choose a reason for hiding this comment

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

Given we're not modifying anything here, I don't think there's any reason to re-use this function. It's already way too complicated for it's own good. Better to split this out into a new view function, IMO.

)

if request.user.is_authenticated:
context['bundles'] = request.user.bundles.all()
Copy link
Member

Choose a reason for hiding this comment

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

This isn't used anywhere so it doesn't need to be here.

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.

Series view
3 participants