-
Notifications
You must be signed in to change notification settings - Fork 24
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
PSP-7980 : Display L/L Status in Property Information Pane - Management (Summary Section) #4188
Conversation
eddherrera
commented
Jul 11, 2024
…nt (Summary Section)
✅ No secrets were detected in the code. |
hasActiveExpiryDate = false; | ||
|
||
List<PimsLease> activeAgreementsList = propertyLeases.Select(x => x.Lease).ToList(); | ||
var activeLease = activeAgreementsList.FirstOrDefault(x => x.LeaseStatusTypeCode == LeaseStatusTypes.ACTIVE.ToString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding of the requirements, is that if there is any lease that is active for a property, that should be displayed on the management screen.
Given that a single property may have multiple leases (ie, a large lot with many buildings). it seems entirely possible that a single property may have multiple active leases, where some of those active leases may have expired or been terminated.
as a result, I think using FirstOrDefault here doesn't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to check for other leases with a loop.
} | ||
else | ||
{ | ||
var latestRemewal = activeLease.PimsLeaseRenewals.Where(x => x.IsExercised.HasValue && x.IsExercised.Value).OrderByDescending(x => x.CommencementDt).FirstOrDefault(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit latestRemewal -> latestRenewal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI a fast way of checking if a nullable boolean is a specific value is just x.IsExercised == true, since null != true and false != true
var latestRemewal = activeLease.PimsLeaseRenewals.Where(x => x.IsExercised.HasValue && x.IsExercised.Value).OrderByDescending(x => x.CommencementDt).FirstOrDefault(); | ||
if (latestRemewal is null) // No Renewal - Check only Lease dates. | ||
{ | ||
if(activeLease.OrigExpiryDate.HasValue && activeLease.OrigExpiryDate.Value.Date >= DateTime.UtcNow.Date) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is an example of when you don't want to use the UTC date. If you did, and a user used this logic before 7am, the utc date would not match the pst date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way to think of this - when we store dates with times, we store them in UTC. When we store dates only, we store them as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note that this is backend (C#) code so this runs in the timezone of the .NET container - which by default is UTC in docker/openshift. I'm not aware that we set the timezone to PST on our live env containers.
If we want to do this "Date math" in PST we need to do that manually (either subtract 7hrs from UTC or convert the exp date to UTC)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
} | ||
else if(activeLease.OrigExpiryDate.HasValue && !latestRemewal.ExpiryDt.HasValue) | ||
{ | ||
hasActiveLease = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't this also mean that the lease has an active expiry date? Or if not, is this even possible for exercised agreements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not quite sue, I am trying to cover for the scenarios not documented in the AC.
} | ||
else if (!activeLease.OrigExpiryDate.HasValue && latestRemewal.ExpiryDt.HasValue) | ||
{ | ||
hasActiveLease = latestRemewal.ExpiryDt.Value.Date >= DateTime.UtcNow.Date; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same thing here?
✅ No secrets were detected in the code. |
✅ No secrets were detected in the code. |
✅ No secrets were detected in the code. |