Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ltd 5653 desnz multiple licence conditions #2243

Merged
merged 8 commits into from
Dec 3, 2024

Conversation

Tllew
Copy link
Contributor

@Tllew Tllew commented Nov 20, 2024

Converted GiveApprovalAdviceView to a sessionwizard
Added conditional checkboxes to the licence conditions
Added a DESNZeditview to handle the prepopulation of the sessionwizard
Renamed the old views to be -legacy to move us a step closer toward this flow being the default (because of this it may be difficult to read the file changes, I'd recommend checking the commit history for a simpler view of the changes)

This is hopefully the first step to converting this whole are to a sessionwizard and removing the factories

LTD-5653

@Tllew Tllew force-pushed the LTD_5653_DESNZ_multiple_licence_conditions branch from 6e48010 to 26dafad Compare November 21, 2024 16:03
@Tllew Tllew marked this pull request as ready for review November 21, 2024 16:30
Copy link
Contributor

@currycoder currycoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really great work - I think it's almost there, I just have a bunch of (mostly minor) enhancements to suggest. The only real major is the comment about being put on the edit approval view when editing refusal advice.

caseworker/advice/conditionals.py Show resolved Hide resolved
caseworker/advice/conditionals.py Outdated Show resolved Hide resolved
caseworker/advice/forms.py Outdated Show resolved Hide resolved
caseworker/advice/forms.py Outdated Show resolved Hide resolved
caseworker/advice/forms.py Outdated Show resolved Hide resolved
caseworker/advice/templates/advice/view_my_advice.html Outdated Show resolved Hide resolved
caseworker/advice/views.py Outdated Show resolved Hide resolved
caseworker/advice/views.py Outdated Show resolved Hide resolved
caseworker/advice/views.py Outdated Show resolved Hide resolved
caseworker/advice/urls.py Outdated Show resolved Hide resolved
@Tllew Tllew force-pushed the LTD_5653_DESNZ_multiple_licence_conditions branch 2 times, most recently from 422a817 to 185de88 Compare November 25, 2024 15:14
Copy link
Contributor

@currycoder currycoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one. Just a few minor comments/re-comments which I will leave with you

caseworker/advice/views.py Outdated Show resolved Hide resolved
caseworker/advice/views.py Outdated Show resolved Hide resolved
caseworker/advice/views.py Outdated Show resolved Hide resolved
unit_tests/caseworker/advice/views/test_edit_advice.py Outdated Show resolved Hide resolved

def __init__(self, *args, **kwargs):
kwargs.pop("approval_reason")
kwargs.pop("proviso")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still wondering about this

caseworker/advice/forms.py Outdated Show resolved Hide resolved
"footnote": data["footnote_details"],
"proviso": data.get("proviso", ""),
"note": data.get("instructions_to_exporter", ""),
"footnote_required": True if data.get("footnote_details") else False,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant the field in general - footnote and footnote_required seems odd

caseworker/advice/templates/advice/view_my_advice.html Outdated Show resolved Hide resolved
@Tllew Tllew force-pushed the LTD_5653_DESNZ_multiple_licence_conditions branch 2 times, most recently from d3dc88e to 6ed6bd7 Compare November 26, 2024 10:13
@currycoder currycoder force-pushed the LTD_5653_DESNZ_multiple_licence_conditions branch from 5bead11 to 8c6fede Compare November 29, 2024 11:34
caseworker/advice/forms.py Outdated Show resolved Hide resolved
caseworker/advice/forms.py Outdated Show resolved Hide resolved
caseworker/advice/conditionals.py Outdated Show resolved Hide resolved
caseworker/advice/forms.py Outdated Show resolved Hide resolved

def __init__(self, *args, **kwargs):
kwargs.pop("approval_reason")
kwargs.pop("proviso")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm seeing this in all of the forms and it makes me wonder if there's a better way of solving this than just popping things to make them go away.

I also prefer del in cases where we are not using the returned value.

caseworker/advice/views.py Outdated Show resolved Hide resolved
unit_tests/caseworker/advice/views/test_edit_advice.py Outdated Show resolved Hide resolved
unit_tests/caseworker/advice/views/test_edit_advice.py Outdated Show resolved Hide resolved
@Tllew Tllew force-pushed the LTD_5653_DESNZ_multiple_licence_conditions branch from d5d5833 to 94b6d62 Compare December 2, 2024 16:58
@Tllew Tllew force-pushed the LTD_5653_DESNZ_multiple_licence_conditions branch from 94b6d62 to 915e82e Compare December 2, 2024 16:58
@Tllew Tllew merged commit ec6c5fd into dev Dec 3, 2024
12 checks passed
@Tllew Tllew deleted the LTD_5653_DESNZ_multiple_licence_conditions branch December 3, 2024 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants