Skip to content

Apply COG sibling logic to loan returns#6038

Merged
acwhite211 merged 28 commits into
productionfrom
issue-5501
Jan 15, 2025
Merged

Apply COG sibling logic to loan returns#6038
acwhite211 merged 28 commits into
productionfrom
issue-5501

Conversation

@acwhite211

@acwhite211 acwhite211 commented Jan 6, 2025

Copy link
Copy Markdown
Collaborator

Fixes #5501

Extend the COG sibling relationship logic from loans to loan returns. So, when a loan return is added, there will also be loan returns created for it's siblings.

The complication with the return counts is handled in the following ways:

  • If the original loan return has all the items returned/resolved, then the new loan return should have all the items returned/resolved.
  • If the original loan return has zero the items returned/resolved, then the new loan return should have zero the items returned/resolved.
  • Otherwise, just match the quantity returned/resolved to the original loan return, in the case of a partial return, but don't exceed the loan prep quantity.

Checklist

  • Self-review the PR after opening it to make sure the changes look good and
    self-explanatory (or properly documented)
  • Add relevant issue to release milestone

Testing instructions

  • Create a new consolidated COG. Go to http://localhost/specify/view/collectionobjectgroup/new/, set the name, then set the cogType to consolidated. Save.
  • Add a new child CO with a new prep. Set the count to 2.
  • Repeat: Add a new child CO with a new prep. Set the count to 2.
  • Save.
  • Create a new loan, use the catalog numbers from the new COs so that both of the new preps are in the loan preps.
  • Set the loan number to something random.
  • Save.
  • Click 'Return Loan'.
  • Select the first loan prep only. Set the quantity returned and quantity resolved to 1.
  • Save.
  • Refresh page.
  • See that both of the loan preps have been fully returned. See that the quantity returned and quantity resolved are both 2.
  • Verify that the behavior for prep CO that are not a part of consolidated COG is the same as before.

==> The purpose of these tests is to verify that the count returned is set to the maximum, whatever the number entered by the user + verify that all siblings of a prep CO that is a part of a consolidated COG are also returned to they max.

@acwhite211 acwhite211 added this to the 7.9.11 milestone Jan 6, 2025
@acwhite211 acwhite211 self-assigned this Jan 6, 2025
@acwhite211
acwhite211 marked this pull request as ready for review January 7, 2025 15:06
@acwhite211 acwhite211 changed the title init modify_update_of_loan_return_sibling_preps Apply COG sibling logic to loan returns Jan 7, 2025
@acwhite211
acwhite211 requested review from a team, emenslin and pashiav January 7, 2025 15:58

@alesan99 alesan99 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • See that both of the loan preps have been fully returned. See that the quantity returned and quantity resolved are both 2.
  • Verify that the behavior for prep CO that are not a part of consolidated COG is the same as before.

If I'm understanding correctly, I believe something is wrong here?

image

image

I returned the 2nd prep with quantity and quantity resolved set to 1.
The values were maxed out to 2 for the 1st prep, but not the 2nd.

@acwhite211

acwhite211 commented Jan 7, 2025

Copy link
Copy Markdown
Collaborator Author

@alesan99 Thanks, I'll check it out. After refreshing the page, the quantity returned and quantity resolved should both be 2.

@pashiav pashiav left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • See that both of the loan preps have been fully returned. See that the quantity returned and quantity resolved are both 2.
  • Verify that the behavior for prep CO that are not a part of consolidated COG is the same as before.

(I tried 2 preps and 6 preps following the same directions)

When testing 2 preps, I had the same behavior as #6038 (review).

When testing 6 preps, only one loan prep was fully returned. The other prep only had 1 prep fully returned.
Screenshot 2025-01-07 at 10 52 26 AM

@acwhite211

Copy link
Copy Markdown
Collaborator Author

@pashiav @alesan99 That issue with the counts should be fixed now 😀

@acwhite211
acwhite211 requested review from alesan99 and pashiav January 7, 2025 17:14

@alesan99 alesan99 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That fixed it!

