[PATCH] Add help text to project and patch_project fields
Daniel Axtens
dja at axtens.net
Mon Oct 1 22:25:06 AEST 2018
Stephen Finucane <stephen at that.guru> writes:
> On Mon, 2018-10-01 at 05:37 -0400, Veronika Kabatova wrote:
>>
>> ----- Original Message -----
>> > From: "Stephen Finucane" <stephen at that.guru>
>> > To: "Daniel Axtens" <dja at axtens.net>, vkabatov at redhat.com, patchwork at lists.ozlabs.org
>> > Sent: Sunday, September 30, 2018 12:23:13 AM
>> > Subject: Re: [PATCH] Add help text to project and patch_project fields
>> >
>> > On Sat, 2018-09-29 at 01:24 +1000, Daniel Axtens wrote:
>> > > vkabatov at redhat.com writes:
>> > >
>> > > > From: Veronika Kabatova <vkabatov at redhat.com>
>> > > >
>> > > > Duplication of the field to avoid performance issues caused some
>> > > > unexpected results when moving patches between projects in the admin
>> > > > interface. It's easy to change the "project" field and click save,
>> > > > instead of double checking which field needs to be modified and kept in
>> > > > sync with the one being changed. This lead to patch showing correct
>> > > > project in the API and patch detail in the web UI, but the patch itself
>> > > > was listed in the wrong project when looking at the patch listings.
>> > > >
>> > > > Because of the discussions of the denormalization of the tables, trying
>> > > > to code automatic modification of the other field if one is being
>> > > > changed seems like too much work for very little benefit. What we can do
>> > > > instead is mention this dependency in fields' help text. Modification of
>> > > > the project is not an everyday task so it shouldn't put too much burden
>> > > > on the users and this simple reminder can prevent unexpected breakage
>> > > > and bug reports.
>> > > >
>> > >
>> > > Acked-by: Daniel Axtens <dja at axtens.net>
>> > > > Signed-off-by: Veronika Kabatova <vkabatov at redhat.com>
>> > >
>> > > Stephen, I think this should go to stable/2.1. Thoughts?
>> >
>> > I don't think it can. I haven't checked yet but I'm pretty sure this
>> > requires a migration and we can't backport those.
>> >
>> > Rather than doing this, could we not just override the 'save' function
>> > to always save the other field?
>> >
>>
>> We'd need to figure out which one of the fields is the changed one and
>> modify the other one to match it, which based on [1] doesn't look as easy
>> as it sounds. "I'll scroll till I find the field I want" and "here's the
>> long content, I'll go to the end and scroll up" would lead to the same but
>> opposite issue instead of fixing the problem, if we decided to hardcode one
>> of the fields as the one to be copied over.
>>
>> Because of the above, I opted for the simplest help text solution. I'm not
>> opposed to implement a more complicated solution that doesn't require any
>> steps from the user (it was the first solution I looked into) if you think
>> the complications are worth it, or have a better idea how to work around it.
>
> Ah, good point. I'd like to explore this before we go down any other
> route though. How about this: in the save functions for Submission and
> Patch we search for 'updated_from_child' and 'updated_from_parent'
> kwargs. If these are present, we only update ourselves (to avoid
> recursively calling each other). If they're not present, we update the
> other table (calling 'save' with the paramter) and then ourselves. That
> make sense?
>
> Stephen
>
> PS: I'm still hacking on the tags feature and trying to get performance
> somewhere we'd like it. As things stand, it's looking increasingly
> likely that we're going to need to move a single table inheritance
> model for Patch/CoverLetter but I'm exploring alternatives too (DB
> stuff also isn't my strength). Rest assured though, I consider that a
> blocker for v2.2 so it won't slip forever :)
Cool. What did you mean by "single table inheritance model"? My desired
end state is that we do away with the Submission model and just have a
patch table and a cover letter table - I think this is what you were
describing but I'm not sure. I keep meaning to have a crack at this in
my totally-non-existent spare time but if you are already working on it
there are always lots of other things I can be hacking on in Patchwork.
Regards,
Daniel
>
>>
>> Veronika
>>
>>
>> [1] https://stackoverflow.com/questions/1355150/django-when-saving-how-can-you-check-if-a-field-has-changed
>>
>> > Stephen
>> >
>> > > Regards,
>> > > Daniel
>> > >
>> > > > ---
>> > > > patchwork/models.py | 12 ++++++++++--
>> > > > 1 file changed, 10 insertions(+), 2 deletions(-)
>> > > >
>> > > > diff --git a/patchwork/models.py b/patchwork/models.py
>> > > > index a043844d..e30a9739 100644
>> > > > --- a/patchwork/models.py
>> > > > +++ b/patchwork/models.py
>> > > > @@ -350,7 +350,11 @@ class FilenameMixin(object):
>> > > > class Submission(FilenameMixin, EmailMixin, models.Model):
>> > > > # parent
>> > > >
>> > > > - project = models.ForeignKey(Project, on_delete=models.CASCADE)
>> > > > + project = models.ForeignKey(Project, on_delete=models.CASCADE,
>> > > > + help_text='If modifying this field on
>> > > > Patch, '
>> > > > + 'don\'t forget to modify the "Patch
>> > > > project" '
>> > > > + 'field appropriately as well to ensure
>> > > > proper '
>> > > > + 'functionality!')
>> > > >
>> > > > # submission metadata
>> > > >
>> > > > @@ -405,7 +409,11 @@ class Patch(Submission):
>> > > >
>> > > > # duplicate project from submission in subclass so we can count the
>> > > > # patches in a project without needing to do a JOIN.
>> > > > - patch_project = models.ForeignKey(Project, on_delete=models.CASCADE)
>> > > > + patch_project = models.ForeignKey(Project, on_delete=models.CASCADE,
>> > > > + help_text='"Project" field needs
>> > > > to be '
>> > > > + 'kept in sync and changed manually
>> > > > in '
>> > > > + 'case of modifications to ensure
>> > > > proper '
>> > > > + 'functionality!')
>> > > >
>> > > > objects = PatchManager()
>> > > >
>> > > > --
>> > > > 2.17.1
>> > > >
>> > > > _______________________________________________
>> > > > Patchwork mailing list
>> > > > Patchwork at lists.ozlabs.org
>> > > > https://lists.ozlabs.org/listinfo/patchwork
>> > >
>> > > _______________________________________________
>> > > Patchwork mailing list
>> > > Patchwork at lists.ozlabs.org
>> > > https://lists.ozlabs.org/listinfo/patchwork
>> >
>> >
>> >
More information about the Patchwork
mailing list