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.

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

Feature/multiple sessions tasks #603

Closed
wants to merge 60 commits into from

Conversation

EdoStorm96
Copy link
Contributor

Fixes #373. For the solution to this problem, I went with Michael's proposed solution to the problem. Basically, both sessions and tasks are now created iteratively. So when done creating a session or a task, you can choose to make another or continue to the study overview (eventually). To make sure we get rid of overly long PDF's with repeating tasks and sessions, both session and tasks now have an attribute called repeats, which just signifies how often these will be performed. This means that behind the scenes, we just have one session object, which might get repeated three times, whereas we used to have 3 sessions for such a case. Implementing this of course had other implications:

  • sessions_/tasks_number is now a model method, returning eg. self.task_set.count()
  • I got rid of the SessionStart view, as that was just used for giving the initial sessions_number and is now longer needed. Its help text is now on the SessionUpdate view, which is (from the user POV) the first page you see when creating session.
  • I changed up some of the names of the views, as I found them to be a bit confusing.
  • session.net_duration is now a bit more spicy, as I needed to multiply task.repeats by task.duration and then sum that for all tasks.
  • I added edit and delete buttons for everything in the StudyEnd view. Although this leads to a weird UI quirk for me of like an _ appearing between the items?
  • I had to edit form_buttons.html to accomodate for a "Save and create a new session/task >>" option.
  • I had to make some changes to the pdf_diff_logic. I got rid of the SessionOverviewSection, as that was the equivalent of the SessionStart view and just showed the sessions number. To not lose its section title, I put some logic in the SessionSection to display its title, if session.order == 1.
  • I removed most of the copying functionality, as that seemed obsolete, given you can now just say a session/task has repeats ...

Some last bugs/things to note/ideas:

  • We probably need some new help text, explaining this process. What do you think would be a good place for that? I think the process is bit more intuitive, anyway ...
  • When the empty UpdateView is rendered after SessionCreate or TaskCreate all field required warnings light up. I guess because the page gets loaded with a mostly empty object ... any ideas on how to overcome this?
  • Maybe we can add a button to create a task or session to the StudyEnd page?

I'm pretty pleased with how this turned out. Based on my testing, this works well, but I might have missed some things. I'm curious to hear your suggestions! Good luck on the review and take your time! ;p

@EdoStorm96 EdoStorm96 requested review from tymees and miggol January 31, 2024 10:11
@EdoStorm96
Copy link
Contributor Author

I'm not sure what to do about the black PR checks ... When I attempt to run black on the files in question (some of which I have not changed in this branch) on my device, it leaves them unchanged.

@tymees
Copy link
Member

tymees commented Feb 1, 2024

Haven't looked at the code yet, but regarding this comment:

I'm not sure what to do about the black PR checks ... When I attempt to run black on the files in question (some of which I have not changed in this branch) on my device, it leaves them unchanged.

