Skip to content

Conversation

@m-salaudeen
Copy link
Contributor

Description

Context

Type of changes

  • Refactoring (non-breaking change)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would change existing functionality)
  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I am familiar with the contributing guidelines
  • I have followed the code style of the project
  • I have added tests to cover my changes
  • I have updated the documentation accordingly
  • This PR is a result of pair or mob programming

Sensitive Information Declaration

To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.

  • I confirm that neither PII/PID nor sensitive data are included in this PR and the codebase changes.

@m-salaudeen m-salaudeen self-assigned this Nov 3, 2025
@m-salaudeen m-salaudeen requested a review from a team as a code owner November 3, 2025 14:38
@m-salaudeen m-salaudeen requested a review from a team as a code owner November 13, 2025 07:49

const templateStatusToDisplayMappingsLetter = (status: TemplateStatus) =>
statusToDisplayMappings[status];
const isProofEmpty = (
Copy link
Contributor

Choose a reason for hiding this comment

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

This returns true if template.files.proofs in non-empty, right?
Should this be called isProofAvailable instead?

? templateStatusToDisplayMappingsLetter(
template.templateStatus,
isRoutingEnabled,
isProofEmpty(template)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shift right option:

You could get rid of templateStatusToDisplayMappingsDigital and templateStatusToDisplayMappingsLetter, you could rework statusToDisplayMapping like this:

It'd mean that you don't need to introduce the TEMPLATE_PROOF_APPROVED status on the open api spec and into the type system.

export const statusToDisplayMapping = (
  template: TemplateDto,
  isRoutingEnabled: boolean = false
): string => {
  const statusToDisplayMappings: Record<TemplateStatus, string> = {
    NOT_YET_SUBMITTED:
      template.templateType === 'LETTER' ? 'Not yet submitted' : 'Draft',
    SUBMITTED:
      template.templateType === 'LETTER' &&
      isRoutingEnabled &&
      isProofAvailable(template)
        ? 'Template proof approved'
        : 'Submitted',
    DELETED: '', // will not be shown in the UI
    PENDING_PROOF_REQUEST: 'Files uploaded',
    PENDING_UPLOAD: 'Checking files',
    PENDING_VALIDATION: 'Checking files',
    VALIDATION_FAILED: 'Checks failed',
    VIRUS_SCAN_FAILED: 'Checks failed',
    WAITING_FOR_PROOF: 'Waiting for proof',
    PROOF_AVAILABLE: 'Proof available',
  } as const;

  return statusToDisplayMappings[template.templateStatus];
};

VIRUS_SCAN_FAILED: 'Checks failed',
WAITING_FOR_PROOF: 'Waiting for proof',
PROOF_AVAILABLE: 'Proof available',
TEMPLATE_PROOF_APPROVED: 'Template proof approved',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
TEMPLATE_PROOF_APPROVED: 'Template proof approved',
PROOF_APPROVED: 'Template proof approved',

more in line with the others, plus I feel like "template" is implied given its a TemplateStatus?

user.clientId
);

console.log('clientConfigurationResult', clientConfigurationResult);
Copy link
Contributor

Choose a reason for hiding this comment

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

just a comment to make sure these get taken out before merge

.resolves({ Attributes: databaseTemplate });

const response = await templateRepository.submit(id, user, 0);
const response = await templateRepository.submit(id, user, 0, 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 think we should have another two tests in here now, instead of one - one from PROOF_AVAILABLE and one from PROOF_APPROVED?

channelIdentifier: 'letter',
PageModel: TemplateMgmtSubmitLetterPage,
expectedHeading: "Approve and submit 'valid-letter-submit-template'",
expectedHeading: "Approve 'valid-letter-submit-template'",
Copy link
Contributor

Choose a reason for hiding this comment

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

think we should have additional tests in here for whether the flags are enabled/disabled, using the different users?

expect(container.asFragment()).toMatchSnapshot();
});

it('should render with client proofing disabled', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('should render with client proofing disabled', () => {
it('should render with client proofing and routing disabled', () => {

':expectedLetterStatus': 'PROOF_AVAILABLE' satisfies TemplateStatus,
':expectedLetterStatus': isRoutingEnabled
? ('PROOF_APPROVED' satisfies TemplateStatus)
: ('PROOF_AVAILABLE' satisfies TemplateStatus),
Copy link
Contributor

Choose a reason for hiding this comment

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

@m-salaudeen if routing is enabled, you should never get here, right? So the expected letter status should always be PROOF_AVAILABLE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it shouldn't, but this check is only making sure the right status is there when changing to SUBMITTED.

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.

5 participants