INT-B-21839 Added payment service items to audit history (Capturing rejection reason) #14590
Conversation
…eated template for move history events for rejecting/accepting payment_service_items
…cher for payment_service_items, renamed move history event for consistency
…-B-21838-payment-request-rejection-reason
Bundle StatsHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded
Removed
Bigger
Smaller No assets were smaller Unchanged
|
brooklyn-welsh
added
ByteSize M&Ms
| eventName: o.updatePaymentServiceItemStatus, | ||
| tableName: t.payment_service_items, | ||
| getEventNameDisplay: () => { | ||
| return <div>Updated Payment Service Item</div>; |
There was a problem hiding this comment.
Could conditionally render if the service item was rejected/approved?
Rejected Payment Service Item
or
Approved Payment Service Item
You can look at the changedValues variable to determine which name to show
There was a problem hiding this comment.
Agreed. And if you change this, I'd recommend changing the associated tests to check for that text being displayed instead of just the status being different.
There was a problem hiding this comment.
Good idea, will update the template and tests to replace "updated" with the appropriate verb
There was a problem hiding this comment.
https://github.com/transcom/mymove/pull/14590/files#r1917293731
as well as the associated test change (to look for the text to display on the component).
reason: the ask of the PO isn't protected unless the information is displayed
brooklyn-welsh
changed the title
…updating a service item. Also updated field mapping to include "denied_at" field.
…-B-21838-payment-request-rejection-reason
brooklyn-welsh
requested review from
begrohmann,
WeatherfordAaron and
danieljordan-caci
There was a problem hiding this comment.
Could we omit the empty values showing up here?
So when a TIO rejects a service item, we don't need the Approved at, Denied at (it's already showing the timestamp in the Date & Time column of the move history, and I would even say we don't need the DENIED showing because we have the Rejected Payment Service Item
And for approved payment service items, we don't need Denied at, Reason or Approved at
Just trying to think of ways to condense it down to show only the important information and reduce a little redundancy. Any thoughts on that?
There was a problem hiding this comment.
Also want to bring this up cause I just pushed the BL into PR, but B-21519 is addressing an issue when the TIO "clears selection", which changes the status back toREQUESTED. I put the code change in your branch and this is what displays:
So ensure that we aren't showing the Approved at for payment service items that are switched back into REQUESTED status
No I agree, wasn't sure how much data they expect from the audit page, so left in the empty values (denoting a deleted value in the db), for time stamps I left them in since they seem to be using a different timezone, I assume one is UTC and the other is local, and wasn't sure if that mattered. And yeah the "DENIED" and "APPROVED" fields are also just extra noise with the prefix added. I agree that they aren't worth keeping. I can edit the sql/mappings to get rid of those then request review again |
Cool thanks for the heads up, this should be fixed when I stop tracking deleted values in the db, but will double check to make sure before pushing up more changes. |
Sounds good! Thank you! |
…g, removed a bunch of unnecessary change tracking for payment items (approved_at, denied_at)
…-B-21838-payment-request-rejection-reason
… title text. Fixed a potential bad if check in the actionPrefix section.
…-B-21838-payment-request-rejection-reason
Okay, I should have removed all of the extra info now, should only show reason for rejected items, and the other fields you mentioned were completely removed. I also added rejection reasons to completed payment service requests andedited the PR description with a new screenshot at the bottom. I thought this more closely matches the AC for the story, but think that keeping the individual items for each service item in the log would also be helpful. Let me know what you think. |
There was a problem hiding this comment.
Still seeing Approved at: when the TIO clicks the "clear selection" button.
In payment_service_item_status_updater.go put this:
switch desiredStatus {
// when the user hits "clear selection" we want to clear all the fields
case models.PaymentServiceItemStatusRequested:
paymentServiceItem.RejectionReason = nil
paymentServiceItem.DeniedAt = nil
paymentServiceItem.ApprovedAt = nil
// if being denied, we want to nil out approvedAt and populate deniedAt
case models.PaymentServiceItemStatusDenied:
paymentServiceItem.RejectionReason = rejectionReason
paymentServiceItem.DeniedAt = models.TimePointer(time.Now())
paymentServiceItem.ApprovedAt = nil
// if being approved, populate approvedAt
case models.PaymentServiceItemStatusApproved:
paymentServiceItem.RejectionReason = nil
paymentServiceItem.DeniedAt = nil
paymentServiceItem.ApprovedAt = models.TimePointer(time.Now())
}
paymentServiceItem.Status = desiredStatus
and the clear selection button should work as expected
Also seeing Approved at: for approved service items - can shed that since we already see it on the far left with the audit history timestamp
Good catch, glossed over the clear selection button. Thanks for the code snippet, works great! I'll need to make one more edit as I'm still seeing "Reason: -" if I clear selection when the item is in a rejected state, but I think I already know what I need to do to fix that. |
…cked "cleared selection" on a service item it would show as rejected. Added unit test for this case.
…-B-21838-payment-request-rejection-reason
Okay newest push should fix that issue and the other one I referenced, also added a unit test to catch these as well. |
Oh nice! Pulling this now. |
| if desiredStatus == models.PaymentServiceItemStatusDenied { | ||
| switch desiredStatus { | ||
| // when the user hits "clear selection" we want to clear all the fields | ||
| case models.PaymentServiceItemStatusRequested: | ||
| paymentServiceItem.RejectionReason = nil | ||
| paymentServiceItem.DeniedAt = nil | ||
| paymentServiceItem.ApprovedAt = nil | ||
| // if being denied, we want to nil out approvedAt and populate deniedAt | ||
| case models.PaymentServiceItemStatusDenied: | ||
| paymentServiceItem.RejectionReason = rejectionReason | ||
| paymentServiceItem.DeniedAt = models.TimePointer(time.Now()) | ||
| paymentServiceItem.ApprovedAt = nil | ||
| paymentServiceItem.Status = desiredStatus | ||
| } | ||
| // If we're approving this thing then we don't want there to be a rejection reason | ||
| // We also will want to update the ApprovedAt field and nil out the DeniedAt field. | ||
| if desiredStatus == models.PaymentServiceItemStatusApproved { | ||
| // if being approved, populate approvedAt | ||
| case models.PaymentServiceItemStatusApproved: | ||
| paymentServiceItem.RejectionReason = nil | ||
| paymentServiceItem.DeniedAt = nil | ||
| paymentServiceItem.ApprovedAt = models.TimePointer(time.Now()) | ||
| paymentServiceItem.Status = desiredStatus | ||
| } | ||
| paymentServiceItem.Status = desiredStatus |
There was a problem hiding this comment.
oh don't add this, this is going in with B-21519
I was just saying to put that there to test haha
There was a problem hiding this comment.
Ohh lol got it, will take that out and push back up then
| @@ -24,6 +24,13 @@ const filterContextStatus = (context, statusToFilter) => { | |||
| <div className={styles.serviceItemRow} key={`${value.name}`}> | |||
| <div>{value.name}</div> | |||
| <div>{price.toFixed(2)}</div> | |||
| <div className={styles.break} /> | |||
| {value.status === 'DENIED' ? ( | |||
There was a problem hiding this comment.
Can you use the PAYMENT_SERVICE_ITEM_STATUS variable in constants.js?
| historyRecord.changedValues.rejection_reason !== null && | ||
| historyRecord.changedValues.status !== 'APPROVED' && | ||
| historyRecord.changedValues.status !== 'REQUESTED') || | ||
| historyRecord.changedValues.status === 'REJECTED' |
There was a problem hiding this comment.
Can you use variables instead? PAYMENT_SERVICE_ITEM_STATUS in constants.js - sorry, just now thought of that
There was a problem hiding this comment.
No good point, I'll add that as well. I wasn't seeing the approved at anymore earlier but I must've missed something, I'll keep testing
There was a problem hiding this comment.
I double pulled just to be sure haha
There was a problem hiding this comment.
Could you let me know if you have any specific steps to reproduce the clear selection bug? Without the go code I added earlier it doesn't even seem to have any effect (i.e. doesn't save as cleared, is still approved/rejected after you leave).
Tried clearing selection on a few different items in different states and can't get it to show up. Also not seeing approved at either, might try making a whole new payment request just to make sure nothing weird is going on with the one I'm testing with.
There was a problem hiding this comment.
Yeah, sorry, it's a known issue (thus, the BL to fix it). Hoping to have it merged in soon so you have it. I was just testing it with it so we don't have to do any revisions later.
There was a problem hiding this comment.
Ah okay, no worries. I'm still not sure why the Approved at: would be showing in your timestamps though. But, ping me whenever it gets merged in, then I'll update this one and see if it's working like it should
…-B-21838-payment-request-rejection-reason
Agility ticket
Summary
When reviewing a payment request, approvals/rejections and rejection reasons were not being captured in the Move History audit log. This branch adds the necessary SQL and backend code to capture these events, as well as the frontend template for payment service items.
How to test
Edit: Also added rejection reason to payment request as shown here:
