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

Pidp 705 imms bc enrolment card #588

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

AdityaMantripragada
Copy link
Collaborator

No description provided.

@AdityaMantripragada AdityaMantripragada added the DO NOT MERGE Definitely don't merge this PR! label Aug 26, 2024
.HasGoodStanding)
&& profile.HasBCProviderCredential => StatusCode.Complete,
{ HasBCServicesCardCredential: true } => StatusCode.Incomplete,
_ => StatusCode.Locked
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please double check your logic here. I assume you based this off the ProvincialAttachmentSystemSection; but that section functions differently than all of our other ones. For PAS, we mark it as "complete" once the user has met all the requirements to enter PAS since there isn't actually a PAS enrolment for them to request. As well, this mentions endorsements which is not relevant to ImmsBC.

I would recommend you look at PrescriptionRefillEformsSection instead. For example, the section should be marked as complete when the user has an enrolment of the correct type.

@@ -108,6 +110,7 @@ public class ProfileData
public bool HasBCServicesCardCredential { get; set; }
public bool LicenceDeclarationComplete { get; set; }


Copy link
Collaborator

@james-hollinger james-hollinger Sep 16, 2024

Choose a reason for hiding this comment

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

Very minor thing but there are extra newlines in this file and others; could you please remove them?

[ProducesResponseType(StatusCodes.Status204NoContent)]
[ProducesResponseType(StatusCodes.Status400BadRequest)]
public async Task<IActionResult> CreateImmsBCEnrolment([FromServices] ICommandHandler<ImmsBC.Command, IDomainResult> handler,
[FromRoute] ImmsBC.Command command)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very small formatting request: could you please align the square brackets on this line like the other endpoints


public class ImmsBC
{
public static IdentifierType[] AllowedIdentifierTypes => [IdentifierType.PhysiciansAndSurgeons, IdentifierType.Pharmacist, IdentifierType.Nurse];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately there is a bit of domain knowledge that the JIRA ticket glossed over here; don't worry that you didn't catch it.

The ticket asked for users to have a college licence from the college of Physicians and Surgeons / Nurses / Pharmacists.
However, the college of Nurses and the College of Pharmacists actually have two Identifier Types each:
College of Nurses is both Nurse but also Midwife.
College of Pharmacists is both Pharmacist but also Pharm Tech.

It is rather difficult to find this knowledge in our code base without already knowing it; the one place we have it is in the PlrClient when we do a college licence search:
image

I believe this is the first time we have had an enrolment that includes these two Colleges so there also isn't any code you could have based it on.


if (dataObj.AlreadyEnroled
|| dataObj.Email == null
|| !await this.plrClient.GetStandingAsync(dataObj.Cpn))
Copy link
Collaborator

@james-hollinger james-hollinger Sep 16, 2024

Choose a reason for hiding this comment

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

The user not only needs to be in good standing, they need to have a good standing licence from the allowed types. You can do

  || !(await this.plrClient.GetStandingsDigestAsync(dto.Cpn))
    .With(AllowedIdentifierTypes)
    .HasGoodStanding)

Copy link
Collaborator

Choose a reason for hiding this comment

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

As well, I believe that one of the requirements is that the user has a BC Provider Credential. This requirement should be checked both here and in the ProfileStatus for this section

if (!await this.keycloakClient.AssignAccessRoles(dataObj.UserId, MohKeycloakEnrolment.ImmsBC))
{
return DomainResult.Failed();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We arn't actually assigning roles with this enrolment, just making an AccessRequest record. For now this part can be removed as well as the MohKeycloakEnrolment.ImmsBC

Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE Definitely don't merge this PR!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants