Closed EdoStorm96 closed 5 months ago
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.
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 ;) )
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:
start_review
and some blocking conditions that were accidentally left out. I restored themFirstly, 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:
get_next_url()
functions to determine where to go next
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.
Ok, so here is a new version of this PR. It did indeed get a bit unwieldy again, as Ty warned me ... But I think it is looking nice!
So this is what I demo'd last week, with the changes requested during that demo. I've also incorporated all feedback from this PR. I am still waiting for an improved text for session_start.html
from Desiree, but that can also be done later. Now, this text is mostly the same as it was, so this is fine for now, imo.
Most commits are pretty manageable, except for f40ceca ... I am sorry about that, things were a bit to intertwined. This was a large overhaul of the flow ... However, one thing to pay attention to in this commit, is that I added a template tag for creating a hacky unordered html list, which I added in fetc_filters.py
: create_unorderded_html_list()
. This was based on code used in pdf_diff_logic.py
. So now we can officially close #18 right? ;p
Sidenote: It was at this point in writing this comment I realised I should probably replace the function in pdf_diff_logic.py
, with the new function, which I did. This works fine. Upon testing, I found a bug in create_context_diff()
, due to an if-statement, which should have been an if-not ... I fixed this too. See the last commit. It's ironic that my previous monster PR has come back to haunt this one ...
Otherwise, I think the commit flow, is kindoff doable ... Good luck with the review. I am looking forward to hearing your thoughts :)
I haven't looked at the updates, but I wanted to react to two things in your update comment:
I am still waiting for an improved text for session_start.html from Desiree, but that can also be done later.
I agree, if the merging of this PR is going to be held till that point. I'd like to avoid non-ready code changes to develop atm, as we're trying to create a release.... (Also, there's something to be said for leaving this change for a follow-up release, as the currently staged changes are already extensive)
EDIT: no this PR has a critical bugfix that we need for the next release... In hindsight, I should've fixed that bug in a different PR....
Otherwise, I think the commit flow, is kindoff doable ... Good luck with the review.
Hahahahahahahahahahahahahahaha
What I think still needs work is the final overview page. It would be nice if interventions and observations followed the same design language. It's a bit out of scope for this PR perhaps, but I would rather get this right in one go.
Mmmm, Yes I agree. I'll look into it.
I don't know why we use a RedirectActionView for creating new tasks and sessions. Is this related to the previous setup? Is it still necessary? I think it would be nicer if these were just CreateViews, only creating the object once you hit save.
Now that I'm thinking about it, I guess this should be possible? I'll look into it as well.
Back buttons do not always work or make sense. When creating a new task, the back button now goes to the general session info. I think it should go to the overview. It could also be changed to a javascript browser back.
Here I just found it a bit weird to make forward and back redirect to the same page ... but why not. It was my initial intuition as well.
The back button on SessionEnd 404's. woops. On it.
@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.
@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. :)
So @miggol I've implemented your requests. The validation now works well. Check it out :)
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()
SessionStart
view, as that was just used for giving the initialsessions_number
and is now longer needed. Its help text is now on theSessionUpdate
view, which is (from the user POV) the first page you see when creating session.session.net_duration
is now a bit more spicy, as I needed to multiplytask.repeats
bytask.duration
and then sum that for all tasks.StudyEnd
view. Although this leads to a weird UI quirk for me of like an_
appearing between the items?form_buttons.html
to accomodate for a "Save and create a new session/task >>" option.SessionOverviewSection
, as that was the equivalent of theSessionStart
view and just showed the sessions number. To not lose its section title, I put some logic in theSessionSection
to display its title,if session.order == 1
.Some last bugs/things to note/ideas:
UpdateView
is rendered afterSessionCreate
orTaskCreate
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?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