Feature/stepper refinement #700
Conversation
There was a problem hiding this comment.
You got a lot done with this PR! We still need to double check that all stepper items actually add errors, for example the "Financing" modelform, but that is more of an issue with the forms.
Also, this smaller part of #692 is not addressed:
- Remove the
<a />tags from disabled stepper items
| @@ -7,6 +7,14 @@ | |||
| {% trans "Algemene informatie over de aanvraag" %} - {{ block.super }} | |||
| {% endblock %} | |||
|
|
|||
| {% block stepper %} | |||
| {% if create %} | |||
There was a problem hiding this comment.
A simple if stepper would be better IMO.
proposals/utils/stepper_helpers.py
Outdated
| @@ -129,6 +129,14 @@ def is_current(self, request): | |||
| """ | |||
| return False | |||
|
|
|||
| def css_classes(self): | |||
| css_classes = super().css_classes() | |||
| if self.children[0].get_errors(): | |||
There was a problem hiding this comment.
This only checks the first child for errors:
Because this trajectory still has some errors in it, I think it would make more sense to do something like:
child_errors = []
for child in self.children:
child_errors += child.get_errors()
if child_errors != []:
css_classes += "incomplete"
breakSo that the parent still turns yellow in this case.
But we could also ask the secretary for a preference. Because the parents are forms themselves it might make sense to only have them turn yellow for children if they're collapsed i.e. their children are hidden.
proposals/utils/validate_proposal.py
Outdated
| { | ||
| "url": reverse("tasks:session_end", args=[session.pk]), | ||
| "page_name": _("{}: sessie {} overzicht").format( | ||
| study.name, |
There was a problem hiding this comment.
In the case that a proposal only has one trajectory, there is no study name.
proposals/utils/validate_proposal.py
Outdated
| if hasattr(item, "form_class") and item.form_class == SessionOverviewForm: | ||
| troublesome_pages.extend(validate_sessions_tasks(item.study)) | ||
| if item.get_errors(): | ||
| if item.parent and item.parent.title in study_titles: |
There was a problem hiding this comment.
If you try really hard and call your trajectory something like "Basisgegevens" you can get this if-statement to go down the wrong path, lol.
proposals/utils/validate_proposal.py
Outdated
| ), | ||
| None, | ||
| ) | ||
| def validate_sessions_tasks(study): |
There was a problem hiding this comment.
I would really really prefer if we could somehow have this be part of the stepper/checker infrastructure, so that all we had to do was run a stepper.get_all_errors() to get list of errors with titles and URLs.
For example, calling this code from the SessionsChecker. Or otherwise a ModelFormItem for each of the task pages that is hidden but does collect errors.
That would keep everything in one place and allow us to turn the Sessions item yellow if its subpages had errors.
|
Ok, So I've adressed your comments ... I guess this is fine for now, although I don´t love some aspects of the current design ... Mainly the validation of sessions and tasks ;p A small overview of what I've done:
IMO this is fine for now though. Curious to hear your thoughts 👍 |
So I went a bit ham ... I think I've made some nice improvements to the stepper. It seems to work well, but I'm curious to hear your thoughts, @miggol.
I am now a lot more intimate with the new stepper, and also more appreciative of its utility. Kudos again :)
So, what I've done:
clean()'s of WMO related forms as these seem kindoff incomplete ...get_form_kwargs()method.Sorry for the big PR ... One thing led to another and such. But overall I think this has improved the stepper quite a bit!