DH-IT-Portal-Development / ethics

Ethical Committee web application in Django
http://fetc.hum.uu.nl
MIT License
2 stars 1 forks source link

Base stepper PR #689

Closed miggol closed 2 months ago

miggol commented 2 months ago

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:

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

miggol commented 2 months ago

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 commented 2 months ago

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

miggol commented 2 months ago

I was a little hasty pushing the last fixes.

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.