Code improvement feedback app by vanlummelhuizen · Pull Request #1672 · Signbank/Global-signbank

vanlummelhuizen · 2025-12-11T09:15:55Z

This is an attempt at making the code of the feedback app more clean, DRY-er, easier to read and to maintain. Please let me know what you think.

Copilot

Pull request overview

This pull request refactors the feedback app to improve code quality, reduce duplication, and enhance maintainability. The changes consolidate repetitive view logic, simplify control flow patterns, and modernize code conventions.

Key changes:

Introduced a generic_showfeedback helper function to eliminate duplicated permission checking and rendering logic across multiple views
Refactored recordsignfeedback to handle both glosses and morphemes through a single code path using an is_morpheme parameter
Simplified form handling patterns by inverting conditionals (checking for POST early and returning)

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
signbank/feedback/views.py	Core refactoring: consolidated duplicate view logic, simplified control flow with early returns, modernized string formatting to f-strings
signbank/feedback/urls.py	Cleaned up imports by removing unused dependencies
signbank/feedback/templates/feedback/show_feedback_signs.html	Fixed display of sign languages to properly iterate and comma-separate values
signbank/feedback/models.py	Reorganized imports, simplified conditional expressions, removed unused `__init__` method, updated to modern `super()` syntax
signbank/feedback/admin.py	Minor formatting cleanup removing extra blank lines

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-11T12:58:31Z

signbank/feedback/views.py

+    return render(request, template_name, context)
+
+
+def generic_showfeedback(request: QueryDict, extra_context: dict, template_name: str) -> HttpResponse:


The request parameter should be typed as HttpRequest, not QueryDict. QueryDict is the type for request.GET and request.POST, not the request object itself.

Copilot · 2025-12-11T12:58:31Z

signbank/feedback/views.py

+    extra_context = {'morphfb': (MorphemeFeedback.objects
+                                 .filter(morpheme__lemma__dataset__in=get_selected_datasets(request))
+                                 .filter(status__in=('unread', 'read')))}
+    return generic_showfeedback(request, extra_context,"feedback/show_feedback_morphemes.html")


Missing space after comma before the template name argument.

Copilot · 2025-12-11T12:58:31Z

signbank/feedback/views.py

-    else:
-        gloss = get_object_or_404(Gloss, id=glossid, archived=False)
-    return recordsignfeedback(request, glossid)
+    return recordsignfeedback(request, glossid, is_morpheme=False)


The parameter name glossid is inconsistent with the updated function signature which uses id. Consider renaming the parameter in glossfeedback and morphemefeedback to id for consistency.

Copilot · 2025-12-11T12:58:32Z

signbank/feedback/views.py

    value = request.POST.get('value', '')

-    (what, fbid) = field.split('_')
+    _, id = field.split('_')


The variable id shadows the function parameter with the same name. Use a different name like feedback_id to avoid confusion.

Copilot · 2025-12-11T12:58:32Z

signbank/feedback/models.py


    def __init__(self, *args, **kwargs):
-        super(SignFeedbackForm, self).__init__(*args, **kwargs)
+        super().__init__(*args, **kwargs)


The MorphemeFeedbackForm.__init__ method was removed, but it should be retained if there's any initialization logic needed for consistency with other form classes. If truly unnecessary, this is fine.

susanodd · 2025-12-15T10:52:38Z

A lot of improvements. The original code was very weird, especially now seeing it next to the new code.
The additions of expression conditionals instead of if-else branching on empty values improves the code a lot.
It's great those weird "values" with the ids in the forms have been removed!

The method with country and language,

Global-signbank/signbank/feedback/models.py

Lines 16 to 19 in 39df090

def t(message):

"""Replace $country and $language in message with dat from settings"""

tpl = string.Template(message)

return tpl.substitute(country=settings.COUNTRY_NAME, language=settings.LANGUAGE_NAME)

Is this being used for the feedback?

The code runs great! It looks like users have been using the feedback!

Just that question about that function.

I can't get the review thing to work for me. It won't let me comment in specific places.

Woseseltops

Thanks for cleaning up! Almost 200 lines of code less with the same functionality must be a great improvement to maintainability :)

Woseseltops · 2025-12-19T15:30:49Z

signbank/feedback/templates/feedback/show_feedback_missing_signs.html

                     </div>
                  <form action="{{PREFIX_URL}}/feedback/missingsign/delete/{{fb.pk}}" method='post'>
                      {% csrf_token %}
-                      <input type='hidden' name='id' value='deletefeedbackmissingn_{{fb.pk}}'>


Are you sure this doesn't do anything? I would guess this is an essential parameter to communicate which feedback needs deleting.

The ID of the Feedback instance that is to be deleted, is send through the url in the form action. As far as I can tell the hidden input is never used in

Global-signbank/signbank/feedback/views.py

Lines 207 to 226 in 39df090

def delete(request, kind, id):

"""Mark a feedback item as deleted, kind 'signfeedback', 'generalfeedback' or 'missingsign'"""

url = request.META.get('HTTP_REFERER', '/')

kinds = {

'sign': SignFeedback,

'morpheme': MorphemeFeedback,

'general': GeneralFeedback,

'missingsign': MissingSignFeedback

}

if kind not in kinds:

return redirect(url)

