Base stepper PR #689

miggol · 2024-08-28T13:25:33Z

Alright, this is ready for review. I don't consider everything done but I want to move on to other things and allow the rest of the team to continue development with a working stepper.

It's a bit of a beast with some rough edges. I encourage asking for docstrings, comments, and clarification during review.

Things that still need doing:

Disabled mode for stepper items, with correct classes and no link
Error collection, which should be an easy addition as all stepper items that point to a ModelForm view have the instantiated form available
Stepper bubble colour depending on errors and completeness
Collecting all errors in the Stepper class for presentation in ProposalSubmit
The Documents page, which will come with the Attachments update
Collapsing of non-active stepper subfolders
Stepper flow for non-standard Proposal types.

Thankfully, these are all smaller issues that are very doable by others now that the framework is there.

…epper

Rather than the stepper template

Wow! Somehow this just works out and results in a clean merge

EdoStorm96

Ok, Nice job! Whew, what a beast! ;p I am starting to kindoff get it. It seems like it will be quite powerful for different apps in the future!

For my review, I had a look through everything you altered and did some minor testing. It looks like everything is working as intended, therefore, I did not really go into the minutia of how stuff works (Like SessionCheck.make_stepper_item() ...) but had a pretty good look and tried to understand the design as best I could. In general I do find it a bit complicated, but I think going over these points would improve the whole thing quite a bit!

TaskMixin (or somewhere) still needs a StepperContextMixin. Otherwise TaskCreate and stuff cause an error. (minor)
A clearer separation of classes that are meant to be inhereted and classes that are meant to be instantiated. Maybe these can live in different files? Because I find it a bit messy if they are all in checkers.py. Maybe just move stuff like "Base" checkers and stepper items to the helpers file. Maybe have comments throughout this file to seperate different types of Base classes ... And group different types together.
Also Docstrings for anything intended as a baseclass would be very nice!
Lastly, I have a bit of an issue with the Layout class. I find it a bit unclear when the Stepper does things and when the Layout does things and I feel like these could be one big Stepper class? Maybe it would be to much, but now they are also very intertwined, which is indeed a bit spaghetti-ish.

See my other comments for minor things I found.

EdoStorm96 · 2024-08-28T14:18:56Z

main/templates/base/stepper.html

                </a>
-                {% if url.children %}
+                {% counter depth create 1 %}


I feel like this is not doing anything?

It prevents a bit of ugliness caused by the empty <ul />.

Note the inconsistent item spacing and "tail" at the end

EdoStorm96 · 2024-08-28T14:52:14Z

proposals/utils/stepper_helpers.py