Also tried it with >2 preps, with varying counts per prep, and with partial loans. It properly recognizes the maximums those cases too 👍

@emenslin emenslin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

  • See that both of the loan preps have been fully returned. See that the quantity returned and quantity resolved are both 2.
  • Verify that the behavior for prep CO that are not a part of consolidated COG is the same as before.

Returning consolidated loan preps sometimes triggers unload protect

cZCYCm4uCt.mp4

Preparations that are not part of a nonconsolidated COG also return the full amount even if you only select 1, is this the intended behavior?

lF9FLhEuJF.mp4

@acwhite211

Copy link
Copy Markdown
Collaborator Author

@emenslin Good catch. Pushed a commit that should fix it so that loan preps not in consolidated preps can still be partially returned.

@acwhite211

Copy link
Copy Markdown
Collaborator Author

Returning consolidated loan preps sometimes triggers unload protect

This might be because the counts get modified in the back-end without the front-end knowing, so it thinks there are changes that need to be saved. It seems that the loan return preps are being saved properly. After pressing the save button again, the save button is greyed out, like it should be. This isn't a major issue, so I think we'll be ok with this behavior for now.

@acwhite211
acwhite211 requested a review from emenslin January 7, 2025 20:04
@emenslin

emenslin commented Jan 8, 2025

Copy link
Copy Markdown
Collaborator

Are we sure this behavior is okay and wouldn't be confusing considering this doesn't happen anywhere else?

pashiav

This comment was marked as duplicate.

@pashiav
pashiav requested a review from a team January 15, 2025 16:06
@acwhite211

Copy link
Copy Markdown
Collaborator Author

@pashiav Thanks, looks like the fix I made only avoided that error on the Loan form and not on the CO form. Just pushed a fix.

@specify/ux-testing @pashiav @emenslin good for re-review 😀

@acwhite211
acwhite211 requested a review from pashiav January 15, 2025 16:18

@pashiav pashiav left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • See that both of the loan preps have been fully returned. See that the quantity returned and quantity resolved are both 2.
  • Verify that the behavior for prep CO that are not a part of consolidated COG is the same as before.

#6038 (review) fixed!

#6038 (review) still occurs:

'Is resolved' is checked when the preparation is only partially resolved, the box should only be checked when the full amount of preps have been resolved

https://ciscollections21524gen-issue-5501.test.specifysystems.org/specify/view/loan/253/
Screenshot 2025-01-15 at 10 58 43 AM

@acwhite211

Copy link
Copy Markdown
Collaborator Author

@pashiav Thanks, the isresolved logic should work now for both consolidated and unconsolidated cogs. Sorry I didn't catch that sooner.

@acwhite211
acwhite211 requested a review from pashiav January 15, 2025 18:09

@emenslin emenslin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

  • See that both of the loan preps have been fully returned. See that the quantity returned and quantity resolved are both 2.
  • Verify that the behavior for prep CO that are not a part of consolidated COG is the same as before.

Is Resolved issue is fixed, however, I could be wrong but I thought the ability to search COGs was removed from this pr? I just want to make sure this is supposed to be implemented here before approving
Screenshot 2025-01-15 122744

@CarolineDenis

Copy link
Copy Markdown
Contributor

@emenslin, indeed it is supposed to be removed

@emenslin emenslin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

  • See that both of the loan preps have been fully returned. See that the quantity returned and quantity resolved are both 2.
  • Verify that the behavior for prep CO that are not a part of consolidated COG is the same as before.

Searching for COGS was removed, looks good

@emenslin
emenslin requested a review from a team January 15, 2025 20:00

@pashiav pashiav left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • See that both of the loan preps have been fully returned. See that the quantity returned and quantity resolved are both 2.
  • Verify that the behavior for prep CO that are not a part of consolidated COG is the same as before.

Looks good! Working as expected.

Triggered by 0608e6e on branch refs/heads/issue-5501
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅Done

Development

Successfully merging this pull request may close these issues.

Handle returned loan prep for consolidated COG

7 participants