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

MLPAB-2692: Allow changing details after a failed vouch #1694

Merged

Conversation

acsauk
Copy link
Contributor

@acsauk acsauk commented Dec 30, 2024

Purpose

Fixes MLPAB-2692

Copy link

codecov bot commented Dec 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.01%. Comparing base (b40c25f) to head (33a63b4).
Report is 16 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1694   +/-   ##
=======================================
  Coverage   95.00%   95.01%           
=======================================
  Files         285      285           
  Lines       16242    16253   +11     
=======================================
+ Hits        15431    15442   +11     
  Misses        641      641           
  Partials      170      170           
Flag Coverage Δ
unittests 95.01% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -44,7 +44,7 @@ describe('what you can do now', () => {
cy.url().should('contain', '/task-list')
cy.checkA11yApp()

cy.contains('li', "Confirm your identity").should('contain', 'Completed').click();
cy.contains('li', "Confirm your identity").should('contain', 'In progress').click();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless there were changes after the original ticket I think this assertion was incorrect.

<p class="govuk-inset-text">{{ tr .App "someOfTheseDetailsCanNoLongerBeChanged" }}</p>
{{ end }}

{{ if $vouchInProgress }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added https://opgtransform.atlassian.net/browse/MLPAB-2778 to get some updated content.

@acsauk acsauk marked this pull request as ready for review January 6, 2025 15:18
@acsauk acsauk requested a review from a team as a code owner January 6, 2025 15:18
@hawx
Copy link
Contributor

hawx commented Jan 6, 2025

image

This page should allow me to edit my details. I think it would make sense to move the logic into CanChange where possible, so it is consistent.

// CanChange returns true if the donor can make changes to their LPA.
func (p *Provided) CanChange() bool {
return p.SignedAt.IsZero()
// CanChangePersonalDetails returns true if the donor can make changes to their FirstNames, LastName or DateOfBirth.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer us to keep CanChange too then, and have CanChangePersonalDetails use that instead of !p.SignedAt.IsZero. I just don't trust that the condition won't change in future, and it is a bit nicer to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point - I was on the fence as it was only checking one prop but, like you said, this could change.

@acsauk acsauk merged commit 023c903 into main Jan 7, 2025
31 checks passed
@acsauk acsauk deleted the MLPAB-2692-prevent-donor-details-update-during-pending-id branch January 7, 2025 13:51
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.

2 participants