PSP-8316 | Lease consultation updates #4281

FuriousLlama · 2024-08-24T00:21:56Z

Added view and containers on the front end for consultations. Updated backend for new specifications

…backend for new specifications

github-actions · 2024-08-24T00:24:26Z

✅ No secrets were detected in the code.

github-actions · 2024-08-24T00:26:21Z

✅ No secrets were detected in the code.

… into features/psp-8316

github-actions · 2024-08-24T00:37:59Z

✅ No secrets were detected in the code.

devinleighsmith · 2024-08-26T21:52:51Z

The story, mockup, and confluence seem to have "There are no approvals or consultations." not "There are no consultations."

devinleighsmith · 2024-08-26T21:54:26Z

According to the mockup/confluence the heading should be Approvals/Consultation (although I wonder if it should be Approvals/Consultations)

devinleighsmith · 2024-08-26T21:56:28Z

Oh, and the button should be Add Approval/Consultation apparently.

I'm getting the feeling that maybe the mockups/confluence changed during development?

devinleighsmith · 2024-08-26T22:10:10Z

.../frontend/src/features/mapSideBar/lease/tabs/consultations/edit/EditConsultationYupSchema.ts

+      .max(2000, 'Other Description must be at most ${max} characters'),
+    otherwise: Yup.string().nullable(),
+  }),
+  comment: Yup.string().max(500, 'Other name must be at most ${max} characters'),


I think this text is probably not correct for this field.

devinleighsmith · 2024-08-26T22:10:38Z

.../frontend/src/features/mapSideBar/lease/tabs/consultations/edit/EditConsultationYupSchema.ts

+  otherDescription: Yup.string().when('consultationTypeCode', {
+    is: (consultationTypeCode: string) => consultationTypeCode && consultationTypeCode === 'OTHER',
+    then: Yup.string()
+      .required('Other Description required')


Perhaps: Other description?

devinleighsmith · 2024-08-26T22:16:24Z

Oddly, I don't see any documentation stating that Response received on is conditionally rendered, but that is the current logic. Is there a requirement I'm not seeing.

One issue with the current implementation is that if I set Response received Yes, and set a date, and then set response received No, the date is still set, and is displayed on the view only version of the screen, even when set to No.

devinleighsmith · 2024-08-26T22:17:58Z

@FuriousLlama what is the plan for the DB issue related to this story? Does Doug have a defect logged? my concern is that the next sprint is a release sprint, and I'm not sure Doug has made any corrections for this issue in 88.

devinleighsmith · 2024-08-26T22:21:23Z

Consultation type on the add/edit/view form should be Consultation/Approval type: or Approval/Consultation type depending on confluence or the mockup

devinleighsmith · 2024-08-26T22:22:45Z

There are a number of tooltips that were requested on the create/edit screen that appear to be missing (mentioned on the mockup and confluence).

devinleighsmith · 2024-08-26T22:24:26Z

Hmm, the confluence page specifies that the only allowed values for the Response received or Yes/No but the implementation has Unknown as well.

devinleighsmith · 2024-08-26T22:26:02Z

The text on the modal is Approval/Consultation as well.

devinleighsmith · 2024-08-26T22:42:41Z

Hmm, also I think the Consultation top-level header is being styled the same as the other headers. In the mockup I think it is using different styling (Hopefully consistent with existing PSP headers).

devinleighsmith · 2024-08-26T22:44:53Z

source/backend/api/Areas/Leases/Controllers/ConsultationController.cs

+        /// </summary>
+        /// <returns>The consultation items.</returns>
+        [HttpPost("{id:long}/consultations")]
+        [HasPermission(Permissions.LeaseView)]


devinleighsmith · 2024-08-26T22:47:24Z

source/backend/api/Areas/Leases/Controllers/ConsultationController.cs

+                User.GetUsername(),
+                DateTime.Now);
+
+            var result = _leaseService.DeleteConsultation(consultationId);


not sure if it matters, but:

the lease id is unused.

the other endpoints validate that the lease id passed is valid for the given consultationId.

Good point, I have added validation to all the routes

devinleighsmith · 2024-08-26T22:48:14Z

source/backend/api/Services/LeaseService.cs

+            return _consultationRepository.GetConsultationById(consultationId);
+        }
+
+        public PimsLeaseConsultation AddConsultation(long leaseId, PimsLeaseConsultation consultation)


