Add help text to project and patch_project fields

Message ID 20180927101408.25078-1-vkabatov@redhat.com
State New
Headers show
Series
  • Add help text to project and patch_project fields
Related show

Commit Message

Veronika Kabatova Sept. 27, 2018, 10:14 a.m.
From: Veronika Kabatova <vkabatov@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.

Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>
---
 patchwork/models.py | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Daniel Axtens Sept. 28, 2018, 3:24 p.m. | #1
vkabatov@redhat.com writes:

> From: Veronika Kabatova <vkabatov@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@axtens.net>
> Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>

Stephen, I think this should go to stable/2.1. Thoughts?

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@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
Stephen Finucane Sept. 29, 2018, 10:23 p.m. | #2
On Sat, 2018-09-29 at 01:24 +1000, Daniel Axtens wrote:
> vkabatov@redhat.com writes:
> 
> > From: Veronika Kabatova <vkabatov@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@axtens.net>
> > Signed-off-by: Veronika Kabatova <vkabatov@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?

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@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/patchwork
> 
> _______________________________________________
> Patchwork mailing list
> Patchwork@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/patchwork
Veronika Kabatova Oct. 1, 2018, 9:37 a.m. | #3
----- Original Message -----
> From: "Stephen Finucane" <stephen@that.guru>
> To: "Daniel Axtens" <dja@axtens.net>, vkabatov@redhat.com, patchwork@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@redhat.com writes:
> > 
> > > From: Veronika Kabatova <vkabatov@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@axtens.net>
> > > Signed-off-by: Veronika Kabatova <vkabatov@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.


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@lists.ozlabs.org
> > > https://lists.ozlabs.org/listinfo/patchwork
> > 
> > _______________________________________________
> > Patchwork mailing list
> > Patchwork@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/patchwork
> 
> 
>
Stephen Finucane Oct. 1, 2018, 9:57 a.m. | #4
On Mon, 2018-10-01 at 05:37 -0400, Veronika Kabatova wrote:
> 
> ----- Original Message -----
> > From: "Stephen Finucane" <stephen@that.guru>
> > To: "Daniel Axtens" <dja@axtens.net>, vkabatov@redhat.com, patchwork@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@redhat.com writes:
> > > 
> > > > From: Veronika Kabatova <vkabatov@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@axtens.net>
> > > > Signed-off-by: Veronika Kabatova <vkabatov@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 :)

> 
> 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@lists.ozlabs.org
> > > > https://lists.ozlabs.org/listinfo/patchwork
> > > 
> > > _______________________________________________
> > > Patchwork mailing list
> > > Patchwork@lists.ozlabs.org
> > > https://lists.ozlabs.org/listinfo/patchwork
> > 
> > 
> >
Daniel Axtens Oct. 1, 2018, 12:25 p.m. | #5
Stephen Finucane <stephen@that.guru> writes:

> On Mon, 2018-10-01 at 05:37 -0400, Veronika Kabatova wrote:
>> 
>> ----- Original Message -----
>> > From: "Stephen Finucane" <stephen@that.guru>
>> > To: "Daniel Axtens" <dja@axtens.net>, vkabatov@redhat.com, patchwork@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@redhat.com writes:
>> > > 
>> > > > From: Veronika Kabatova <vkabatov@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@axtens.net>
>> > > > Signed-off-by: Veronika Kabatova <vkabatov@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@lists.ozlabs.org
>> > > > https://lists.ozlabs.org/listinfo/patchwork
>> > > 
>> > > _______________________________________________
>> > > Patchwork mailing list
>> > > Patchwork@lists.ozlabs.org
>> > > https://lists.ozlabs.org/listinfo/patchwork
>> > 
>> > 
>> >
Veronika Kabatova Oct. 1, 2018, 4:54 p.m. | #6
----- Original Message -----
> From: "Stephen Finucane" <stephen@that.guru>
> To: "Veronika Kabatova" <vkabatov@redhat.com>
> Cc: "Daniel Axtens" <dja@axtens.net>, patchwork@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@that.guru>
> > > To: "Daniel Axtens" <dja@axtens.net>, vkabatov@redhat.com,
> > > patchwork@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@redhat.com writes:
> > > > 
> > > > > From: Veronika Kabatova <vkabatov@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@axtens.net>
> > > > > Signed-off-by: Veronika Kabatova <vkabatov@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@lists.ozlabs.org
> > > > > https://lists.ozlabs.org/listinfo/patchwork
> > > > 
> > > > _______________________________________________
> > > > Patchwork mailing list
> > > > Patchwork@lists.ozlabs.org
> > > > https://lists.ozlabs.org/listinfo/patchwork
> > > 
> > > 
> > > 
> 
> 
>
Stephen Finucane Oct. 1, 2018, 8:24 p.m. | #7
On Mon, 2018-10-01 at 22:25 +1000, Daniel Axtens wrote:
> Stephen Finucane <stephen@that.guru> writes:
> 
> > On Mon, 2018-10-01 at 05:37 -0400, Veronika Kabatova wrote:
> > > 
> > > ----- Original Message -----
> > > > From: "Stephen Finucane" <stephen@that.guru>
> > > > To: "Daniel Axtens" <dja@axtens.net>, vkabatov@redhat.com, patchwork@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@redhat.com writes:
> > > > > 
> > > > > > From: Veronika Kabatova <vkabatov@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@axtens.net>
> > > > > > Signed-off-by: Veronika Kabatova <vkabatov@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.

No, "single table inheritance" means we'd fold Patch and CoverLetter
models back into Submission and add a 'type' column. This seems
reasonable given that all the Patch-specific fields are nullable. The
only issue I'm finding is making sure a series only has one
CoverLetter. Essentially we would need some kind of ForeignKey
constraint.

The reason I prefer this model to separate tables is that it allows us
to map multiple other models (tags, comments) to both cover letters and
patches equally. However, that doesn't apply to everything (checks).
I'm still working on it.

Stephen

> 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@lists.ozlabs.org
> > > > > > https://lists.ozlabs.org/listinfo/patchwork
> > > > > 
> > > > > _______________________________________________
> > > > > Patchwork mailing list
> > > > > Patchwork@lists.ozlabs.org
> > > > > https://lists.ozlabs.org/listinfo/patchwork
> > > > 
> > > > 
> > > >
Daniel Axtens Oct. 2, 2018, 1:09 a.m. | #8
Stephen Finucane <stephen@that.guru> writes:

> On Mon, 2018-10-01 at 22:25 +1000, Daniel Axtens wrote:
>> Stephen Finucane <stephen@that.guru> writes:
>> 
>> > On Mon, 2018-10-01 at 05:37 -0400, Veronika Kabatova wrote:
>> > > 
>> > > ----- Original Message -----
>> > > > From: "Stephen Finucane" <stephen@that.guru>
>> > > > To: "Daniel Axtens" <dja@axtens.net>, vkabatov@redhat.com, patchwork@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@redhat.com writes:
>> > > > > 
>> > > > > > From: Veronika Kabatova <vkabatov@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@axtens.net>
>> > > > > > Signed-off-by: Veronika Kabatova <vkabatov@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.
>
> No, "single table inheritance" means we'd fold Patch and CoverLetter
> models back into Submission and add a 'type' column. This seems
> reasonable given that all the Patch-specific fields are nullable. The
> only issue I'm finding is making sure a series only has one
> CoverLetter. Essentially we would need some kind of ForeignKey
> constraint.
>
> The reason I prefer this model to separate tables is that it allows us
> to map multiple other models (tags, comments) to both cover letters and
> patches equally. However, that doesn't apply to everything (checks).
> I'm still working on it.

Righty. That's OK with me. I'm also happy for there to be checks
attached to cover letters - either we can just hide it in the UI or we
can use it to represent checks applied to an entire series.

Not as critical as we don't ever need to list them all, but where do
comments fit into this design?

Regards,
Daniel

Patch

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()