Conversation
ZoqkMaze
force-pushed
the
q_management
branch
from
af5b9f8 to
c69ac77
Compare
niklasmohrin
left a comment
niklasmohrin
left a comment
There was a problem hiding this comment.
do you want to get feedback yet? I took only a very quick look over the changes so far
Yes, please. Hopefully, only one test is missing |
|
@hansegucker @fekoch @jooooosef @ybrnr I won't find time to review this before I am there tomorrow, and by then I expect that other things will have come up. Can some of you give a review round or two on this? Maybe if you have some time tomorrow at the meeting :) |
ybrnr
left a comment
ybrnr
left a comment
There was a problem hiding this comment.
you should also run ./manage.py precommit
ZoqkMaze
force-pushed
the
q_management
branch
from
c395de3 to
0d09e3b
Compare
niklasmohrin
left a comment
niklasmohrin
left a comment
There was a problem hiding this comment.
looks good, but there are some rough corners we should still fix
fekoch
left a comment
fekoch
left a comment
There was a problem hiding this comment.
@janno42 and me rethought about the "All" - "Visible" - "Non-Archived" filter-buttons. Can you change them to be the following:
- "All" (the default) should show all non-archived questionnaires
- "Visible": same as before
- "Archived": only archived questionaires
ZoqkMaze
force-pushed
the
q_management
branch
from
cee7d11 to
b34a6db
Compare
janno42
left a comment
janno42
left a comment
There was a problem hiding this comment.
Please remove the text "There are no matching questionnaires" and instead keep the empty table (with header). We will address the handling of empty tables in a separate issue.
niklasmohrin
left a comment
niklasmohrin
left a comment
There was a problem hiding this comment.
Everything seems to work as expected, some small comments on the code
| return result | ||
|
|
||
|
|
||
| def get_string_from_url_or_session(request: HttpRequest, parameter: str, default=False) -> str: |
There was a problem hiding this comment.
Not sure if there is a sensible default, but the type annotation should surely be:
| def get_string_from_url_or_session(request: HttpRequest, parameter: str, default=False) -> str: | |
| def get_string_from_url_or_session(request: HttpRequest, parameter: str, default: str) -> str: |
I am not sure why the function passed typechecking in the first place, maybe the GET.get returns Any?
| self.assertEqual(counter.text, "0") | ||
|
|
||
|
|
||
| class QuestionnaireLiveTest(LiveServerTest): |
There was a problem hiding this comment.
Very nice, this is a very clear test :)
| <a href="{% url 'staff:questionnaire_index' %}?filter_questionnaires=false" role="button" class="btn btn-sm btn-light{% if not filter_questionnaires %} active{% endif %}"> | ||
| {% translate 'Show' %} | ||
| <a href="{% url 'staff:questionnaire_index' %}?filter_questionnaires=all" role="button" class="btn btn-sm btn-light{% if filter_questionnaires == 'all' %} active{% endif %}"> | ||
| {% translate 'All' %} |
There was a problem hiding this comment.
| {% translate 'All' %} | |
| {% translate 'All' %} |
| {% if questionnaires %} | ||
| <div class="card collapsible {{ extra_classes }}"> |
There was a problem hiding this comment.
Can you remove the two leading spaces in the rest of the file? Currently, the whole file is indented
|
Note that we still have an "There are no questionnaires yet." with no table at all if there are no questionnaires in any of the tabs; @janno42 do you want this one to also go away? |
7 participants
fix #2545