[PATCH] Add help text to project and patch_project fields
Veronika Kabatova
vkabatov at redhat.com
Tue Oct 2 02:54:56 AEST 2018
----- Original Message -----
> From: "Stephen Finucane" <stephen at that.guru>
> To: "Veronika Kabatova" <vkabatov at redhat.com>
> Cc: "Daniel Axtens" <dja at axtens.net>, patchwork at lists.ozlabs.org
> Sent: Monday, October 1, 2018 11:57:09 AM
> Subject: Re: [PATCH] Add help text to project and patch_project fields
>
> 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?
>
I might be misunderstanding this. If you edit a patch via "patches" admin,
patch's save method is always called. So it doesn't matter which field was
actually changed, the "project" one would be consistently overwritten by
"patch_project" based on this logic.
Can you please clarify if I'm missing something obvious?
Thanks,
Veronika
> 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 :)
>
> >
> > 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