Skip to content

Conversation

@charmingduchess
Copy link
Contributor

@charmingduchess charmingduchess commented Dec 17, 2025

The aim of this work was to consol

  • create Item and Location models to try and abstract and clarify where logic lives
  • Begin to dismantle the deliveryLocationsResolver. this is not totally complete. much of the logic has been moved into the Item and Location models. I want to rechristen it something like locationUtils, but open to input on how to think about what's left there.
  • moved the recap barcode crunching to scsb client module
  • there are some logic changes, specifically in returning undefined instead of an empty string for certain cases of locations with no delivery locations. Any problems with that?
  • Eliminate attachDeliveryLocationsAndEddRequestability entirely. Where this method was once called, instead the Item model factory with updated delivery locations is returned.
  • create Item factory or update its constructor so the returned item has appropriate values updated (to avoid spread operator biz and having to update individual properties after instantiating item model). This is only for the delivery locations use case for now.

Further work to ticket:

  • replace JsonLdSerializers with Item (and Bib etc) models?
  • update response massager with bib/item model type processing??

Copy link
Member

@nonword nonword left a comment

Choose a reason for hiding this comment

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

Looking great. OOP for the win

this.propertiesToOverWrite.forEach((property) => {
updated[property] = this[property]
})
return updated
Copy link
Member

Choose a reason for hiding this comment

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

What's going on here?


Item.updatedWithLocationAndRequestability = (item) => {
const model = new Item(item)
return model.updated()
Copy link
Member

Choose a reason for hiding this comment

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

Is this just a factory method? What's model.updated() doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes I had originally build this as a factory method but did not end up using it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the idea is to overwrite a specific portion of the properties on the item that is passed in

}

get deliveryLocationsByM2CustomerCode () {
if (nyplCore.m2CustomerCodes()?.[this.m2CustomerCode]?.sierraDeliveryLocations) {
Copy link
Member

Choose a reason for hiding this comment

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

Should these also be checking this.nyplCoreLocation.deliverableToResolution before considering returning any locations for the given resolution method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. That is what the code was doing before. Do you remember why we didn't simply rely on the presence of a customer code to use that resolution?

lib/the-thing.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I assume this shouldn't be in here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It needs to be somewhere! I'm not sure where it goes. It surely needs a different name. I was thinking of putting it in something like a scsb requests module... and/or doing the barcode resolution in a slightly different way.

@charmingduchess charmingduchess marked this pull request as ready for review December 29, 2025 18:30
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.

3 participants