psp-8630 | Updated Leases and added renewals #4180

FuriousLlama · 2024-07-10T19:48:40Z

Note: Backend tets will be added after verifying with one of our BAs in a live environment

…tion

github-actions · 2024-07-10T20:03:43Z

✅ No secrets were detected in the code.

devinleighsmith · 2024-07-10T23:19:10Z

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

+        }
+
+        /// <summary>
+        /// Update the specified tenants on the passed lease.


Hmm, what is this endpoint doing in this controller? this seems specific to tenants.

Thought I might need an update for the renewals, so left a stub. Its gone now

devinleighsmith · 2024-07-10T23:22:07Z

source/backend/api/Services/LeaseService.cs

+            _logger.LogInformation("Getting renewals on lease {leaseId}", leaseId);
+            _user.ThrowIfNotAuthorized(Permissions.LeaseView);
+            var pimsUser = _userRepository.GetByKeycloakUserId(_user.GetUserKey());
+            pimsUser.ThrowInvalidAccessToLeaseFile(_leaseRepository.GetNoTracking(leaseId).RegionCode);


what does this do for a non-existent lease id?

If the lease ID matches nothing it will return an empty collection

devinleighsmith · 2024-07-10T23:26:13Z

source/backend/api/Services/LeaseService.cs

+                var agreementStart = lease.OrigStartDate.Value;
+                var agreementEnd = lease.OrigExpiryDate.Value;
+                currentEndDate = agreementEnd;
+                if (DateTime.Compare(agreementEnd, agreementStart) <= 0)


FYI C# supports comparing DateTimes with the regular < <= > >= operators.

devinleighsmith · 2024-07-10T23:33:10Z

source/backend/api/Services/LeaseService.cs

