Skip to content

Commit 171ee9c

Browse files
wes-otfwes-otf
authored
Reverted to using difflab for revisions comparsions [redux] (#4610)
<!-- Thanks for contributing to Hypha! Please ensure your contributions pass all necessary linting/testing and that the appropriate documentation has been updated. --> (The other merge got botched so this is essentially a copy of #4609) This is a temporary reversion to difflab for the revisions comparison as the htmldiff library was using too much memory and timing out when the compare view was loaded. Eventually would be nice to make an html type diff work as lots is lost in the stripping of the html tags. ## Test Steps <!-- If step does not require manual testing, skip/remove this section. Give a brief overview of the steps required for a user/dev to test this contribution. Important things to include: - Required user roles for where necessary (ie. "As a Staff Admin...") - Clear & validatable expected results (ie. "Confirm the submit button is now not clickable") - Language that can be understood by non-technical testers if being tested by users --> - [ ] Ensure the compare view works as expected (side by side revisions) and matches the themes vibe
1 parent dfb51a2 commit 171ee9c

4 files changed

Lines changed: 213 additions & 86 deletions

File tree

‎hypha/apply/funds/differ.py‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
import re
2+
from difflib import SequenceMatcher
3+
from typing import Tuple
4+
5+
import nh3
6+
from django.utils.html import format_html
7+
from django.utils.safestring import mark_safe
8+
9+
10+
def wrap_deleted(text):
11+
return format_html("<del>{}</del>", mark_safe(text))
12+
13+
14+
def wrap_added(text):
15+
return format_html("<ins>{}</ins>", mark_safe(text))
16+
17+
18+
def compare(answer_a: str, answer_b: str, should_clean: bool = True) -> Tuple[str, str]:
19+
"""Compare two strings, populate diff HTML and insert it, and return a tuple of the given strings.
20+
21+
Args:
22+
answer_a:
23+
The original string
24+
answer_b:
25+
The string to compare to the original
26+
should_clean:
27+
Optional boolean to determine if the string should be sanitized with NH3 (default=True)
28+
29+
Returns:
30+
A tuple of the original strings with diff HTML inserted.
31+
"""
32+
33+
if should_clean:
34+
answer_a = re.sub("(<li[^>]*>)", r"\1◦ ", answer_a)
35+
answer_b = re.sub("(<li[^>]*>)", r"\1◦ ", answer_b)
36+
answer_a = nh3.clean(answer_a, tags=set(), attributes={})
37+
answer_b = nh3.clean(answer_b, tags=set(), attributes={})
38+
39+
diff = SequenceMatcher(None, answer_a, answer_b)
40+
from_diff = []
41+
to_diff = []
42+
for opcode, a0, a1, b0, b1 in diff.get_opcodes():
43+
if opcode == "equal":
44+
from_diff.append(mark_safe(diff.a[a0:a1]))
45+
to_diff.append(mark_safe(diff.b[b0:b1]))
46+
elif opcode == "insert":
47+
from_diff.append(mark_safe(diff.a[a0:a1]))
48+
to_diff.append(wrap_added(diff.b[b0:b1]))
49+
elif opcode == "delete":
50+
from_diff.append(wrap_deleted(diff.a[a0:a1]))
51+
to_diff.append(mark_safe(diff.b[b0:b1]))
52+
elif opcode == "replace":
53+
from_diff.append(wrap_deleted(diff.a[a0:a1]))
54+
to_diff.append(wrap_added(diff.b[b0:b1]))
55+
56+
from_display = "".join(from_diff)
57+
58+
to_display = "".join(to_diff)
59+
from_display = re.sub("(\\.\n)", r"\1<br><br>", from_display)
60+
to_display = re.sub("(\\.\n)", r"\1<br><br>", to_display)
61+
from_display = re.sub(r"([◦])", r"<br>\1", from_display)
62+
to_display = re.sub(r"([◦])", r"<br>\1", to_display)
63+
from_display = mark_safe(from_display)
64+
to_display = mark_safe(to_display)
65+
66+
return (from_display, to_display)

‎hypha/apply/funds/templates/funds/revisions_compare.html‎

Lines changed: 98 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -13,113 +13,134 @@
1313
{% endblock %}
1414

1515
{% block content %}
16-
<div class="my-4 mx-auto layout layout-flowrow-until-md layout-sidebar-flowrow-start">
17-
18-
<article class="layout-main">
16+
<div class="my-4 layout layout-flowrow-until-md layout-sidebar-flowrow-start">
17+
<div class="layout-main">
1918
<div class="mb-4">
2019
<h2 class="section-header">{% trans "Changes" %}</h2>
2120
<div class="flex flex-wrap gap-2">
22-
<span class="badge badge-soft">{{ from_revision.timestamp|date:"SHORT_DATETIME_FORMAT" }}</span>
21+
<span class="badge badge-soft">
22+
<relative-time format='datetime' datetime="{{ from_revision.timestamp|date:'c' }}">
23+
{{ from_revision.timestamp|date:'SHORT_DATETIME_FORMAT' }}
24+
</relative-time>
25+
</span>
2326

2427
{% if to_revision.id != from_revision.id %}
2528
{% trans "↔" %}
26-
<span class="badge badge-soft">{{ to_revision.timestamp|date:"SHORT_DATETIME_FORMAT" }}</span>
29+
<span class="badge badge-soft">
30+
<relative-time format='datetime' precision='second' datetime="{{ to_revision.timestamp|date:'c' }}">
31+
{{ to_revision.timestamp|date:'SHORT_DATETIME_FORMAT' }}
32+
</relative-time>
33+
</span>
2734
{% endif %}
2835
</div>
2936
</div>
37+
<table class="px-4 pb-4 w-full max-w-none prose card card-border prose-h2:mt-0 prose-h2:text-lg html-diff">
3038

31-
<div class="card card-border">
32-
<div class="gap-8 max-w-full card-body html-diff">
33-
{% for diff in required_fields %}
34-
{% if forloop.first %}
35-
<section>
36-
<h4 class="pb-1 mb-2 font-medium border-b text-h3 border-base-300 question">{% trans "Title" %}</h4>
37-
<div>{{ diff }}</div>
38-
</section>
39-
{% elif forloop.counter == 2 %}
40-
<section>
41-
<h4 class="pb-1 mb-2 font-medium border-b text-h3 border-base-300 question">{% trans "Legal Name" %}</h4>
42-
<div>{{ diff }}</div>
43-
</section>
44-
{% elif forloop.counter == 3 %}
45-
<section>
46-
<h4 class="pb-1 mb-2 font-medium border-b text-h3 border-base-300 question">{% trans "E-mail" %}</h4>
47-
<div>{{ diff }}</div>
48-
</section>
49-
{% elif forloop.counter == 4 %}
50-
<section>
51-
<h4 class="pb-1 mb-2 font-medium border-b text-h3 border-base-300 question">{% trans "Address" %}</h4>
52-
<div>{{ diff }}</div>
53-
</section>
54-
{% elif forloop.counter == 5 %}
55-
<section>
56-
<h4 class="pb-1 mb-2 font-medium border-b text-h3 border-base-300 question">{% trans "Project Duration" %}</h4>
57-
<div>{{ diff }}</div>
58-
</section>
59-
{% elif forloop.counter == 6 %}
60-
<section>
61-
<h4 class="pb-1 mb-2 font-medium border-b text-h3 border-base-300 question">{% trans "Requested Funding" %}</h4>
62-
<div>{{ diff }}</div>
63-
</section>
64-
{% elif forloop.counter == 7 %}
65-
<section>
66-
<h4 class="pb-1 mb-2 font-medium border-b text-h3 border-base-300 question">{% trans "Organization" %}</h4>
67-
<div>{{ diff }}</div>
68-
</section>
69-
{% else %}
70-
<section>
71-
<div>{{ diff }}</div>
72-
</section>
73-
{% endif %}
74-
{% endfor %}
75-
{% for diff in stream_fields %}
76-
{{ diff }}
77-
{% endfor %}
78-
</div>
79-
</div>
80-
</article>
39+
{% for from_field, to_field in required_fields %}
40+
{% if forloop.first %}
41+
<tr>
42+
<td><h2>{% trans "Title" %}</h2>{{ from_field }}</td>
43+
<td><h2>{% trans "Title" %}</h2>{{ to_field }}</td>
44+
</tr>
45+
{% elif forloop.counter == 2 %}
46+
<tr>
47+
<td><h2>{% trans "Legal Name" %}</h2>{{ from_field }}</td>
48+
<td><h2>{% trans "Legal Name" %}</h2>{{ to_field }}</td>
49+
</tr>
50+
{% elif forloop.counter == 3 %}
51+
<tr>
52+
<td><h2>{% trans "E-mail" %}</h2>{{ from_field }}</td>
53+
<td><h2>{% trans "E-mail" %}</h2>{{ to_field }}</td>
54+
</tr>
55+
{% elif forloop.counter == 4 %}
56+
<tr>
57+
<td><h2>{% trans "Address" %}</h2>{{ from_field }}</td>
58+
<td><h2>{% trans "Address" %}</h2>{{ to_field }}</td>
59+
</tr>
60+
{% elif forloop.counter == 5 %}
61+
<tr>
62+
<td><h2>{% trans "Project Duration" %}</h2>{{ from_field }}</td>
63+
<td><h2>{% trans "Project Duration" %}</h2>{{ to_field }}</td>
64+
</tr>
65+
{% elif forloop.counter == 6 %}
66+
<tr>
67+
<td><h2>{% trans "Requested Funding" %}</h2>{{ from_field }}</td>
68+
<td><h2>{% trans "Requested Funding" %}</h2>{{ to_field }}</td>
69+
</tr>
70+
{% else %}
71+
<tr>
72+
<td>{{ from_field }}</td>
73+
<td>{{ to_field }}</td>
74+
</tr>
75+
{% endif %}
76+
{% endfor %}
77+
{% for from_field, to_field in stream_fields %}
78+
<tr>
79+
<td>{{ from_field }}</td>
80+
<td>{{ to_field }}</td>
81+
</tr>
82+
{% endfor %}
83+
</table>
84+
</div>
8185

8286
<aside class="layout-sidebar">
8387
<div class="sticky top-4">
8488
<h2 class="mb-4 card-title">{% trans "Revisions" %}</h2>
8589
<div class="list">
8690
{% for revision in all_revisions %}
87-
<a
88-
class="list-row {% if revision.id == from_revision.id %} bg-base-300 {% else %}hover:bg-base-200{% endif %}"
89-
href="{{ revision.get_compare_url_to_latest }}"
90-
>
91-
<div class="list-col-grow">
92-
<span class="font-semibold">{{ revision.author }}</span> {% trans "edited" %}
91+
{% if forloop.first %}
92+
<div class="list-row">
93+
<div class="list-col-grow">
94+
<span class="font-semibold">{{ revision.author }}</span> {% trans "edited" %}
9395

94-
<relative-time datetime={{ revision.timestamp|date:"c" }} class="text-fg-muted">
95-
{{ revision.timestamp|date:"SHORT_DATETIME_FORMAT" }}
96-
</relative-time>
96+
<relative-time datetime={{ revision.timestamp|date:"c" }} class="text-fg-muted">
97+
{{ revision.timestamp|date:"SHORT_DATETIME_FORMAT" }}
98+
</relative-time>
9799

98-
{% if revision.is_draft %}
99-
<span class="uppercase badge badge-warning badge-outline">
100-
({% trans "draft" %})
101-
</span>
102-
{% endif %}
103-
</div>
100+
{% if revision.is_draft %}
101+
<span class="uppercase badge badge-warning badge-outline">
102+
({% trans "draft" %})
103+
</span>
104+
{% endif %}
105+
</div>
104106

105-
<div>
106-
{% if forloop.first %}
107+
<div>
107108
<span class="uppercase badge badge-info badge-outline">
108109
{% trans "latest" %}
109110
</span>
110-
{% else %}
111-
{% if revision.id != from_revision.id %}
111+
</div>
112+
</div>
113+
{% else %}
114+
<a
115+
class="list-row {% if revision.id == from_revision.id %} bg-base-300 {% else %}hover:bg-base-200{% endif %}"
116+
href="{{ revision.get_compare_url_to_latest }}"
117+
>
118+
<div class="list-col-grow">
119+
<span class="font-semibold">{{ revision.author }}</span> {% trans "edited" %}
120+
121+
<relative-time datetime={{ revision.timestamp|date:"c" }} class="text-fg-muted">
122+
{{ revision.timestamp|date:"SHORT_DATETIME_FORMAT" }}
123+
</relative-time>
124+
125+
{% if revision.is_draft %}
126+
<span class="uppercase badge badge-warning badge-outline">
127+
({% trans "draft" %})
128+
</span>
129+
{% endif %}
130+
</div>
131+
{% if revision.id != from_revision.id %}
132+
<div>
112133
<span
113134
class="btn btn-sm"
114135
href="{{ revision.get_compare_url_to_latest }}"
115136
>
116137
{% trans "view" %}
117138
{% heroicon_mini "arrow-right" class="size-4" %}
118139
</span>
119-
{% endif %}
140+
</div>
120141
{% endif %}
121-
</div>
122-
</a>
142+
</a>
143+
{% endif %}
123144
{% endfor %}
124145
</div>
125146
</div>

‎hypha/apply/funds/views/revisions.py‎

Lines changed: 44 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
1-
import html_diff
1+
import re
2+
from typing import List
3+
4+
import nh3
25
from django.shortcuts import get_object_or_404
36
from django.utils.decorators import method_decorator
4-
from django.utils.html import format_html
57
from django.views.generic import (
68
DetailView,
79
ListView,
@@ -11,6 +13,7 @@
1113
staff_required,
1214
)
1315

16+
from ..differ import compare
1417
from ..models import (
1518
ApplicationRevision,
1619
ApplicationSubmission,
@@ -96,12 +99,11 @@ def compare_revisions(self, from_data, to_data):
9699
to_required = self.render_required()
97100

98101
required_fields = [
99-
format_html(html_diff.diff(*fields))
100-
for fields in zip(from_required, to_required, strict=False)
102+
compare(*fields) for fields in zip(from_required, to_required, strict=False)
101103
]
102104

103105
stream_fields = [
104-
format_html(html_diff.diff(*fields))
106+
compare(*self.cleanse_stream_fields(*fields), should_clean=False)
105107
for fields in zip(
106108
from_rendered_text_fields, to_rendered_text_fields, strict=False
107109
)
@@ -135,3 +137,40 @@ def get_context_data(self, **kwargs):
135137
"stream_fields": stream_fields,
136138
}
137139
return super().get_context_data(**ctx, **kwargs)
140+
141+
def cleanse_stream_fields(self, a_field, b_field) -> List[str]:
142+
"""Sanitizes the HTML outside of the h2 heading
143+
This is a temp fix and we should move to full HTML diffing
144+
145+
Args:
146+
a_field: the field to sanitize
147+
b_field: the field to sanitize
148+
149+
Returns:
150+
The sanitized stream field answers in a list
151+
"""
152+
153+
sanitized_answers = []
154+
155+
for field in (a_field, b_field):
156+
# TODO: Using regex with HTML is not ideal but this temp until we move to xml parsing
157+
field_match = re.match(
158+
r"^\s*<section class=\".*\">\s*(<h2 class=\".*\">[\s\S]*?</h2>)([\s\S]*?)</section>",
159+
field,
160+
)
161+
try:
162+
# Keep h2 tags but purge any classes/attributes
163+
heading = nh3.clean(field_match.group(1), tags={"h2"}, attributes={})
164+
165+
# Handle lists on the answer fields by subbing HTML for chars
166+
answer = re.sub("(<li[^>]*>)", r"\1◦ ", field_match.group(2))
167+
# Cleanse answer of HTML
168+
answer = nh3.clean(answer, attributes={}, tags=set())
169+
170+
sanitized_answers.append(f"{heading}{answer}")
171+
except AttributeError:
172+
# If it fails to match for some reason just cleanse the fields but leave h2s
173+
answer = nh3.clean(answer, attributes={}, tags={"h2"})
174+
sanitized_answers.append(field)
175+
176+
return sanitized_answers

‎hypha/static_src/tailwind/components/html-diff.css‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,10 @@
33
--marker-ms: -10px;
44
--marker-ps: 8px;
55

6-
:is(h1, h2, h3, h4, h5, h6):has(ins),
6+
/* :is(h1, h2, h3, h4, h5, h6):has(ins),
77
:is(h1, h2, h3, h4, h5, h6):has(del),
8+
tr:has(ins),
9+
tr:has(del),
810
ul:has(ins),
911
ul:has(del),
1012
ol:has(ins),
@@ -14,7 +16,7 @@
1416
border-inline-start: var(--marker-border-width) solid var(--color-warning);
1517
margin-inline-start: var(--marker-ms);
1618
padding-inline-start: var(--marker-ps);
17-
}
19+
} */
1820

1921
table,
2022
.prose {
@@ -42,8 +44,7 @@
4244
border-inline-start: var(--marker-border-width) solid var(--color-success);
4345
}
4446

45-
del,
46-
del * {
47+
del {
4748
@apply bg-error text-error-content;
4849
}
4950

0 commit comments

Comments
 (0)