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

feat(DTFS2-7498): fetch amendments eligibility criteria from db #4155

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

BethThomas141
Copy link
Contributor

Introduction ✏️

Include a summary of the changes and related feature(s) or issue(s).

Resolution ✔️

List all changes made to the codebase.

Miscellaneous ➕

List any additional fixes or improvements.

Comment on lines +18 to +19
.find({ $and: [{ facilityType }, { isInDraft: { $eq: false } }] })
.sort({ version: -1 })
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this one of those magic mongodb things where it filters for a content within the array?

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 $elemMatch does this if its not doing magic

Copy link
Contributor

Choose a reason for hiding this comment

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

This is really nice though!

Comment on lines +18 to +19
.find({ $and: [{ facilityType }, { isInDraft: { $eq: false } }] })
.sort({ version: -1 })
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 $elemMatch does this if its not doing magic

Comment on lines +18 to +19
.find({ $and: [{ facilityType }, { isInDraft: { $eq: false } }] })
.sort({ version: -1 })
Copy link
Contributor

Choose a reason for hiding this comment

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

This is really nice though!

@@ -39,6 +41,12 @@ export class PortalFacilityAmendmentService {
throw new InvalidAuditDetailsError(`Supplied auditDetails 'id' ${auditDetails.id.toString()} does not correspond to a valid user`);
}

const { type: facilityType } = await findOneFacility(facilityId);

const { version, criteria } = await EligibilityCriteriaAmendmentsRepo.findLatestEligibilityCriteria(facilityType);
Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think version make sense at a glance - it probably should be eligibilityCriteriaVersion or similar

}),
),
})
.optional(),
Copy link
Contributor

Choose a reason for hiding this comment

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

should this not be optional?

}),
},
{
description: 'eligibility is missing id',
Copy link
Contributor

Choose a reason for hiding this comment

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

Think these need updating to criteria

@@ -41,12 +41,18 @@ export const getEligibility = async (req: GetEligibilityRequest, res: Response)
return res.redirect('/not-found');
}

// TODO 7765: Fetch criteria using GET endpoint from database and add to viewModel
if (!amendment.eligibilityCriteria) {
Copy link
Contributor

Choose a reason for hiding this comment

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

surprised that this is optional

Comment on lines +123 to +128
export type AmendmentsEligibilityCriterion = {
id: number;
text: string;
textList?: string[];
answer: boolean | null;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this extend the db type - confused me a bit but happy to leave

const exception = new EligibilityCriteriaNotFoundError();

// Assert
expect(exception.message).toEqual('Latest eligibility criteria not found');
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this always be latest - think its fine to leave tbh

import { ApiError } from './api.error';
import { EligibilityCriteriaNotFoundError } from './eligibility-criteria-not-found.error';

describe('EligibilityCriteriaNotFoundError', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you check the status of the error

isInDraft: boolean;
createdAt: UnixTimestampMilliseconds;
criteria: EligibilityCriterion[];
auditRecord?: AuditDatabaseRecord;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unsure if we need auditRecords - if these are inputted manually into the database then they wont be needed. If we're expecting them then Abhi should create it with type: none

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