unused param.

devinleighsmith · 2024-08-26T22:50:03Z

source/backend/apimodels/Models/Concepts/Lease/ConsultationLeaseMap.cs

                .Map(dest => dest.OtherDescription, src => src.OtherDescription)
+                .Map(dest => dest.RequestedOn, src => src.RequestedOn.ToNullableDateTime())


is this a DateOnly or a DateTime?

Its a datetime, however we only need dates, that's wh y the model changes it

devinleighsmith · 2024-08-26T22:51:28Z

source/backend/dal/Repositories/ConsultationRepository.cs

+
+            return Context.PimsLeaseConsultations
+                .Where(lc => lc.LeaseId == leaseId)
+                .Include(lc => lc.ConsultationTypeCodeNavigation)


Do we need the ConsultationStatusTypeCodeNavigation as well? or neither?

Consultation status is no longer used.

devinleighsmith · 2024-08-26T22:54:00Z

source/frontend/src/components/common/Section/Section.tsx

@@ -30,7 +30,7 @@ export const Section: React.FC<
  className,
  ...rest
 }) => {
-  const [isCollapsed, setIsCollapsed] = useState<boolean>(!initiallyExpanded && true);


this was a strange one.

yeah, changed it after some tests to make sure it does what we want

devinleighsmith · 2024-08-26T22:54:38Z

source/frontend/src/components/common/buttons/Button.tsx

@@ -66,7 +66,6 @@ const StyledButton = styled(BootstrapButton)`
    align-items: center;
    justify-content: center;
    padding: 0.4rem 1.2rem;
-    min-height: 3rem;


ooph, this seems scary, are you sure that this doesn't have any unintended consequences?

Pretty confident, was causing misalignment on a lot of pages

Its subtle, but this has definitely re-introduced the issue where the save and cancel buttons aren't the same size:

Could you either revert this, or find a way of sizing the cancel button (with its additional border) the same as the save button?

devinleighsmith · 2024-08-26T22:54:47Z

source/frontend/src/components/common/buttons/Button.tsx

@@ -76,7 +75,6 @@ const StyledButton = styled(BootstrapButton)`
    font-weight: 700;
    letter-spacing: 0.1rem;
    cursor: pointer;
-    height: 3.8rem;


same as above

devinleighsmith · 2024-08-26T22:58:23Z

...ce/frontend/src/features/mapSideBar/lease/tabs/consultations/detail/ConsultationListView.tsx

+  onDelete,
+}) => {
+  const keycloak = useKeycloakWrapper();
+  const history = useHistory();


See github actions warnings about unused variables.

I think you have seen an old commit?

devinleighsmith · 2024-08-26T23:00:40Z

...ce/frontend/src/features/mapSideBar/lease/tabs/consultations/detail/ConsultationListView.tsx

+  }, [consultationTypeCodes, consultations]);
+
+  if (loading) {
+    return <LoadingBackdrop show={loading} parentScreen={true} />;


You probably couldn't see this, because load times are so fast on our locals, but this is what the loading screen looks like:

My preference for these is to render an empty version of the screen with the loading backdrop overtop, but some kind of correction required here.

I think there is some room for improvement here. Will chat win ana to check what her thoughts are on loading.

devinleighsmith · 2024-08-26T23:02:16Z

...ce/frontend/src/features/mapSideBar/lease/tabs/consultations/detail/ConsultationListView.tsx

+                      <Row>
+                        <Col>
+                          {group.consultationTypeCode === 'OTHER'
+                            ? `${consultation.otherDescription} Consultation`


is null/undefined checking required here? aka ?? ''

Not needed, the view model uses empty strings if the value coming from the wire is empty

This is what I'm seeing locally:

devinleighsmith · 2024-08-26T23:05:59Z

.../frontend/src/features/mapSideBar/lease/tabs/consultations/edit/ConsultationAddContainer.tsx

+  ) => {
+    try {
+      const consultationSaved = await addConsultation(leaseId, values.toApi());
+      if (consultationSaved) {


hmm, the only issue I see is that if somehow this is false, no error is presented but also the onSuccess and navigation isn't fired.

That is true, however, that's our current pattern in other screens.

devinleighsmith · 2024-08-26T23:06:34Z

.../frontend/src/features/mapSideBar/lease/tabs/consultations/edit/ConsultationAddContainer.tsx

+      const consultationSaved = await addConsultation(leaseId, values.toApi());
+      if (consultationSaved) {
+        onSuccess();
+        history.push(backUrl);


Should this component be handling navigation or should it just be firing callbacks?

Navigation at the container makes sense to me.

devinleighsmith · 2024-08-26T23:08:58Z

source/frontend/src/features/mapSideBar/lease/tabs/consultations/edit/ConsultationEditForm.tsx

+  return (
+    initialValues && (
+      <StyledFormWrapper>
+        <Formik<ConsultationFormModel>


I think these forms could use the component as well.

Not sure what you mean here.

Oh, one of my comments must not have been saved, referring to the ConfirmNavigation component.

devinleighsmith · 2024-08-26T23:09:23Z

...ontend/src/features/mapSideBar/lease/tabs/consultations/edit/ConsultationUpdateContainer.tsx

+const ConsultationUpdateContainer: React.FunctionComponent<
+  React.PropsWithChildren<IConsultationUpdateContainerProps>
+> = ({ leaseId, consultationId, onSuccess, View }) => {
+  const [leaseConsultations, setLeaseConsultations] = useState<ApiGen_Concepts_ConsultationLease[]>(


some more unused variables detected by GH.

I think this is an earlier commit

devinleighsmith · 2024-08-26T23:13:00Z

...ontend/src/features/mapSideBar/lease/tabs/consultations/edit/ConsultationUpdateContainer.tsx

+        person = await getPerson(consultation.personId);
+      }
+      if (isValidId(consultation.organizationId)) {
+        person = await getOrganization(consultation.organizationId);


typo, should be organization not person.

devinleighsmith · 2024-08-26T23:13:58Z

...ontend/src/features/mapSideBar/lease/tabs/consultations/edit/ConsultationUpdateContainer.tsx

+  ) => {
+    try {
+      const consultationSaved = await updateConultation(leaseId, values.id, values.toApi());
+      if (consultationSaved) {


same comment here, what should happen if the object is falsey?

Same as above

devinleighsmith · 2024-08-26T23:15:14Z

source/frontend/src/features/mapSideBar/lease/tabs/consultations/edit/models.ts

+      ? fromApiOrganization(organization)
+      : undefined;
+
+    consultation.primaryContactId = apiModel.primaryContactId;


hmm, I think this is ok - just want to make sure we shouldn't be conditionally setting this based on whether the id is an org?

AFAIK only persons can be primary contacts

devinleighsmith · 2024-08-26T23:15:54Z

source/frontend/src/hooks/pims-api/useApiConsultations.ts

+    () => ({
+      getLeaseConsultationsApi: (leaseFileId: number) =>
+        api.get<ApiGen_Concepts_ConsultationLease[]>(`/leases/${leaseFileId}/consultations`),
+      getLeaseConsultationByIdApi: (leaseFileId: number, agreementId: number) =>


is agreementId correct?

devinleighsmith · 2024-08-26T23:17:20Z

source/frontend/src/hooks/repositories/useConsultationProvider.ts

+    onError: useAxiosErrorHandler('Failed to create Lease File Consultation'),
+  });
+
+  const updateLeaseConsultation = useApiRequestWrapper<


Hmm, I think you have try/catches down the line from these calls, but I think the default behaviour is to catch errors from these and use the onError provided below. If you want to throw an exception I think you need throwError: true.

thats right. I've updated it now

devinleighsmith · 2024-08-26T23:17:38Z

source/frontend/src/interfaces/IContactSearchResult.ts

@@ -54,6 +54,12 @@ export function isPersonResult(
  return contactResult.id.startsWith('P') && contactResult.personId !== undefined;
 }

+export function isOrganizationResult(


thanks for this.

devinleighsmith

Only blocking this because I'm concerned about the changes to the base button styling. If you are confident that isn't an issue, let me know.

FuriousLlama · 2024-08-26T23:32:41Z

Only blocking this because I'm concerned about the changes to the base button styling. If you are confident that isn't an issue, let me know.

Pretty confident is not an issue. That extra padding was causing alignment issues all over the app. Checked when it was added and its very early on so I don't think it was part of any button styling enhancement.

github-actions · 2024-08-27T01:02:46Z

✅ No secrets were detected in the code.

FuriousLlama · 2024-08-27T16:50:15Z

Oddly, I don't see any documentation stating that Response received on is conditionally rendered, but that is the current logic. Is there a requirement I'm not seeing.

One issue with the current implementation is that if I set Response received Yes, and set a date, and then set response received No, the date is still set, and is displayed on the view only version of the screen, even when set to No.

Yes, I infered that. I will check with dev once this hits DEV and adjust it as necessary

… into features/psp-8316

github-actions · 2024-08-27T19:34:09Z

✅ No secrets were detected in the code.

devinleighsmith · 2024-08-27T19:36:14Z

I had a look at the button styling, and this did introduce an issue, but its minor enough that I'll lift the change request. Would still prefer a solution before this is merged.

devinleighsmith

Approved, but would like some more changes before merge. Also assuming test coverage is coming in future?

github-actions · 2024-08-27T21:16:19Z

✅ No secrets were detected in the code.

FuriousLlama added 3 commits August 23, 2024 16:41

Added view and containers on the frontend for consultations. Updated …

1643178

…backend for new specifications

Cleanup

9b375ed

Regenerated snaps

cbbbe9e

FuriousLlama added the enhancement New feature or request label Aug 24, 2024

FuriousLlama self-assigned this Aug 24, 2024

Merge branch 'dev' into features/psp-8316

7a2c7b0

Merge branch 'features/psp-8316' of https://github.com/FuriousLlama/PSP…

6ea47cc

… into features/psp-8316

eddherrera self-requested a review August 26, 2024 15:40

devinleighsmith reviewed Aug 26, 2024

View reviewed changes

devinleighsmith requested changes Aug 26, 2024

View reviewed changes

asanchezr added the 5.5 label Aug 27, 2024

Merge branch 'dev' into features/psp-8316

df3bff5

FuriousLlama added 2 commits August 27, 2024 12:31

Improvements

1bacd6f

Merge branch 'features/psp-8316' of https://github.com/FuriousLlama/PSP…

c7f705f

… into features/psp-8316

FuriousLlama requested a review from devinleighsmith August 27, 2024 19:33

devinleighsmith approved these changes Aug 27, 2024

View reviewed changes

Merge branch 'dev' into features/psp-8316

bd94ed3

FuriousLlama merged commit 52137cd into bcgov:dev Aug 27, 2024
7 checks passed

github-actions bot mentioned this pull request Sep 1, 2024

Monthly pr metrics report devinleighsmith/PSP#202

Open

		.Map(dest => dest.OtherDescription, src => src.OtherDescription)
		.Map(dest => dest.RequestedOn, src => src.RequestedOn.ToNullableDateTime())

PSP-8316 | Lease consultation updates #4281

PSP-8316 | Lease consultation updates #4281

Conversation

FuriousLlama commented Aug 24, 2024 • edited Loading

github-actions bot commented Aug 24, 2024

github-actions bot commented Aug 24, 2024

github-actions bot commented Aug 24, 2024

devinleighsmith commented Aug 26, 2024

devinleighsmith commented Aug 26, 2024

devinleighsmith commented Aug 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinleighsmith commented Aug 26, 2024

devinleighsmith commented Aug 26, 2024

devinleighsmith commented Aug 26, 2024

devinleighsmith commented Aug 26, 2024

devinleighsmith commented Aug 26, 2024

devinleighsmith commented Aug 26, 2024

devinleighsmith commented Aug 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinleighsmith left a comment

Choose a reason for hiding this comment

FuriousLlama commented Aug 26, 2024

github-actions bot commented Aug 27, 2024

FuriousLlama commented Aug 27, 2024

github-actions bot commented Aug 27, 2024

devinleighsmith commented Aug 27, 2024

devinleighsmith left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 27, 2024

FuriousLlama commented Aug 24, 2024 •

edited

Loading