Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/diff and other minor issues. #711

Merged
merged 21 commits into from
Oct 22, 2024
Merged

Fix/diff and other minor issues. #711

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
8093d5e
fix: update the diff template
EdoStorm96 Sep 26, 2024
cd09962
fix: make sure StudyDesign also deletes study types
EdoStorm96 Sep 26, 2024
06a6da9
fix: small bug on task_create
EdoStorm96 Sep 26, 2024
f9b095b
Merge branch 'major/4' into fix/diff
EdoStorm96 Sep 30, 2024
82863a6
feat: dumb css implementation on compare_documents.html
EdoStorm96 Sep 30, 2024
690ae5a
fix: simplify diff css and put in own diff.css file
EdoStorm96 Oct 1, 2024
da35b00
fix: use Session.repeats too for Session.net_duration()
EdoStorm96 Oct 1, 2024
7d45799
fix: reinsert diff-ignore
EdoStorm96 Oct 1, 2024
0eed19e
style: back to black
EdoStorm96 Oct 1, 2024
13f4a88
fix: change default view for compare_documents to split
EdoStorm96 Oct 7, 2024
5bc4b70
fix: make compare_documents extend fetc_base
EdoStorm96 Oct 7, 2024
ab1ce42
fix: improve styling on compare_documents.html
EdoStorm96 Oct 8, 2024
87c1c26
style: djlint
EdoStorm96 Oct 8, 2024
aef04eb
fix: undo self.repeats in Session.net_duration()
EdoStorm96 Oct 10, 2024
54118f9
Merge branch 'major/4' into fix/diff
EdoStorm96 Oct 10, 2024
0d9376d
fix: typo
EdoStorm96 Oct 10, 2024
565bff2
fix: replace th with td on row.verbose_name in diff
EdoStorm96 Oct 10, 2024
aa21af3
fix: implement get property methods for study_types, to not have to d…
EdoStorm96 Oct 10, 2024
1a1224a
style: style
EdoStorm96 Oct 10, 2024
8167d23
fix: correct title variable proposal_confirmation.html
EdoStorm96 Oct 10, 2024
c320001
Merge branch 'major/4' into fix/diff
EdoStorm96 Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions main/static/main/diff.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
.diff del {
text-decoration: none;
background-color: #ff6565;
}

.diff ins {
text-decoration: none;
background-color: #73ff73;
}
7 changes: 4 additions & 3 deletions proposals/templates/proposals/compare_documents.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
{% endblock %}

{% block html_head %}
<link rel="stylesheet" type="text/css" href="{% static 'main/diff.css' %}">
<script src="{% static 'main/js/htmldiff.js' %}"></script>
<script defer>
$(function () {
Expand Down Expand Up @@ -90,8 +91,8 @@ <h4>
<div class="col-12 text-center mt-5 mb-5" id="loading-icon">
<img src="{% static 'main/images/loading.gif' %}" />
</div>
<div style="display: none" class="col-6 split_view" id="old_text">{{ old_text|linebreaks }}</div>
<div style="display: none" class="col-6 split_view" id="new_text">{{ new_text|linebreaks }}</div>
<div class="col-12" id="combined_text"></div>
<div style="display: none" class="col-6 split_view diff" id="old_text">{{ old_text|linebreaks }}</div>
<div style="display: none" class="col-6 split_view diff" id="new_text">{{ new_text|linebreaks }}</div>
<div class="col-12 diff" id="combined_text"></div>
</div>
{% endblock %}
1 change: 1 addition & 0 deletions proposals/templates/proposals/proposal_diff.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
{% endblock %}

{% block html_head %}
<link rel="stylesheet" type="text/css" href="{% static 'main/diff.css' %}">
<script src="{% static 'main/js/htmldiff.js' %}"></script>
<script>
$(function () {
Expand Down
83 changes: 44 additions & 39 deletions proposals/templates/proposals/table_with_header_diff.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,48 +1,53 @@
{% load diff_tags %}
{% load i18n %}
{% load static %}

{% if section_title %}
<h2 class="mt-5 mb-1">{% trans section_title %}</h2>
{% endif %}
{% if sub_title %}<h3 class='mt-3'>{{ sub_title }}</h3>{% endif %}
<table class="proposals-diff ">
<tr>
<th class="proposals-diff-question"></th>
<th class="proposals-diff-answer">{% trans "Vorige aanvraag" %}</th>
<th class="proposals-diff-answer">{% trans "Huidige aanvraag" %}</th>
</tr>
{% if warning %}
{% for row in rows|slice:":1" %}
<tr class="diff-ignore">
<th>{{ row.verbose_name }}</th>
{# If the first object is missing, place a warning on the first TD #}
{% if missing_object == 'old' %}
<td rowspan='{{ rows|length }}'>
<div class='warning'>{{ warning }}</div>
</td>
<table class="table">
<thead>
<tr>
<th scope="col" width="30%"></th>
<th scope="col" width="35%">{% trans "Vorige aanvraag" %}</th>
<th scope="col" width="35%">{% trans "Huidige aanvraag" %}</th>
</tr>
</thead>
<tbody>
{% if warning %}
{% for row in rows|slice:":1" %}
<tr class="diff-ignore">
<th>{{ row.verbose_name }}</th>
{# If the first object is missing, place a warning on the first TD #}
{% if missing_object == 'old' %}
<td rowspan='{{ rows|length }}' class="align-middle">
<div class='alert alert-info'>{{ warning }}</div>
</td>
<td>{{ row.value }}</td>
{# Otherwise, we're missing the second one, thus we place the warning on the second #}
{% elif missing_object == 'new' %}
<td>{{ row.value }}</td>
<td rowspan='{{ rows|length }}' class="align-middle">
<div class='alert alert-info'>{{ warning }}</div>
</td>
{% endif %}
</tr>
{% endfor %}
{% for row in rows|slice:"1:" %}
<tr class="diff-ignore">
<th>{{ row.verbose_name }}</th>
<td>{{ row.value }}</td>
{# Otherwise, we're missing the second one, thus we place the warning on the second #}
{% elif missing_object == 'new' %}
<td>{{ row.value }}</td>
<td rowspan='{{ rows|length }}'>
<div class='warning'>{{ warning }}</div>
</td>
{% endif %}
</tr>
{% endfor %}
{% for row in rows|slice:"1:" %}
<tr class="diff-ignore">
<th>{{ row.verbose_name }}</th>
<td>{{ row.value }}</td>
</tr>
{% endfor %}
{% else %}
{% for row in rows %}
<tr>
<th>{{ row.verbose_name }}</th>
<td>{{ row.old_value }}</td>
<td>{{ row.new_value }}</td>
</tr>
{% endfor %}
{% endif %}
</tr>
{% endfor %}
{% else %}
{% for row in rows %}
<tr class="diff">
<th>{{ row.verbose_name }}</th>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks better as a <td> rather than a <th>. The page has so much bold text on it right now.

<td>{{ row.old_value }}</td>
<td>{{ row.new_value }}</td>
</tr>
{% endfor %}
{% endif %}
</tbody>
</table>
9 changes: 9 additions & 0 deletions studies/views/study_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,15 @@ def form_valid(self, form):
for study_type in form.fields["study_types"].choices:
form_value = study_type[0] in form.data.getlist("study_types")
form.instance.__setattr__(study_type[0], form_value)
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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

form.instance.save()

return super(StudyDesign, self).form_valid(form)
Expand Down
2 changes: 1 addition & 1 deletion tasks/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, net duration should not take these repeats into account.


return 0

Expand Down
3 changes: 3 additions & 0 deletions tasks/views/task_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ def get_session(self):
"""Retrieves the Study from the pk kwarg"""
return Session.objects.get(pk=self.kwargs["pk"])

def get_proposal(self):
return self.get_session().study.proposal


class TaskUpdate(TaskMixin, UpdateView):
pass
Expand Down
Loading