Ltd 5714 other ogd sessionwizard #2263

Tllew · 2024-12-05T16:50:54Z

replaced is_desnz with is_ogd team to ensure fcdo can't access the site manually
only sending to legacy view when fcdo team

Added a constant to WebDriverDelay due to frustration, leaving it in so we have a set place to modify this delay incase someone else gets as frustrated as I did with the tests

LTD-5714

currycoder

A few minor suggestions throughout to clean this up a bit. Otherwise looking great

currycoder · 2024-12-16T13:53:10Z

caseworker/advice/forms/approval.py

@@ -56,7 +56,7 @@ class Layout:
        TITLE = "Recommend an approval"

    approval_reasons = forms.CharField(
-        widget=forms.Textarea(attrs={"rows": 7, "class": "govuk-!-margin-top-4"}),
+        widget=forms.Textarea(attrs={"rows": 7, "class": "govuk-!-margin-top-4", "name": "approval_reasons"}),


I'm surprised this "name": "approval_reasons" is necessary. My understanding is that django injects that for you when this is rendered in the template.

I agree. If it's added for a particular reason could we get a comment as to why?

I believe this was me trying to force remove the form index from the approval reason, as the name here is formatted: formname-approval_reasons

ultimately this didn't work and I needed to change the ui tests to point to "recommend_approval-approval_reasons"

this is something the formwizard inserts so that you can have multiple fields with the same name in a single formwizard.

(removed now)

currycoder · 2024-12-16T13:55:50Z

caseworker/advice/templates/advice/view_my_advice.html


                {% if buttons.edit_recommendation %}
-                  {% if can_user_make_desnz_edit %}
+                  {% if can_user_make_edit %}


I hadn't noticed before that there's sort of two parts to this permission; 1) can the user make the new edit expressed with this new django rule, 2) can the user make an edit at all, expressed with buttons.edit_recommendation

I feel like it would be good to get away from buttons.edit_recommendation if we can and get to the django rules governing this completely.

yeah this needs to be converted to use django rules everywhere.
get_advice_tab_context() decides whether we show or hide button depending on current user etc and in template we use rules on top of that. We need to make changes in multiple places.

I've looked into this a little, I do agree we could remove the if buttons. logic however there's some additional logic behind the edit_recommendation that I'm not confident we'd cover with the logic we have in the template, it seems like it could be a little out of scope at this stage in the LCDC project

currycoder · 2024-12-16T13:58:11Z

caseworker/advice/views/approval.py

+    form_class = SelectAdviceForm
+
+    def get_success_url(self):
+        recommendation = self.request.POST.get("recommendation")


Would recommend getting this from the form's cleaned_data rather by setting it as an instance variable in form_valid(). We are otherwise getting the raw value rather than the cleaned one

currycoder · 2024-12-16T13:59:24Z

caseworker/advice/views/approval.py

