Message ID | 1301950805.2613.68.camel@feioso |
---|---|
State | RFC |
Headers | show |
On Mon, 2011-04-04 at 18:00 -0300, Guilherme Salgado wrote: > Hi there, > > I've been working on a new patch-list page which includes all patches > submitted by the logged in user (in fact by any Person linked to the > logged in user) that are still waiting for feedback > (state.action_required==True). > > The reason I've worked on this is because in Linaro we'd benefit from > having a single place where users can see all their patches that haven't > gotten feedback yet, without having to go to the patch-list of every > project they might have contributed and filtering the patches there by > submitter. I think other users might find this useful as well, so here > it is. > > Notice that this is still a work in progress and, AFAIK, this is the > first cross-project patch list, but it uses the same underlying > infrastructure used by other patch lists, so some things there may not > work (cross-project bundling, for instance). Also, the initial state of > the filters does not reflect reality and this seems tricky to do as you > can't use the UI to filter on multiple submitters, but I don't think > this is that big a deal. I've done some more testing and these are some things that will be tricky to have working: - Restrict the values in the Delegate form field to the project maintainers -- tricky as the patches are not from a single project - Bundle patches -- again, tricky as there are patches from multiple projects and the DB schema doesn't permit that - At least the Submitter and State filters don't make much sense as the purpose of the list is to show patches on a certain state submitted by a certain user. I suppose this is similar to the behavior of the State filter on the todo list? Given the above, I'm kind of leaning towards not using the existing generic_list() helper and omitting the filters, the form to bundle patches and the Delegate field from the Properties form. This would make sure we don't mislead users into doing things that won't actually work as they expect, but it also leaves the ability to change the state and/or archive patches, which I think is quite useful.
Hi Guilherme, > Given the above, I'm kind of leaning towards not using the existing > generic_list() helper and omitting the filters, the form to bundle > patches and the Delegate field from the Properties form. This would make > sure we don't mislead users into doing things that won't actually work > as they expect, but it also leaves the ability to change the state > and/or archive patches, which I think is quite useful. I've wanted to add a 'my patches' view previously, so thanks for taking a look at this. However, the multiple-projects-on-one-list part does make this a bit difficult. Would separating the patches by project still work for you? I'd imagined implementing this by: * Adding a "my patches for $project" view, which should just be a matter of configuring the filters correctly. Other than the submitter filter, the other filters would have the usual default settings, and be changeable via the UI. This means that we get the "action required" patches by default, but the user can still get the full list easily. * Changing the 'contributor to...' text at the top of the userprofile view to something (a table?) listing the numbers of patches in an action required state. Project names would be links to the "my patches for $project". This gives a quick overview of patches in all projects. The downside of this is that if a user is participating in multiple projects, they need to visit multiple pages if they want to see the list of all patches. However, I think this is okay, for a few reasons: * Although users may be contributing to multiple projects, it's likely that only one or two would be their main focus of attention at any one time. * Cross-posting a patch to multiple (patchwork-enabled) projects would results in multiple entries on the 'my patches' page, only one being relevant. Cross-posting a series of patches will make this unusable, unless we can then filter by project. * With the current patchwork installations, the projects on each server (patchwork.ozlabs.org/patchwork.kernel.org/others) may not have any specific grouping; this puts the 'my patches' into fairly arbitrary groups. Any thoughts? Jeremy
On Thu, 2011-04-07 at 09:23 +0800, Jeremy Kerr wrote: > Hi Guilherme, > > > Given the above, I'm kind of leaning towards not using the existing > > generic_list() helper and omitting the filters, the form to bundle > > patches and the Delegate field from the Properties form. This would make > > sure we don't mislead users into doing things that won't actually work > > as they expect, but it also leaves the ability to change the state > > and/or archive patches, which I think is quite useful. > > I've wanted to add a 'my patches' view previously, so thanks for taking > a look at this. However, the multiple-projects-on-one-list part does > make this a bit difficult. > > Would separating the patches by project still work for you? I think it would not be ideal for our use case, but it is probably good enough and I'd be prefer to work on something that has a chance of being accepted, so I'm willing to give it a try. > > I'd imagined implementing this by: > > * Adding a "my patches for $project" view, which should just be a > matter of configuring the filters correctly. Other than the > submitter filter, the other filters would have the usual default > settings, and be changeable via the UI. This means that we get > the "action required" patches by default, but the user can still > get the full list easily. AFAICT, the submitter filter will match against the person name or id, which is not going to match all my patches if the names of all Person entries associated with my user are not identical. We could try and make the filter match against either a person or a user, but I think a UI for that would be confusing. Or we could make it match against the user linked to the selected person when there's one, but this may not be what everybody expects (e.g. they may want the ability to search patches submitted using just one of their email addresses). > > * Changing the 'contributor to...' text at the top of the > userprofile view to something (a table?) listing the numbers of > patches in an action required state. Project names would be > links to the "my patches for $project". This gives a quick > overview of patches in all projects. Indeed, that'd be an important thing if we have separate lists per project. > > The downside of this is that if a user is participating in multiple > projects, they need to visit multiple pages if they want to see the list Exactly; that's the main reason why I went with a single cross-project list. All patches submitted upstream by Linaro engineers will end up in our patchwork instance, so there's a big chance that most people will have patches in a few different projects. It also means users have just one URL to bookmark or type instead of having to go to the profile page and follow links from there. > of all patches. However, I think this is okay, for a few reasons: > > * Although users may be contributing to multiple projects, it's > likely that only one or two would be their main focus of > attention at any one time. In our case there are a bunch of cases where there are more than one or two but as long as it's less than a handful (which seems to be the case for everyone so far) it should be fine. > > * Cross-posting a patch to multiple (patchwork-enabled) projects > would results in multiple entries on the 'my patches' page, only > one being relevant. Cross-posting a series of patches will make > this unusable, unless we can then filter by project. I don't think this is much worse than when you have one list per project -- you'll still see them all, just on multiple lists instead of on a single one. > > * With the current patchwork installations, the projects on each > server (patchwork.ozlabs.org/patchwork.kernel.org/others) may > not have any specific grouping; this puts the 'my patches' into > fairly arbitrary groups. In practice the only grouping is for the projects you contribute to, which seems to be a reasonable thing to me. If you don't contribute to most of the projects in a given patchwork instance, then you'd see no trace of them on your 'my patches' view. Another downside of this approach is that it makes it harder to find a given patch by skimming through the list. Supposing you know on which project the patch is, you can go straight there and there will probably be less patches for you to skim through.
On Thu, 2011-04-07 at 09:23 +0800, Jeremy Kerr wrote: > Hi Guilherme, > > > Given the above, I'm kind of leaning towards not using the existing > > generic_list() helper and omitting the filters, the form to bundle > > patches and the Delegate field from the Properties form. This would make > > sure we don't mislead users into doing things that won't actually work > > as they expect, but it also leaves the ability to change the state > > and/or archive patches, which I think is quite useful. > > I've wanted to add a 'my patches' view previously, so thanks for taking > a look at this. However, the multiple-projects-on-one-list part does > make this a bit difficult. > > Would separating the patches by project still work for you? > > I'd imagined implementing this by: > > * Adding a "my patches for $project" view, which should just be a > matter of configuring the filters correctly. Other than the > submitter filter, the other filters would have the usual default > settings, and be changeable via the UI. This means that we get > the "action required" patches by default, but the user can still > get the full list easily. > > * Changing the 'contributor to...' text at the top of the > userprofile view to something (a table?) listing the numbers of > patches in an action required state. Project names would be > links to the "my patches for $project". This gives a quick > overview of patches in all projects. Another thing I've just noticed we'll have to keep in mind is that in these per-project lists, users won't be able to do mass-state-changes unless they're the maintainers of the project in question. We could change generic_list() to behave differently and always include the state-changing form, but this seems to me like yet another indication that generic_list() may not be the appropriate thing to use here.
Hi Guilherme, > Another thing I've just noticed we'll have to keep in mind is that in > these per-project lists, users won't be able to do mass-state-changes > unless they're the maintainers of the project in question. We could > change generic_list() to behave differently and always include the > state-changing form, but this seems to me like yet another indication > that generic_list() may not be the appropriate thing to use here. That would indicate to me that we should update generic_list to handle these cases properly. What we could do: if there are editable patches in the list, then include the checkbox-per-patch column (with checkboxed disabled for non-editable patches), and include the state-changing form. Otherwise, just leave it as a read-only view. Cheers, Jeremy
On Tue, 2011-04-12 at 09:53 +0800, Jeremy Kerr wrote: > Hi Guilherme, > > > Another thing I've just noticed we'll have to keep in mind is that in > > these per-project lists, users won't be able to do mass-state-changes > > unless they're the maintainers of the project in question. We could > > change generic_list() to behave differently and always include the > > state-changing form, but this seems to me like yet another indication > > that generic_list() may not be the appropriate thing to use here. > > That would indicate to me that we should update generic_list to handle > these cases properly. > > What we could do: if there are editable patches in the list, then > include the checkbox-per-patch column (with checkboxed disabled for > non-editable patches), and include the state-changing form. Otherwise, > just leave it as a read-only view. I was going to give this a try but then I realized that the checkboxes are also used by the bundle form, which doesn't require patch edit rights. IOW, doing this change means people would lose the ability to bundle patches on which they don't have edit rights, which I think is not desirable, right?
Hi Guilherme, > I was going to give this a try but then I realized that the checkboxes > are also used by the bundle form, which doesn't require patch edit > rights. IOW, doing this change means people would lose the ability to > bundle patches on which they don't have edit rights, which I think is > not desirable, right? Yes, you're exactly right; we need to keep the checkboxes there in all cases. Are you still planning to send a patch to add per-project "my patches" lists? Cheers, Jeremy
On Thu, 2011-04-14 at 14:32 +0800, Jeremy Kerr wrote: > Hi Guilherme, > > > I was going to give this a try but then I realized that the checkboxes > > are also used by the bundle form, which doesn't require patch edit > > rights. IOW, doing this change means people would lose the ability to > > bundle patches on which they don't have edit rights, which I think is > > not desirable, right? > > Yes, you're exactly right; we need to keep the checkboxes there in all > cases. > > Are you still planning to send a patch to add per-project "my patches" > lists? Yep, I've sent a couple patches last Friday with the changes we discussed on IRC. Did you have some time to check them? Cheers, Guilherme
diff --git a/apps/patchwork/models.py b/apps/patchwork/models.py index e4df2c5..7e2905d 100644 --- a/apps/patchwork/models.py +++ b/apps/patchwork/models.py @@ -100,6 +100,11 @@ class UserProfile(models.Model): def n_todo_patches(self): return self.todo_patches().count() + def submitted_patches_waiting_feedback(self): + people = Person.objects.filter(user=self) + states = State.objects.filter(action_required=True) + return Patch.objects.filter(submitter__in=people, state__in=states) + def todo_patches(self, project = None): # filter on project, if necessary diff --git a/apps/patchwork/tests/models.py b/apps/patchwork/tests/models.py new file mode 100644 index 0000000..99d9d20 --- /dev/null +++ b/apps/patchwork/tests/models.py @@ -0,0 +1,37 @@ + +from django.test import TestCase + +from patchwork.models import State +from patchwork.tests.factory import ObjectFactory + + +class UserProfileTestCase(TestCase): + + def setUp(self): + super(UserProfileTestCase, self).setUp() + self.factory = ObjectFactory() + + def test_submitted_patches_waiting_feedback(self): + # Create two people linked to the same user. + person = self.factory.makePerson(is_user=True) + profile = person.user.get_profile() + person2 = self.factory.makePerson(is_user=False) + person2.user = person.user + person2.save() + + # Create 4 patches, where the first three have a person linked to our + # newly created user as the submitter but only the first two ones are + # in a state that needs action. + patch1 = self.factory.makePatch(submitter=person) + patch2 = self.factory.makePatch(submitter=person2) + patch3 = self.factory.makePatch(submitter=person2) + patch3.state = State.objects.get(name='Accepted') + patch3.save() + patch4 = self.factory.makePatch() + + # Here we see that UserProfile.submitted_patches_waiting_feedback() + # only returns the two patches that are in a state that requires + # action and that have been submitted by a person linked to that + # profile. + self.assertEquals([patch1, patch2], + list(profile.submitted_patches_waiting_feedback())) diff --git a/apps/patchwork/urls.py b/apps/patchwork/urls.py index b49b4e1..b5855da 100644 --- a/apps/patchwork/urls.py +++ b/apps/patchwork/urls.py @@ -33,6 +33,7 @@ urlpatterns = patterns('', # logged-in user stuff (r'^user/$', 'patchwork.views.user.profile'), + (r'^user/submitted/$', 'patchwork.views.user.submitted_patches_list'), (r'^user/todo/$', 'patchwork.views.user.todo_lists'), (r'^user/todo/(?P<project_id>[^/]+)/$', 'patchwork.views.user.todo_list'), diff --git a/apps/patchwork/views/user.py b/apps/patchwork/views/user.py index 1ae3c2d..dd55cf7 100644 --- a/apps/patchwork/views/user.py +++ b/apps/patchwork/views/user.py @@ -109,6 +109,18 @@ def unlink(request, person_id): @login_required +def submitted_patches_list(request): + profile = request.user.get_profile() + patches = profile.submitted_patches_waiting_feedback() + + context = generic_list( + request, None, 'patchwork.views.user.submitted_patches_list', + patches=patches) + + return render_to_response('patchwork/submitted-list.html', context) + + +@login_required def todo_lists(request): todo_lists = [] diff --git a/templates/patchwork/profile.html b/templates/patchwork/profile.html index 44df921..be65211 100644 --- a/templates/patchwork/profile.html +++ b/templates/patchwork/profile.html @@ -36,6 +36,17 @@ Contributor to </div> <div class="box"> + <h2>Patches you submitted</h2> +{% if user.get_profile.submitted_patches_waiting_feedback.count %} + <p>There are <a href="{% url patchwork.views.user.submitted_patches_list %}"> + {{ user.get_profile.submitted_patches_waiting_feedback.count }} patches submitted by you</a> + that haven't been reviewed yet.</p> +{% else %} + <p>There are no patches submitted by you that haven't been reviewed.</p> +{% endif %} +</div> + +<div class="box"> <h2>Linked email addresses</h2> <p>The following email addresses are associated with this patchwork account. Adding alternative addresses allows patchwork to group contributions that diff --git a/templates/patchwork/submitted-list.html b/templates/patchwork/submitted-list.html new file mode 100644 index 0000000..51c8603 --- /dev/null +++ b/templates/patchwork/submitted-list.html @@ -0,0 +1,14 @@ +{% extends "base.html" %} + +{% load person %} + +{% block title %}{{ user }}'s submitted patches{% endblock %} +{% block heading %}{{user}}'s submitted patches{% endblock %} + +{% block body %} + +<p>TODO</p> + +{% include "patchwork/patch-list.html" %} + +{% endblock %}