Refactor: class-based view for Eligibility Index - #3065
Conversation
f62683f to
4e7347d
Compare
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
lalver1
left a comment
There was a problem hiding this comment.
Thanks for highlighting how this PR differs from previous ones in that it moves the context into the CBV as we set out initially, and thanks for this reminder 🙂
I tend to prefer this approach since the context and the view are now in the same place. If we need to update or add copy later, it seems that this setup makes it easier to find it since the context is closer to the view (and consequently the html via template_name). The code inside the views does get longer, but maybe that's ok?
Just noticed a minor typo, unrelated to the open question, in the comment below
| flow_id = form.cleaned_data.get("flow") | ||
| flow = EnrollmentFlow.objects.get(id=flow_id) | ||
| session.update(request, flow=flow) | ||
| eligibliity_index = { |
There was a problem hiding this comment.
Minor typo (eligibliity) here and in the context.update() line below
4e7347d to
8264b2f
Compare
|
RE: your open question @angela-tran I'm glad you decided to just do this here. I was kind of avoiding it because it seemed like another layer of things to think about (OK OK, I was lazy 😅). But I agree with you and Luis, it makes a lot of sense to keep things self-contained in the CBVs. Originally originally, we even had the idea to have a module (file) for each CBV. As we move context data into the views, we may want to go back to that idea to make for more reasonable and readable file sizes. |
786a38a to
c864006
Compare
|
@angela-tran you may want to rebase on the latest |
these were testing that the view function used certain templates and forms. with the CBV, these are now just values set on the view.
one of the main motivations for the CBV refactor is to consolidate and localize the data needed for views.
c864006 to
0b21a62
Compare
|
Thanks @lalver1 and @thekaveman for adding your thoughts. It makes sense that there was hesitation around moving the context over since clearly I had hesitation too about what questions it would potentially open up. Glad to hear this is still the direction we want to go. |
lalver1
left a comment
There was a problem hiding this comment.
This looks great @angela-tran! 🎉
Closes #2868
This PR converts the eligibility
indexview function into a class-based view.The corresponding copy context for the Eligibility Index was moved into the CBV's
get_context_datamethod.Open question
This PR is all working and technically ready to merge, but I wanted to call something out for discussion and to get ahead of this PR possibly having a "surprising" effect 😣 ...
In our other CBV refactors, I haven't seen us move the copy context objects into the CBV, but I thought this was one of the main reasons for moving to CBVs (#2777). I'm trying my best to help us achieve what we set out to do but am feeling like this PR might cause controversy because it's deviating from the norm.