Fix/diff and other minor issues. #711
Conversation
tasks/models.py
Outdated
| @@ -49,7 +49,7 @@ def net_duration(self): | |||
| if duration := self.task_set.annotate( | |||
| total_duration=models.F("duration") * models.F("repeats") | |||
| ).aggregate(models.Sum("total_duration"))["total_duration__sum"]: | |||
| return duration | |||
| return duration * self.repeats | |||
There was a problem hiding this comment.
Ok, now that I'm reading the help text again, I am actually thinking, the old version, which did not take session repeats into account is more accurate to the goal of this question. What do you think?
There was a problem hiding this comment.
I agree, net duration should not take these repeats into account.
| @@ -50,15 +51,15 @@ <h2>{% trans 'Vergelijk documenten' %}</h2> | |||
| <p> | |||
| {% blocktrans trimmed %} | |||
| Hier kan je twee versies van een document vergelijken. Standaard | |||
| geeft hij een <i>gecombineerde</i> versie weer, waarbij tekst | |||
| geeft hij een <i>gesplitse</i> versie weer, waarbij tekst | |||
There was a problem hiding this comment.
Er mist nog een T in gesplitste
| {% else %} | ||
| {% for row in rows %} | ||
| <tr class="diff"> | ||
| <th>{{ row.verbose_name }}</th> |
There was a problem hiding this comment.
I think this looks better as a <td> rather than a <th>. The page has so much bold text on it right now.
tasks/models.py
Outdated
| @@ -49,7 +49,7 @@ def net_duration(self): | |||
| if duration := self.task_set.annotate( | |||
| total_duration=models.F("duration") * models.F("repeats") | |||
| ).aggregate(models.Sum("total_duration"))["total_duration__sum"]: | |||
| return duration | |||
| return duration * self.repeats | |||
There was a problem hiding this comment.
I agree, net duration should not take these repeats into account.
studies/views/study_views.py
Outdated
| if not form.instance.has_intervention and hasattr( | ||
| form.instance, "intervention" | ||
| ): | ||
| form.instance.intervention.delete() | ||
| if not form.instance.has_observation and hasattr(form.instance, "observation"): | ||
| form.instance.observation.delete() | ||
| if not form.instance.has_sessions and form.instance.session_set.all(): | ||
| for session in form.instance.session_set.all(): | ||
| session.delete() |
There was a problem hiding this comment.
It can be annoying to have these instances dangling around when the type is no longer relevant. But the unintentional feature that comes with it is that users can undo deletion by re-enabling it. Which is nice and we would probably want to keep.
Best to solve the issues with these objects appearing elsewhere symptomatically.
To do this right, we could move to a Study.get_sessions() function where needed. That function would return no sessions if not study.as_sessions.
Or we could assign custom managers to things like study_set and sessions_set that do the same.
Just a suggestion. A couple if statements to fix the diffs is fine by me as well, just a little harder to maintain.
|
Ok, so I've adressed your comments. I've also merged major/4 into this branch to avoid conflicts. So I implemented get_intervention, get_observation and get_sessions property methods on the Study model. For now we are only using these for the diff ... I guess it could remove some if-statements in other places ... What do you think? Have a look at other places where this can be used or just leave it? |
|
Added a little bonus fix for #701 |
| return self.observation | ||
|
|
||
| @property | ||
| def get_sessions(self): |
There was a problem hiding this comment.
This method will either return a list of sessions, or implicitly return None. I would prefer if it returned an empty list if the proposal has no sessions.
If we do that then for session in proposal.get_sessions(): will just do nothing, rather than produce an error. And the empty list will still be falsey for checking if a proposal has sessions.
There was a problem hiding this comment.
It will actually return a QS or a list in that case, but at least they're both iterable.
An empty QS is still falsey thankfully.
So this branch is just some odds and ends.
I firstly wanted to update the diff page, so that it makes use of Bootstrap's table class and such. I had to add some css to make the styling look like the old version. This is now in the diff.css static file.
When testing the diff, I noticed that different study types (sessions, interventions, observations) remain linked to the proposal, even though you might change your proposal's design to not include them. So I added the ability to StudyDesign to delete objects, as well as create them.
I found that TaskCreate did not receive a proposal, so I fixed this.
I also added the diff.css static file to the compare documents pages, so that it uses the same styling.
Finally I noticed that Session.net_duration(), did not take the repeats of the session into account. So I've implemented this as well.
Sorry for this chaotic PR ... I keep noticing small things and wanting to fix it right away ... Hope you can make sense of it. The commits help :)