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

Bug/roe 2018 roe 1507 fix duplication of trust #717

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

mouhajer-ch
Copy link
Contributor

@mouhajer-ch mouhajer-ch commented Mar 3, 2023

JIRA link

Resolves ROE-1507

Change description

  • Fix duplication of trust

Work checklist

  • Tests added where applicable
  • UI changes meet accessibility criteria

@mattch1
Copy link
Contributor

mattch1 commented Mar 13, 2023

Committed a fix for:

  1. validation error message for bad json

  2. count of trusts seems to be the length of string when there is an error.

@mattch1 mattch1 closed this Mar 13, 2023
@mattch1 mattch1 reopened this Mar 13, 2023
@mattch1
Copy link
Contributor

mattch1 commented Mar 13, 2023

Latest commit is a fix for:

  1. validation error message for bad json
  2. count of trusts seems to be the length of string when there is an error.

@@ -74,7 +74,7 @@ export function checkTrustValidations(req: Request, res: Response, next: NextFun
...req.body,
beneficialOwners: getBeneficialOwnerList(appData),
trusts_input: req.body.trusts?.toString(),
trustsAddedSoFar: req.body.trusts,
trustsAddedSoFar: (appData.trusts) ? appData.trusts : [],
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that it seems to be only the number of trusts (or zero) that is required by the view, could this line be changed to:

trustsAddedSoFar: (appData.trusts) ? appData.trusts.length : 0,

And then the view logic changed accordingly?

@@ -42,6 +42,7 @@ export enum ErrorMessages {

// Trusts
TRUST_DATA_EMPTY = "Paste the trust information from the Excel document into the box",
TRUST_DATA_INVALID_FORMAT = "Format of trust information is invalid",
Copy link
Contributor

Choose a reason for hiding this comment

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

So we'll need to check the content of this message with UX or an analyst.

@@ -16,7 +16,11 @@

{% set trustAdded %}
{% if errors and trustsAddedSoFar %}
({{trustsAddedSoFar.length}} Trust information added so far)
{% if trustsAddedSoFar.length > 0 %}
({{trustsAddedSoFar.length}} Trust information added so far)
Copy link
Contributor

Choose a reason for hiding this comment

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

And this output is the other part that needs checking with UX.

@@ -33,6 +33,10 @@ export const post = async (req: Request, res: Response, next: NextFunction) => {
try {
logger.debugRequest(req, `POST ${config.TRUST_INFO_PAGE}`);

if (req.body.submit) {
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like moving the submit redirect here means that it will ignore json posted in the json field on teh page and won't add it to the model. This is different behaviour from what was there previously, it might be ok, we'll have to check with the PO tomorrow in the office. It also allows us to move to the check your answers screen with 0 trusts added, so perhaps we should only allow the user to move forward if there is at least one trust in the model/session data. Can discuss in office tomorrow.

return action === "submit" && (appData.trusts || [] ).length > 0;
};

export const checkTrustBO = (req) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think method name should be checkTrustBOs. It's called from the validator and is linked to the "beneficialOwners" field.

@mattch1
Copy link
Contributor

mattch1 commented Mar 14, 2023

Updated to hide submit button when 0 trusts are added.

@chsonarqubeprchecks
Copy link

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.

None yet

4 participants