Stephen Finucane <[email protected]> writes: > On Mon, 2018-10-01 at 05:37 -0400, Veronika Kabatova wrote: >> >> ----- Original Message ----- >> > From: "Stephen Finucane" <[email protected]> >> > To: "Daniel Axtens" <[email protected]>, [email protected], >> > [email protected] >> > 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: >> > > [email protected] writes: >> > > >> > > > From: Veronika Kabatova <[email protected]> >> > > > >> > > > 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 <[email protected]> >> > > > Signed-off-by: Veronika Kabatova <[email protected]> >> > > >> > > 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 >> > > > [email protected] >> > > > https://lists.ozlabs.org/listinfo/patchwork >> > > >> > > _______________________________________________ >> > > Patchwork mailing list >> > > [email protected] >> > > https://lists.ozlabs.org/listinfo/patchwork >> > >> > >> > _______________________________________________ Patchwork mailing list [email protected] https://lists.ozlabs.org/listinfo/patchwork