@@ -28,7 +47,7 @@ class GiveApprovalAdviceView(LoginRequiredMixin, CaseContextMixin, BaseSessionWi
    ]

    condition_dict = {
-        AdviceSteps.RECOMMEND_APPROVAL: C(is_desnz_team),
+        AdviceSteps.RECOMMEND_APPROVAL: C(is_ogd_team),


I still don't get why this step is conditional. Once you're in the new flow, surely this step must show. We are handling whether or not to take users to the new or legacy flow outside of this view

this is still excluding fcdo to ensure if you were to manually navigate to the page, you'd see an error.

kevincarrogan · 2024-12-27T10:17:57Z

caseworker/advice/conditionals.py

+def is_ogd_team(wizard):
+    return not is_fcdo_team(wizard)


I feel this is named a bit confusingly as FCDO is an OGD and if I were reading this at the point of use this wouldn't be obvious to me.

because we're still excluding fcdo in this pr, i've changed the logic to be : ~C(is_fcdo_team)
and removed is_ogd_team to make this more obvious

kevincarrogan · 2024-12-27T10:18:19Z

caseworker/advice/forms/approval.py

@@ -56,7 +56,7 @@ class Layout:
        TITLE = "Recommend an approval"

    approval_reasons = forms.CharField(
-        widget=forms.Textarea(attrs={"rows": 7, "class": "govuk-!-margin-top-4"}),
+        widget=forms.Textarea(attrs={"rows": 7, "class": "govuk-!-margin-top-4", "name": "approval_reasons"}),


I agree. If it's added for a particular reason could we get a comment as to why?

kevincarrogan · 2024-12-27T10:18:39Z

caseworker/advice/rules.py

-def can_desnz_make_edit(team):
-    return team in services.DESNZ_TEAMS
+def can_ogd_make_edit(team):
+    return not team == services.FCDO_TEAM


!= would probably be better here.

kevincarrogan · 2024-12-27T10:23:55Z

tests_common/constants.py

+class WebDriverDelay:
+    THIRTY = 30
+    FORTYFIVE = 45
+    TWENTY = 20
+    SIXTY = 60


I presume that you're setting these so you can change them locally to higher values?

You could always set these as env variables instead (maybe with names that aren't literally the number) and then anyone could more easily add these and do it more permanently.

The only other thing I would say though is that I would double check whether something else is going on that is causing you to get so many failures locally with timeouts. It could be a symptom of something changing that is making pages load slowly that might be worth proactively fixing.

This was written and added in frustration, an env variable is a nice idea, I kept it in so that if someone else was to hit this and for some reason try to modify the delay then they wouldn't need to manually update each instance of the webdriverwait

Brendan helped me resolve the slow tests in the end, in the end it was:

CELERY_ALWAYS_EAGER=True

That resolved the failing and hanging tests.

I'm happy to either:

remove this as a constant

convert it to an env variable

which would you prefer?

…folder structure

Tllew force-pushed the LTD-5706-approve-session-wizard branch 2 times, most recently from 3abfcc1 to fec9881 Compare December 9, 2024 12:16

Tllew force-pushed the LTD-5714-other-ogd-sessionwizard branch from 38f2d86 to b493985 Compare December 9, 2024 15:30

Base automatically changed from LTD-5706-approve-session-wizard to dev December 10, 2024 09:40

Tllew force-pushed the LTD-5714-other-ogd-sessionwizard branch 5 times, most recently from a0a4030 to 80c4dcc Compare December 12, 2024 13:47

Tllew marked this pull request as ready for review December 12, 2024 14:01

currycoder reviewed Dec 16, 2024

View reviewed changes

kevincarrogan reviewed Dec 27, 2024

View reviewed changes

Tllew added 7 commits January 6, 2025 09:57

minor changes to basesessionwizard and splitting up forms/views into …

5813a13

…folder structure

removed unused code

84e101e

initial commit

150bfe2

all non fcdo ogds using multi_step

f925a11

changes to rules

7a0fbc1

resolve conflicts

563b609

ui tests complete

775b745

Tllew force-pushed the LTD-5714-other-ogd-sessionwizard branch from 083c5c4 to 775b745 Compare January 6, 2025 09:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ltd 5714 other ogd sessionwizard #2263

Ltd 5714 other ogd sessionwizard #2263

Tllew commented Dec 5, 2024 •

edited

Loading

currycoder left a comment

currycoder Dec 16, 2024

kevincarrogan Dec 27, 2024

Tllew Jan 6, 2025 •

edited

Loading

currycoder Dec 16, 2024

saruniitr Dec 18, 2024

Tllew Jan 6, 2025

currycoder Dec 16, 2024

currycoder Dec 16, 2024

Tllew Jan 6, 2025

kevincarrogan Dec 27, 2024

Tllew Jan 6, 2025

kevincarrogan Dec 27, 2024

kevincarrogan Dec 27, 2024

kevincarrogan Dec 27, 2024

Tllew Jan 6, 2025

Ltd 5714 other ogd sessionwizard #2263

Are you sure you want to change the base?

Ltd 5714 other ogd sessionwizard #2263

Conversation

Tllew commented Dec 5, 2024 • edited Loading

currycoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tllew Jan 6, 2025 • edited Loading

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

Tllew commented Dec 5, 2024 •

edited

Loading

Tllew Jan 6, 2025 •

edited

Loading