model = kinds[kind]

if request.POST.get('value', '') == 'confirmed':

item = get_object_or_404(model, pk=id)

item.status = 'deleted'

item.save()

return redirect(url)

Yes, I couldn't find it either.

Woseseltops · 2025-12-19T15:37:52Z

signbank/feedback/views.py

+    missing_sign_feedback = MissingSignFeedback(user=request.user)
+    if 'signlanguage' in form.cleaned_data:
+        missing_sign_feedback.signlanguage = form.cleaned_data['signlanguage']
+    if 'video' in form.cleaned_data and form.cleaned_data['video'] is not None:
+        missing_sign_feedback.video = form.cleaned_data['video']
+    if 'sentence' in form.cleaned_data and form.cleaned_data['sentence'] is not None:
+        missing_sign_feedback.sentence = form.cleaned_data['sentence']
+    missing_sign_feedback.meaning = form.cleaned_data['meaning']
+    missing_sign_feedback.comments = form.cleaned_data['comments']


How about

cleaned_data = form.cleaned_data missing_sign_feedback = MissingSignFeedback( user=request.user, meaning=cleaned_data['meaning'], comments=cleaned_data['comments'], signlanguage=cleaned_data.get('signlanguage'), video=cleaned_data.get('video'), sentence=cleaned_data.get('sentence') )

Since the fields signlanguage, video and sentence can handle None as input (I checked), this should also work. It is a nice improvement. I will apply it.

susanodd · 2026-01-16T11:34:25Z

This is the comments for my review. Not sure if you looked at these.

@vanlummelhuizen can you check this code?

Global-signbank/signbank/feedback/models.py

Lines 16 to 19 in 39df090

def t(message):

"""Replace $country and $language in message with dat from settings"""

tpl = string.Template(message)

return tpl.substitute(country=settings.COUNTRY_NAME, language=settings.LANGUAGE_NAME)

Can the above be deleted from the feedback?

I'm working on the communication emails and there is a context variable that uses settings.LANGUAGE_NAME and in the emails it ends up ultimately as "Global Signbank" (as the result of evaluating the context variables)

signbank/settings/server_specific/default.py:LANGUAGE_NAME = 'Global'
signbank/settings/server_specific/asl_yale.py:LANGUAGE_NAME = "ASL"

As well as other that use code:

current_site = Site.objects.get_current()

This is actually WRONG, it yields:

signbank.science.ru.nl

(from the database)

So the wrong thing is ending up in the emails...

vanlummelhuizen added 14 commits December 3, 2025 14:08

Improvements suggested by linters/analysers Ruff and Flake8

59fc13c

Improvements suggested by Pylint

86cf699

Remove or shorten code and other improvements

b0a85e8

Overhaul view 'generalfeedback'

8663ada

Overhaul view 'missingsign'

9039d2c

Overhaul view 'showfeedback'

0021c3f

Overhaul view 'showfeedback_signs'

4614761

Overhaul view 'showfeedback_morphemes'

946782f

Overhaul view 'showfeedback_missing'

83e8a58

Make showfeedback view more DRY

c9dc7b6

fix: show nice list instead of queryset string

216ed0e

Overhaul view helper 'recordsignfeedback

3d174ce

Overhaul view 'recent_feedback'

f429b74

Remove unused imports

c3684d4

vanlummelhuizen marked this pull request as ready for review December 11, 2025 09:32

vanlummelhuizen requested review from Woseseltops and susanodd December 11, 2025 09:33

Woseseltops requested a review from Copilot December 11, 2025 12:57

Copilot AI reviewed Dec 11, 2025

View reviewed changes

Improvements per CoPilot review

39df090

Woseseltops reviewed Dec 19, 2025

View reviewed changes

Improvement per review from @Woseseltops

e1a4486

susanodd mentioned this pull request Jan 16, 2026

Communication class and admin for email texts #1658

Merged

susanodd approved these changes Mar 26, 2026

View reviewed changes

vanlummelhuizen requested a review from Woseseltops March 26, 2026 13:50

		return render(request, template_name, context)


		def generic_showfeedback(request: QueryDict, extra_context: dict, template_name: str) -> HttpResponse:

	def delete(request, kind, id):
	"""Mark a feedback item as deleted, kind 'signfeedback', 'generalfeedback' or 'missingsign'"""
	url = request.META.get('HTTP_REFERER', '/')

	kinds = {
	'sign': SignFeedback,
	'morpheme': MorphemeFeedback,
	'general': GeneralFeedback,
	'missingsign': MissingSignFeedback
	}
	if kind not in kinds:
	return redirect(url)
	model = kinds[kind]

	if request.POST.get('value', '') == 'confirmed':
	item = get_object_or_404(model, pk=id)
	item.status = 'deleted'
	item.save()

	return redirect(url)

Conversation

vanlummelhuizen commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

susanodd commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Woseseltops left a comment

Choose a reason for hiding this comment

Uh oh!

Woseseltops Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

vanlummelhuizen Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

susanodd Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

Woseseltops Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

vanlummelhuizen Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

susanodd commented Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vanlummelhuizen commented Dec 11, 2025 •

edited

Loading

susanodd commented Dec 15, 2025 •

edited

Loading

susanodd commented Jan 16, 2026 •

edited

Loading