+            classes.append(
+                "disabled",
+            )
+        return self._concat_css_classes(


Might as well do the join here imho

EdoStorm96 · 2024-08-29T08:18:41Z

proposals/utils/checkers.py

+
+class ProposalTypeChecker(
+    Checker,
+    BaseStepperComponent,


Can´t this just inherit from Checker?

EdoStorm96 · 2024-08-29T13:02:48Z

observations/views.py

@@ -53,8 +54,12 @@ def get_back_url(self):
        return reverse(next_url, args=(pk,))

    def get_study(self):
+        # Um.... what?


Haha Dunno. Let's get rid and have this mixin get the StudyURLMixin as well.

EdoStorm96 · 2024-08-29T13:08:44Z

studies/views/study_views.py



 #######################
 # CRUD actions on Study
 #######################
-class StudyUpdate(AllowErrorsOnBackbuttonMixin, UpdateView):
+class StudyUpdate(
+    StudyMixin,


So, thanks to this handy mixin, we can get rid of the context["proposal"] = self.object.proposal's in these views

Do you mean doing context["proposal"] = self.get_proposal() or moving the setting of the proposal context variable into StudyMixin??

Or something else?

EdoStorm96 · 2024-08-29T13:11:29Z

studies/views/study_views.py

@@ -23,12 +23,17 @@
 )
 from ..models import Study, Documents
 from ..utils import check_has_adults, check_necessity_required, get_study_progress
+from ..mixins import StudyMixin


Why is there no StepperContextMixin needed here? I'm confused ...

Because StudyMixin inherits from StepperContextMixin, which in turn gets imported in studies/mixins.py

EdoStorm96 · 2024-08-29T13:18:44Z

tasks/views/session_views.py

@@ -90,8 +100,17 @@ def get_back_url(self):
    def get_study(self):
        return self.object

+    def get_proposal(


Get rid and mix in StudyMixin :)

EdoStorm96 · 2024-08-29T14:00:26Z

proposals/utils/checkers.py

+
+class BaseStepperComponent:
+
+    def __init__(self, stepper, parent=None):


Maybe just make this stuff part of the Checker and get rid of BaseStepperComponent?

Hmm... I like BaseStepperComponent to keep track of what is shared between items and checkers for now.

EdoStorm96 · 2024-08-29T14:14:36Z

proposals/utils/checkers.py

+
+
+class TrajectoriesChecker(
+    Checker,


Couldn´t you just use a ModelFormChecker? And get rid of TrajectoriesItem?

Ah yes, that's better. Would be needed for checking errors in the future, too. Don't know why I chose not to

EdoStorm96 · 2024-08-29T14:17:14Z

proposals/utils/checkers.py

+        )
+        return []
+
+    def make_stepper_item(


Can we bury this somewhere deep? xp

miggol · 2024-09-02T12:12:34Z

Alright thanks for the valuable feedback!

I'm going to go through the smaller stuff first, then consider the Layout class and some of the other class hierarchies, and then document everything after.

miggol · 2024-09-03T13:12:39Z

Okay, I think I've addressed everything. Ready for another review!

Moved base classes into stepper_helpers.py with new docstrings and comments
Merged the Layout and Stepper classes. Layout came into existence when I didn't yet know what direction the classes would take, but in the end it is kind of superfluous
renderable Mixin now lives in main/utils.py
StepperItems are now also renderables
The Stepper has a more functional and less stateful build_stepper() method
All the minor comments

EdoStorm96

Alright, some really good improvement! Nice ideas and good catches. I do have some more comments.

I am having a strange problem where clicking around in the stepper, in a new proposal or older proposal, keeps bringing me to an old proposal ... A few pk's back. Somewhere along the route, the stepper is receiving the wrong proposal object, and redirecting to it. I think this has something to do with ProposalMixin. When I get to the WMOForm and further in the form, the problem doesn´t occur anymore for me.
I am not a huge fan of proposal type hint and suggest we get rid of it. It seems like one more place to keep track of this. I would suggest the following:
- During construction on major/4, the stepper would not appear until after you filled in the first form. I did not mind this design as it gives you kind off a zen start. Maybe we can bring this back? This would then, maybe also eliminate the need for the proposal_type_hint, as we could then just examine the proposal, and provide the correct layout based on that.

We can of course discuss this proposed solution :) See you tomorrow and enjoy the rest of your birthday.

EdoStorm96 · 2024-09-04T12:50:01Z

proposals/utils/stepper.py

+        The meat and potatoes of the stepper. Returns a list of top-level
+        StepperItems to be rendered in the template.
+        """
+        layout = getattr(self, "layout", False)


Does it not make sense to add layout as a required argument in the stepper's init?

I don't think so, because in almost all cases the Stepper can determine its type (and thereby layout) all by itself. The createviews are the only exception I can imagine. So if the layout was an argument it definitely should not be required.

Second, the concept of a "layout" is completely internal to the stepper implementation. I don't think every View that instantiates a stepper should need to know what a layout is and when to choose which one. The createviews know what type of proposal they're creating, because they already have to, and that should be good enough.

I think this is important because there may not exist a 1:1 mapping between proposal types and layouts. A practice proposal might use the same layout as a regular proposal, but require a different checker flow because it asks for a practice reason. Because a checker can do both of those things with just the proposal type, it makes sense to me to put that in a ProposalTypeChecker which then also determines the layout.

Adding to the above, although it isn't necessarily relevant to your comment, is that I think that the state of the stepper object should only be modified by Checkers. Once an object is complicated enough to warrant this kind of crazy depth-first search operation with Checkers, I want to respect that and not also change object state in various object methods.

Otherwise, I can imagine myself stepping through the checkers for ages trying to debug something, only to find that it was in fact that one thing I decided to put in a Stepper method instead.

EdoStorm96 · 2024-09-04T12:52:18Z

main/utils.py

+
+
+class renderable:
+


I like this living here :)

EdoStorm96 · 2024-09-04T12:53:23Z

proposals/utils/stepper_helpers.py

+
+class StepperItem(
+    renderable,
+):


Good idea to make this renderable! Streamlines the operation a lot.

EdoStorm96 · 2024-09-04T13:35:48Z

proposals/utils/stepper.py

+                continue
+            # Remaining empty slots are replaced by placeholders
+            placeholder = PlaceholderItem(
+                self.stepper,


I guess this should be self? This caused an attribute error for me.

EdoStorm96 · 2024-09-04T13:46:50Z

proposals/utils/stepper_helpers.py

+
+    def __init__(self, *args, **kwargs):
+        if not self.form_class:
+            raise ImproperlyConfigured("form_class must be defined")


You forgot to import ImproperlyConfigured

miggol · 2024-09-09T09:56:46Z

I was a little hasty pushing the last fixes.

Imports were still in the wrong place
Incorrect references
Super weird stuff going on caused by #4b8b130

But I think everything is fixed now. Thanks for finding the above.

I am having a strange problem where clicking around in the stepper, in a new proposal or older proposal, keeps bringing me to an old proposal ... A few pk's back. Somewhere along the route, the stepper is receiving the wrong proposal object, and redirecting to it. I think this has something to do with ProposalMixin. When I get to the WMOForm and further in the form, the problem doesn´t occur anymore for me.

I think this was caused by #4b8b130 and is now fixed, but please double check if it's still happening.

I am not a huge fan of proposal type hint and suggest we get rid of it. It seems like one more place to keep track of this. I would suggest the following:

During construction on major/4, the stepper would not appear until after you filled in the first form. I did not mind this design as it gives you kind off a zen start. Maybe we can bring this back? This would then, maybe also eliminate the need for the proposal_type_hint, as we could then just examine the proposal, and provide the correct layout based on that.

Although I posted a lengthy motivation above about why it currently is the way it is, I think removing the stepper from the createviews altogether is a totally cool idea that would save us some complexity, including the type hint. The reason I haven't done this is that it would be extra work and debugging right now (just before leaving for the UK), whereas the current implementation at least works.

And if it turns out there's something we hadn't thought about with the alternate proposal types that does require some kind of proposal type hint, well at least it's already there.

Feel free to not include the stepper in the createviews if that saves you some time, should you decide to work on the alternate proposal flows.

EdoStorm96

Nice that you found the fix! It looks good for now, I´d say :) Nice work!

miggol added 30 commits May 6, 2024 11:22

Merge remote-tracking branch 'origin/update/django_dsc' into major/st…

9a049f5

…epper

wip: Initial stepper code

4d519cd

feat: Base template now includes stepper object

534e2ea

Rather than the stepper template

feat: Add stepper object to ProposalMixin context

be202d3

feat: New StepperContextMixin

66c96d4

wip: Checker class

eee57be

Merge remote-tracking branch 'origin/major/4' into major/stepper

7641d8e

feat: more rigourously define stepper and proposal in context

b93fcd5

feat: Stepper Layout class

4153ce7

feat: Starting checkers, remove Pages, some renamed variables

176b956

fix: renamed variables in template

f2fbcda

fix: correctly define stepper with proposal in views

a27ef68

wip: Move checkers and helpers into separate files

2434dbc

feat: Provide request object to Stepper object

c8337c0

breaking: Repurpose ProposalMixin for views with a proposal main object

5f87c14

feat: Provide Layout and PlaceholderItems access to stepper object

e99a6f9

feat: Provide current checking and associated css class

55fc57e

wip: Combine checkers and stepper items into one

842f0df

wip: Merging stepper items and checkers was a bad idea

b0934c0

fix: Refer to self.get_proposal() instead of self.object

6c46870

wip: Adding more checkers up to and including WMO

60cd6f7

wip: Adding studies stepper item

46b2d0d

Merge branch 'fix/small-django4-fixes' into major/stepper

9134c9e

Wow! Somehow this just works out and results in a clean merge

feat: Add appropriate stepper context mixins to views

03d93ba

feat: Recursive stepper includes

8d70e9c

feat: Provide stepper bubble css classes in context

6799ed6

feat: Stepper items added until StudyDesign

a7ad91d

feat: Make proposal getting consistent in various views

e95a5d0

wip: UpdateOrCreateChecker

2390613

fix: get_proposal for SessionMixin

cd9c8ed

miggol added 4 commits August 28, 2024 14:56

feat: StudyEndChecker

d21bf46

fix: Correct context inheritance, thanks Ty

1d790d8

Merge remote-tracking branch 'origin/major/4' into major/stepper

024c81c

style: Paint everything black

84e6936

miggol requested a review from EdoStorm96 August 28, 2024 13:25

EdoStorm96 requested changes Aug 29, 2024

View reviewed changes

miggol added 13 commits September 2, 2024 15:23

pr: Remove unnecessary BaseStepperComponent

12de201

pr: Just return the css classes

ddce7ec

pr: Tidy up proposal getting and mixins

a92b2d2

pr: Remove TrajectoriesItem and just use an MFC

902f9cb

fix: Stepper in Task views and session_set fix

12aa53e

feat: Move renderable mixin to main/utils.py

4ba78df

feat: Make StepperItems renderable items

ed54927

feat: Merge Layout and Stepper classes, simplify stepper building

e5cb6df

feat: Move some of the remaining items around

b7aae56

fix: Provide location to TrajectoriesChecker

3104d3d

feat: Move all base classes to stepper_helpers.py

127abe0

docs: Added docstrings and comments to stepper helpers

afaa5f2

style: Lick of black paint

8be1a57

miggol requested a review from EdoStorm96 September 3, 2024 13:20

EdoStorm96 requested changes Sep 4, 2024

View reviewed changes

miggol added 3 commits September 9, 2024 11:37

fix: Imports that were left behind

a930845

fix: Incorrect reference to stepper

123dc5c

fix: Misbehaviour caused by mutation of RegularProposalLayout

4b8b130

miggol requested a review from EdoStorm96 September 9, 2024 10:00

EdoStorm96 approved these changes Sep 9, 2024

View reviewed changes

miggol merged commit f68d2ca into major/4 Sep 9, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Base stepper PR #689

Base stepper PR #689

miggol commented Aug 28, 2024 •

edited

Loading

EdoStorm96 left a comment

EdoStorm96 Aug 28, 2024

miggol Sep 2, 2024

EdoStorm96 Aug 28, 2024

EdoStorm96 Aug 29, 2024

EdoStorm96 Aug 29, 2024

EdoStorm96 Aug 29, 2024

miggol Sep 2, 2024

EdoStorm96 Aug 29, 2024

miggol Sep 2, 2024

EdoStorm96 Aug 29, 2024

EdoStorm96 Aug 29, 2024

miggol Sep 3, 2024

EdoStorm96 Aug 29, 2024

miggol Sep 2, 2024

EdoStorm96 Aug 29, 2024

miggol commented Sep 2, 2024

miggol commented Sep 3, 2024 •

edited

Loading

EdoStorm96 left a comment

EdoStorm96 Sep 4, 2024

miggol Sep 9, 2024

EdoStorm96 Sep 4, 2024

EdoStorm96 Sep 4, 2024

EdoStorm96 Sep 4, 2024

EdoStorm96 Sep 4, 2024

miggol commented Sep 9, 2024 •

edited

Loading

EdoStorm96 left a comment


		class BaseStepperComponent:

		def __init__(self, stepper, parent=None):

Base stepper PR #689

Base stepper PR #689

Conversation

miggol commented Aug 28, 2024 • edited Loading

EdoStorm96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miggol commented Sep 2, 2024

miggol commented Sep 3, 2024 • edited Loading

EdoStorm96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miggol commented Sep 9, 2024 • edited Loading

EdoStorm96 left a comment

Choose a reason for hiding this comment

miggol commented Aug 28, 2024 •

edited

Loading

miggol commented Sep 3, 2024 •

edited

Loading

miggol commented Sep 9, 2024 •

edited

Loading