+
+                if (DateTime.Compare(currentEndDate, startDate) >= 0)
+                {
+                    throw new BusinessRuleViolationException("The commencement date of your renewal should be later than the previous expiry date (agreement or renewal)");


I feel like this satisfies the ac in abstract, but not on a 1-1 basis. Make sure there is a not on the story for Praveen so he doesn't fail this.

Also, should the user be able to allow colliding renewal dates when the renewal is not exercised? currently I can.

I had a conversation with julian about this. A non-exercised renewal is functionally a draft, so no validation happens for those

devinleighsmith · 2024-07-10T23:35:18Z

source/backend/dal/Repositories/Interfaces/ILeaseRenewalRepository.cs

+namespace Pims.Dal.Repositories
+{
+    /// <summary>
+    /// ILeaseRenewalRepository interface, provides functions to interact with lease tenants within the datasource.


nit: lease tenants.

devinleighsmith · 2024-07-10T23:41:21Z

source/frontend/src/features/leases/add/LeaseDetailSubForm.tsx

-          <SectionField label="Expiry date">
+          <SectionField
+            label="Expiry"
+            required={statusTypeCode === ApiGen_CodeTypes_LeaseStatusTypes.ACTIVE}


I think this field is never required - this rule only applies to the commencement date on the agreement.

This was part of the conversation with Julian. Active leases must have a commencement and expiry dates

devinleighsmith · 2024-07-10T23:53:36Z

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

+      async (leaseId: number) => await getLeaseRenewals(leaseId),
+      [getLeaseRenewals],
+    ),
+    requestName: 'getApiLeaseChecklist',


needs updated name, and the error needs updating as well.

devinleighsmith · 2024-07-11T00:04:16Z

source/backend/api/Services/LeaseService.cs

+            }
+            else
+            {
+                throw new BusinessRuleViolationException("Active leases must have commencement and expiry dates");


is this true? I think they just need commencement dates.

Yeah, confirmed by julian. Check above reply

devinleighsmith · 2024-07-11T00:06:04Z

I think commencement date and expiry date for the agreement or supposed to have tooltips (based on confluence).

devinleighsmith · 2024-07-11T00:07:37Z

source/frontend/src/features/leases/detail/LeasePages/details/LeaseDetailView.tsx

+      {lease.fileStatusTypeCode.id === ApiGen_CodeTypes_LeaseStatusTypes.TERMINATED && (
+        <>
+          <SectionField label="Termination" labelWidth="3">
+            {lease.terminationDate}


I think this should probably be pretty formatted.

I thought I had it that way. Might have been part of the merge

devinleighsmith · 2024-07-11T00:10:32Z

source/frontend/src/features/leases/detail/LeasePages/details/LeaseDetailView.tsx

+      </SectionField>
+      <Row>
+        <Col>
+          <SectionField label="Commencement" labelWidth="6">


I guess I'm assuming that both view and edit should have the tooltips for the commencement, expiry, and termination?

It was unclear to me. I can add them later once the BAs see the section live

devinleighsmith · 2024-07-11T00:16:26Z

source/frontend/src/features/leases/add/RenewalSubForm.tsx

+                    <Row>
+                      <Col xs="7">
+                        <SectionField
+                          label="Commencement"


tooltips missing for commencement, expiry on read/edit.

Pretty odd. I remember adding them. Anyhow, they are there now.

devinleighsmith · 2024-07-11T00:18:08Z

It looks like the formik yup schema was not updated for the new business rules. So, for example, commencement/expiry date are listed as required for exercised renewals visually, but I can still save an exercised renewal without providing any dates.

devinleighsmith · 2024-07-11T00:22:22Z

source/backend/api/Services/LeaseService.cs

+
+        private void ValidateRenewalDates(PimsLease lease, ICollection<PimsLeaseRenewal> renewals)
+        {
+            if (lease.LeaseStatusTypeCode != PimsLeaseStatusTypes.ACTIVE)


Is this accurate? it seems like dates are optional when a lease is not active, but should the user be allowed to enter overlapping dates when not active?

Thats correct. Check reply above.

devinleighsmith · 2024-07-11T00:31:12Z

This modal message doesn't match jira:
The lease is no longer in Terminated state. The reason for doing so will be cleared from the file details and can only be viewed in the notes tab.
Do you want to proceed?

is there a mismatch in confluence or something?

devinleighsmith · 2024-07-11T00:32:25Z

source/frontend/src/features/leases/add/LeaseDetailSubForm.tsx

@@ -78,13 +78,13 @@ export const LeaseDetailSubForm: React.FunctionComponent<ILeaseDetailsSubFormPro
  };

  return (
-    <Section>
-      <SectionField label="Ministry project" labelWidth="2">
+    <Section header="Original Aggreement">


Aggreement -> Agreement

devinleighsmith · 2024-07-11T00:33:59Z

This modal message doesn't match jira: The lease is no longer in Terminated state. The reason for doing so will be cleared from the file details and can only be viewed in the notes tab. Do you want to proceed?

is there a mismatch in confluence or something?

Also, the message in the story mentions deleting the terminated date when the lease status is set to non-terminated, but I think right now only the termination reason is cleared.

devinleighsmith

Needs some changes before merge.

FuriousLlama · 2024-07-11T05:07:05Z

It looks like the formik yup schema was not updated for the new business rules. So, for example, commencement/expiry date are listed as required for exercised renewals visually, but I can still save an exercised renewal without providing any dates.

You should get a server error. The AC specifies that the validation happens on save

FuriousLlama · 2024-07-11T05:09:34Z

This modal message doesn't match jira: The lease is no longer in Terminated state. The reason for doing so will be cleared from the file details and can only be viewed in the notes tab. Do you want to proceed?

is there a mismatch in confluence or something?

TBH, took most of the text as a suggestion given there where ambiguities between the JIRA ticket, confluence and mokcup + a conversation with julian. At this point I rather fine tune this once the BAs use the application.

github-actions · 2024-07-11T05:14:17Z

✅ No secrets were detected in the code.

… into features/psp-8630

github-actions · 2024-07-11T05:17:21Z

✅ No secrets were detected in the code.

devinleighsmith · 2024-07-11T16:05:16Z

It looks like the formik yup schema was not updated for the new business rules. So, for example, commencement/expiry date are listed as required for exercised renewals visually, but I can still save an exercised renewal without providing any dates.

You should get a server error. The AC specifies that the validation happens on save

Hmm, I don't agree on this one, I can't think of any cases where we mark a field as required but allow the form to be submitted without that required field.

devinleighsmith

Also, the yup schema needs to be updated to enforce the max character limit on the new text fields.

FuriousLlama · 2024-07-11T17:15:33Z

It looks like the formik yup schema was not updated for the new business rules. So, for example, commencement/expiry date are listed as required for exercised renewals visually, but I can still save an exercised renewal without providing any dates.

You should get a server error. The AC specifies that the validation happens on save

Hmm, I don't agree on this one, I can't think of any cases where we mark a field as required but allow the form to be submitted without that required field.

It's debatable. Is this a PR blocker?

devinleighsmith · 2024-07-11T17:16:17Z

It looks like the formik yup schema was not updated for the new business rules. So, for example, commencement/expiry date are listed as required for exercised renewals visually, but I can still save an exercised renewal without providing any dates.

You should get a server error. The AC specifies that the validation happens on save

Hmm, I don't agree on this one, I can't think of any cases where we mark a field as required but allow the form to be submitted without that required field.

It's debatable. Is this a PR blocker?

no, just the comment about the max text lengths.

github-actions · 2024-07-11T17:44:50Z

✅ No secrets were detected in the code.

github-actions · 2024-07-11T17:45:21Z

✅ No secrets were detected in the code.

FuriousLlama added 7 commits July 4, 2024 13:46

Initial work on frontend

0be38a9

Manual merge

be11849

Added renewal frontend subform and backend models

0d51562

Wired Frontend and backend to update and create renewals

5752e9d

Added termination date, aggrement UI updates and renewals date valida…

8f7ae10

…tion

Updated error messages

0a4280c

Updated tests and snaps

b0ff52c

FuriousLlama added the enhancement New feature or request label Jul 10, 2024

FuriousLlama self-assigned this Jul 10, 2024

FuriousLlama added 2 commits July 10, 2024 12:56

Manual merge

52f816a

Regenerated snaps

c4adf13

devinleighsmith reviewed Jul 10, 2024

View reviewed changes

devinleighsmith reviewed Jul 11, 2024

View reviewed changes

devinleighsmith requested changes Jul 11, 2024

View reviewed changes

Merge branch 'dev' into features/psp-8630

d26a88b

FuriousLlama added 2 commits July 10, 2024 22:14

PR updates

6e4ed67

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

158f48f

… into features/psp-8630

FuriousLlama requested a review from devinleighsmith July 11, 2024 05:15

devinleighsmith approved these changes Jul 11, 2024

View reviewed changes

devinleighsmith requested changes Jul 11, 2024

View reviewed changes

FuriousLlama added 2 commits July 11, 2024 10:42

Updated max character limits for renewal forms

e2b05d1

Merge branch 'dev' into features/psp-8630

b3e8ad3

FuriousLlama requested a review from devinleighsmith July 11, 2024 17:59

devinleighsmith approved these changes Jul 11, 2024

View reviewed changes

FuriousLlama merged commit cdff5fe into bcgov:dev Jul 11, 2024
7 checks passed

This was referenced Aug 19, 2024

Monthly pr metrics report devinleighsmith/PSP#195

Open

Yearly issue metrics report for opened issues and prs devinleighsmith/PSP#196

Open

This was referenced Aug 19, 2024

Pull requests report(19/08/2024 20:30) #4273

Open

Pull requests report(19/08/2024 21:02) #4274

Open

psp-8630 | Updated Leases and added renewals #4180

psp-8630 | Updated Leases and added renewals #4180

Conversation

FuriousLlama commented Jul 10, 2024 • edited Loading

github-actions bot commented Jul 10, 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

devinleighsmith commented Jul 11, 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

devinleighsmith commented Jul 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinleighsmith commented Jul 11, 2024

Choose a reason for hiding this comment

devinleighsmith commented Jul 11, 2024

devinleighsmith left a comment

Choose a reason for hiding this comment

FuriousLlama commented Jul 11, 2024

FuriousLlama commented Jul 11, 2024

github-actions bot commented Jul 11, 2024

github-actions bot commented Jul 11, 2024

devinleighsmith commented Jul 11, 2024

devinleighsmith left a comment

Choose a reason for hiding this comment

FuriousLlama commented Jul 11, 2024

devinleighsmith commented Jul 11, 2024

github-actions bot commented Jul 11, 2024

github-actions bot commented Jul 11, 2024

FuriousLlama commented Jul 10, 2024 •

edited

Loading