I ran into this problem as well yesterday @ DIAPP, it seems Black has been updated to include a new rule. (Resulting in the most annoyed commit message I've written lately).
Try updating black.

(Although, the changes in the migration file should've been picked up by your black version ;) )

@tymees
Copy link
Member

tymees commented Feb 1, 2024

For if Michael wants to take a look at this PR as well, Edo and I also did an in person look at this PR. During that conversation, I ended up pushing some changes:

  • Removed the weird '_' Edo noticed
  • By complete accident we realized the current develop always submits the application on the submit page, even if one presses the back button. This is due to a change in start_review and some blocking conditions that were accidentally left out. I restored them
  • I refactored the form buttons template. The previous design was very crowded (with all the different variations of back buttons). This had the added effect of making the new 'create new task' and 'create new session' buttons not stand out as a separate action. I removed all but one of the previous buttons (the 'prev step' button), as our stepper can also be used for that. In addition, I added as much whitespace as possible between the prev and next buttons to visually indicate what they do.

Copy link
Member

@tymees tymees left a comment

Choose a reason for hiding this comment

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

Just commenting because I didn't find any blockers, but also had a lot of small enhancement suggestions. (So both approve/request changes didn't feel applicable.)

The code looks good, and the flow is definitely better this way, so this is already pretty great. With some small additional tweaks it could be awesome!

@@ -57,7 +56,7 @@ def action(self, request):
order = study.session_set.count() + 1
self.session = Session.objects.create(order=order, study=study)

def get_redirect_url(self, *args: Any, **kwargs: Any) -> str | None:
def get_redirect_url(self, *args, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

I would highly recommend running the same python version (3.9) as on the servers, so spot these python-incompatibility issues earlier.

(Referring to your change in 3fa8eda)

Comment on lines +42 to +44
if duration := self.task_set.annotate(
total_duration=models.F("duration") * models.F("repeats")
).aggregate(models.Sum("total_duration"))["total_duration__sum"]:
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

Comment on lines +385 to +386
def sessions_number(self):
return self.session_set.count()
Copy link
Member

Choose a reason for hiding this comment

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

You could consider decorating this as a @property, but this is just an suggestion

Comment on lines +49 to +50
def tasks_number(self):
return self.task_set.count()
Copy link
Member

Choose a reason for hiding this comment

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

You could consider adding @property here as well

Comment on lines 7 to 12
<th width="20%">{% trans "Naam" %}</th>
<th width="15%">{% trans "Nettoduur (in minuten)" %}</th>
<th width="10%">{% trans "Nettoduur (in minuten)" %}</th>
<th width="40%">{% trans "Registratie via" %}</th>
<th width="15%">{% trans "Feedback?" %}</th>
<th width="10%">{% trans "Acties" %}</th>
<th width="10%">{% trans "Herhalingen" %}</th>
<th width="5%">{% trans "Acties" %}</th>
Copy link
Member

Choose a reason for hiding this comment

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

We probably should refactor this to not use a 1997 HTML feature.

@@ -762,11 +754,13 @@ class SessionSection(BaseSection):
"setting_details",
"supervision",
"leader_has_coc",
"tasks_number",
"repeats",
Copy link
Member

Choose a reason for hiding this comment

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

I would move this question nearer to the top of the form. (Just under title maybe?)

For new users of the portal, it's actually a very important question as it can influence the way the person is going to fill in the form.

@@ -20,11 +20,6 @@ <h2>{% trans "Het takenonderzoek en interviews" %}</h2>
In de volgende vragen gaan we nader in op wat je in jouw onderzoek van je deelnemers zal verlangen. Daarbij gelden de volgende definities:
Copy link
Member

Choose a reason for hiding this comment

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

I think this file is unused now, so it should be removed.

On the other hand, as we discussed in person it might be an idea to revive this page as an 'informational only' page that explains the process the user is going to go through in the next few pages

tasks/models.py Outdated
_("Hoeveel taken worden er binnen deze sessie bij de deelnemer afgenomen?"),
null=True,
repeats = models.PositiveBigIntegerField(
_("Hoe vaak wordt deze sessie herhaald?"),
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion:
"Hoe vaak zal de participant deze sessie doorgaan?"

But then using proper Dutch/English

In addition, a help-text with more details could be worth looking at.

tasks/models.py Outdated
@@ -139,6 +138,16 @@ class Task(models.Model):
blank=True,
)

repeats = models.PositiveBigIntegerField(
_("Hoe vaak wordt deze taak herhaald?"),
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion:
"Hoe vaak moet de participant deze taak doen?"

But then, again, with proper Dutch/English

In addition, again, a help-text with more details could be worth looking at.

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 we should also specify that these are repetitions for each session. I fear that researchers will otherwise do mental math and fill in totals for their entire study here. Something like:

"How often will the participant perform this task per session."

Copy link
Member

Choose a reason for hiding this comment

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

As discussed, it might be an idea to add some 'create new task/session' buttons on this page as well for when the user didn't notice they had that option during creation.

Copy link
Contributor

@miggol miggol left a comment

Choose a reason for hiding this comment

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

Final small things:

  • In task_confirm_delete.html, the cancel button is missing the "button" CSS class. This must have already been missing.

tasks/models.py Outdated
@@ -139,6 +138,16 @@ class Task(models.Model):
blank=True,
)

repeats = models.PositiveBigIntegerField(
_("Hoe vaak wordt deze taak herhaald?"),
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 we should also specify that these are repetitions for each session. I fear that researchers will otherwise do mental math and fill in totals for their entire study here. Something like:

"How often will the participant perform this task per session."

<th width="40%">{% trans "Registratie via" %}</th>
<th width="15%">{% trans "Feedback?" %}</th>
<th width="10%">{% trans "Acties" %}</th>
<th width="10%">{% trans "Herhalingen" %}</th>
Copy link
Contributor

Choose a reason for hiding this comment

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

The use of the word "Repeats" could again be confusing here. A task that repeats once would mean you do it twice in total, right? But that's not how we're counting them.

"Times performed" is the best alternative I can think of.

<input class="button button-colored continue-button"
type="submit"
name="create_new_task"
value="{{ next_text|default:create_task_text }}" />
{% endif %}
{% endif %}
Copy link
Contributor

Choose a reason for hiding this comment

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

The create task/session buttons are currently scoped within the {% if not no_back %} if statement. That doesn't cause any problems right now but I don't think it's right.

In the no_back=1 case, i.e. the first page, the next button gets pushed all the way to the left. Maybe it's always been that way but it can be fixed with an else clause and empty div:

    <div class="button-grid">
        {% if not no_back %}
        <input class="button mr-auto button-black"
               type="submit"
               name="save_back"
               value="{% trans '<< Vorige stap' %}" />
        {% else %}
                <div class="mr-auto"></div>
        {% endif %}

Copy link
Member

Choose a reason for hiding this comment

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

The create task/session buttons are currently scoped within the {% if not no_back %} if statement. That doesn't cause any problems right now but I don't think it's right.

Yeah, good spot. It indeed does not cause any problems right now (if ever), but it's is kinda weird conceptually...

Maybe it's always been that way but it can be fixed with an else clause and empty div:

Nope, I introduced this bug in this PR. I've pushed a fix for it, which also moves the new buttons to the right scope. (As it made the fix easier and more readable as well).

<a href="{% url 'tasks:session_delete' session.pk %}">
<img src="{% static 'proposals/images/delete.png' %}"
title="{% trans 'Sessie verwijderen' %}">
title="{% trans 'Sessie aanpassen' %}">
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete and edit tooltips are mixed up here.

@@ -1037,7 +1032,6 @@ def create_context_pdf(context, model):
if study.has_observation:
sections.append(ObservationSection(study.observation))
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 all the major sections like ObservationSection and InterventionSection should get the PageBreakMixin treatment, and probably also each task and session.

If the subtitles like "Sesion 1, task 1" could also get a decent font size bump that would also be a great improvement.

@@ -481,7 +481,8 @@ def form_valid(self, form):
# Checks for practice proposals and starting the right
# kind of review happen over there.
proposal = form.instance
start_review(proposal)
if 'save_back' not in self.request.POST and 'js-redirect-submit' not in self.request.POST:
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, here's what I forgot.

@miggol
Copy link
Contributor

miggol commented Feb 7, 2024

Firstly, great job on this large PR. This was looking like quite the mountain of an issue but you've scaled it rather well.

When it comes to the flow this is of course a huge improvement, but we're not all the way there yet. Like Ty, I also think the Session overview and Study end pages should have create buttons for sessions and even tasks within them. But I would go even further and say that the StudyEnd/SessionEnd page should become a kind of "home base" that the submitter keeps returning to.

Currently I have the following issues with the flow:

  • It's hard to create a new session or task in general
  • It's easy to get lost inside the loop. One moment you're finishing up your intervention, and the next you're creating a new session
  • The new task/session actions are hidden deep within the loop, making it hard to figure out how far done you are as a submitter and which task/session you might want to create next
  • Lots of messy logic scattered throughout various get_next_url() functions to determine where to go next
    • For example, we keep stepping through every task with the next button, which is jarring if all you wanted to do is edit one detail
  • Annoyances like not being able to delete a task or session if that would leave you with none

I think using the overview pages as home bases would solve most of this:

  • Finished creating a session => Session overview
    From where you can create your first task

  • Finished describing a task => Session overview
    From where you can add another task

  • Completely finished describing a session => Session overview
    From where you can add another session, or optionally continue

I think this approach would simplify things for us and the users both.

Right now I'm leaving up in the air if the home base should just be StudyEnd which exposes all functionality in one go, or if SessionEnd should function as an intermediate home base. I think that everything could work on one page if we give StudyEnd a redesign. But maybe an intermediate stepping stone (SessionEnd) could be more userfriendly. My main argument is looping around an overview page rather than going on an adventure everytime you hit Next Step.

@EdoStorm96 EdoStorm96 marked this pull request as ready for review March 13, 2024 13:17
@EdoStorm96 EdoStorm96 requested a review from miggol March 13, 2024 13:18
@EdoStorm96
Copy link
Contributor Author

@miggol I've implemented all your requests. Have a look at them when you have time :) Ty told me he would not have time to review this, this week. But this is ofc in no hurry.

Copy link
Contributor

@miggol miggol left a comment

Choose a reason for hiding this comment

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

Found lots of stuff to complain about, feels like we're getting close though!


I'm not satisfied with the back buttons yet.

Heading back from SessionEnd brings you to SessionStart, when I think it should really bring you back to the sessions overview.

Heading back from SessionUpdate brings you all the way back to the sessions overview, rather than just SessionEnd. Although I can see that making sense if you're only just creating a new session, it feels very wrong when you're just updating a session's details. Overall I think going back to SessionEnd from SessionUpdate is a less bad option.

(though if we used CreateViews here instead of RedirectActionViews we could just have different back buttons for either situation)


There's also a strange bug with the menu bar, where some menu items remain unavailable:

image

When clicking them, you can't go there, but something in the redirect-submit code sets a ?next=%23# URL parameter, after which redirect-submit is broken and both the forward and back buttons won't work.

It's kind of a hilarious bug, the user is suddenly absolutely trapped. I don't have time right now to figure out why it happens exactly, but let me know if you get stuck and we can look at it together. Might be a complicated/weird/interesting bug.

@@ -15,11 +15,8 @@ <h2>{% trans "Sessie verwijderen" %}</h2>
<p>
Weet u zeker dat u deze sessie <strong>{{ session.order }}</strong> in de aanvraag <em>{{ session.study.proposal.title }}</em> wilt verwijderen?
Copy link
Contributor

Choose a reason for hiding this comment

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

Untranslated

@@ -15,11 +15,8 @@ <h2>{% trans "Taak verwijderen" %}</h2>
<p>
Weet u zeker dat u de taak <strong>{{ task.name }}</strong> uit sessie <strong>{{ task.session.order }}</strong> in de aanvraag <em>{{ task.session.study.proposal.title }}</em> wilt verwijderen?
Copy link
Contributor

Choose a reason for hiding this comment

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

Untranslated

<p>
<em>
{% blocktrans with duration=session.tasks_duration repeats=session.repeats trimmed %}
Totale brutoduur: {{ duration }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now this will show "None" instead "Onbekend" when no duration is yet known.

One way to solve this is to write a two-line template tag that takes a variable, and just replaces a None value with a translatable "Onbekend".

@EdoStorm96
Copy link
Contributor Author

@miggol I've incorporated your requests. Since we are already using createviews, your suggestion about the back buttons, was easy to implement.

The bug came down to the session_url and task_url methods in utils.py. There were some checks for completeness, which now, imo, are obsolete. Removing this fixed the bug.

Have a look at the commit history. :)

@EdoStorm96 EdoStorm96 requested a review from miggol March 25, 2024 11:08
Copy link
Contributor

@miggol miggol left a comment

Choose a reason for hiding this comment

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

This PR is getting to the point where my requests are getting more and more polish related.

The main functional issue I still see is the handling of errors in sessions/tasks and the handling of empty tasks, empty sessions, no sessions, etc.

Right now it's possible to click through start to finish without entering anything, and the trajectory will be considered "good". Although StudySubmit will still complain right at the end, which is cool. But it would be nice if there was some intermediate warning about the sessions/tasks being incomplete.

Otherwise I'm always a fan of soft validation where possible. If you want to come back to fill it in later, that's cool. But we should still have a warning.

@@ -2,6 +2,7 @@

{% load static %}
{% load i18n %}
{% load fetc_filters %}

{% block header_title %}
{% trans "Overzicht en eigen beoordeling van het gehele onderzoek" %} - {{ block.super }}
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 should read:

"Overzicht en eigen beoordeling van het gehele traject"

And use the word trajectory in English as well. To the user this is not the entire study, even though we call them studies internally.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or "Overzicht en eigen beoordeling van traject N"

{% with nav_items=study.proposal.available_urls active=3 %}
{% include 'base/navigation.html' %}
{% endwith %}
<h2>{% trans "Overzicht van alle sessies en taken" %}</h2>
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 we should say:

"Overzicht van sessies en taken in traject N"

Because if you've got multiple studies, this isn't all of them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe the trajectory could be a subtitle, like in SessionStart

<p>
<em>{% trans "Vanuit dit overzicht is het mogelijk om sessies aan te maken, te bewerken of te verwijderen." %}</em>
</p>
<div class="session_list">{% include "tasks/session_list.html" %}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

When there aren't any sessions yet, this page is awkwardly empty. I think there should at least be some filler text indicating what to do next. Or a large div that just says "No sessions yet, time to create one!"

This empty space on first visit also kind of invites the SessionStart intro text to be put there. But I'm not 100% on that idea. Perhaps we can ask Desiree for her opinion.

If we put the intro text in an {% empty %}, it of course would no longer be visible once a session was created, which is a downside.

@EdoStorm96
Copy link
Contributor Author

So @miggol I've implemented your requests. The validation now works well. Check it out :)

@EdoStorm96 EdoStorm96 requested a review from miggol March 28, 2024 14:07
Copy link
Contributor

@miggol miggol left a comment

Choose a reason for hiding this comment

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

I think the SessionOverview form is a good place for the extra validation and error processing.

Flow related

I like the new blue infobox on SessionOverview. But it's so friendly that I expect that the next button would bring me along. That is to say, bring me along to the session creation page when I haven't created a session yet.

Errors and validation

StudyEnd

The blue infobox is very friendly, but I think it could be a little less friendly once you arrive at StudyEnd. If you've still not created a session at that point, something's gone wrong and the infobox should probably resemble more of an error.

Right it just looks like an innocuous message you can just scroll past and hit "next".

No tasks

Like with sessions, we need an error when no tasks have been created. In the same way "SessionOverviewForm" is used to create the error for Sessions, SessionEndForm could be where the existence of at least one task can be checked, for the purpose of showing an error on ProposalSubmit.

Besides an error before proposal submission, SessionOverview could also use a "no tasks created" error box.

image

I'm thinking of where the tasks table is, so somewhere in tasks_list.html? As an {% else %} to can_edit_tasks perhaps?

Reflections

As much as I like soft validation, I must admit that it doesn't exactly shine here. Not having made any sessions or tasks is such an obvious fault condition that I'm starting to doubt if we should let users continue from that state.

Perhaps the situation will be improved by more obvious errors. We'll see.

@EdoStorm96 EdoStorm96 marked this pull request as draft May 28, 2024 09:08
@EdoStorm96 EdoStorm96 closed this Jun 11, 2024
@EdoStorm96 EdoStorm96 deleted the feature/multiple_sessions_tasks branch January 16, 2025 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Meerdere sessies en/of meerdere taken
3 participants