Skip to content
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

Circ 2084 #1475

Merged
merged 6 commits into from
Jun 6, 2024
Merged

Circ 2084 #1475

merged 6 commits into from
Jun 6, 2024

Conversation

SreejaMangarapu
Copy link
Contributor

Purpose

purpose of the pr - https://folio-org.atlassian.net/browse/CIRC-2084

Approach

checking if the item is dcb item by the feild isDcb of item and user is dcb user by the lastname of user , if they are related to dcb then adding a flag isDcb with value true for loan , if not adding isDcb = false for loan

TODOS and Open Questions

  • Check logging

Learning

@@ -294,8 +296,14 @@ private Result<MultipleRecords<Loan>> mapResponseToLoans(Response response) {
return MultipleRecords.from(response, Loan::from, RECORDS_PROPERTY_NAME);
}

private static void addIsDcbProperty(Loan loan, Item item, JsonObject storageLoan) {
write(storageLoan, IS_DCB, (nonNull(loan.getUser()) && nonNull(loan.getUser().getLastName())
&& loan.getUser().getLastName().equalsIgnoreCase("DcbSystem"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use string constants for DcbSystem

private static JsonObject mapToStorageRepresentation(Loan loan, Item item) {
log.debug("mapToStorageRepresentation:: parameters loan: {}, item: {}", loan, item);
log.info("mapToStorageRepresentation:: parameters loan: {}, item: {}", loan, item);
Copy link
Contributor

Choose a reason for hiding this comment

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

pls revert this to debug level log

@OleksandrVidinieiev
Copy link
Contributor

@SreejaMangarapu
Few thoughts on this PR:

  • Since we are using the new loan property, we should also add it to the loan schema. Even though mod-circulation does not generate code from schemas, we still try to keep them up-to-date when possible.
  • With this new field we are probably altering at least a few interfaces: most notably GET and POST for /circulation/loans, probably check-out interface too. If so, we should update versions of those interfaces.
  • The underlying spike MODDCB-99 states that this flag should be added to outgoing loan-related events. I see no changes related to Kafka events in this PR. Am I missing something? Or will event-related changes be introduced in subsequent PRs?

Comment on lines 301 to 303
write(storageLoan, IS_DCB, (nonNull(loan.getUser()) && nonNull(loan.getUser().getLastName())
&& loan.getUser().getLastName().equalsIgnoreCase(DCB_USER_LASTNAME))
|| item.isDcbItem());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please extract 3rd argument into a method, e.g. isDcbLoan(Loan loan, Item item). Also, we can probably do item.isDcbItem() first because it's a quicker check, and then check user name.

Comment on lines 30 to 35
public IndividualResource createCirculationItemForDcb(String barcode, UUID holdingId, UUID locationId, String instanceTitle,boolean isDcb) {
CirculationItemsBuilder circulationItemsBuilder = new CirculationItemsBuilder().withBarcode(barcode).withHoldingId(holdingId)
.withLoanType(loanTypesFixture.canCirculate().getId()).withMaterialType(materialTypesFixture.book().getId())
.withLocationId(locationId).withInstanceTitle(instanceTitle).withDcb(isDcb);
return circulationItemClient.create(circulationItemsBuilder);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public IndividualResource createCirculationItemForDcb(String barcode, UUID holdingId, UUID locationId, String instanceTitle,boolean isDcb) {
CirculationItemsBuilder circulationItemsBuilder = new CirculationItemsBuilder().withBarcode(barcode).withHoldingId(holdingId)
.withLoanType(loanTypesFixture.canCirculate().getId()).withMaterialType(materialTypesFixture.book().getId())
.withLocationId(locationId).withInstanceTitle(instanceTitle).withDcb(isDcb);
return circulationItemClient.create(circulationItemsBuilder);
}
public IndividualResource createCirculationItemForDcb(String barcode, UUID holdingId,
UUID locationId, String instanceTitle,boolean isDcb) {
CirculationItemsBuilder circulationItemsBuilder = new CirculationItemsBuilder()
.withBarcode(barcode)
.withHoldingId(holdingId)
.withLoanType(loanTypesFixture.canCirculate().getId())
.withMaterialType(materialTypesFixture.book().getId())
.withLocationId(locationId)
.withInstanceTitle(instanceTitle).withDcb(isDcb);
return circulationItemClient.create(circulationItemsBuilder);
}

@@ -93,6 +93,10 @@
"description": "Indicates whether or not this loan had its due date modified by a recall on the loaned item",
"type": "boolean"
},
"isDcb": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the file name be storage-loan-7-3.json ?

Copy link

sonarqubecloud bot commented Jun 6, 2024

@SreejaMangarapu SreejaMangarapu merged commit fb01b92 into master Jun 6, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants