Skip to content

Conversation

@danamansana
Copy link
Contributor

Beginning of refactor of discovery-api, focusing on lib/routes
The goal is to make the lib/routes file easier to read, with every method corresponding to a route, and each route-related function being relatively simple.
Most of this PR is just moving code out of the lib/routes file to util files.
In addition, the methods in lib/routes are rewritten using async/await instead of promises.
I'd like to rewrite the elastic-body-builder methods as well, but that is only started. In particular the body for finding by uri uses the new innerHits method instead of addInnerHits, but it isn't quite working for the search request, so that method still needs addInnerHits. Reworking the body builder is still ongoing.

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 good

// Create promise to resolve deliveryLocationTypes by patron type:
const lookupPatronType = async function (params) {
try {
await AvailableDeliveryLocationTypes.getScholarRoomByPatronId(params.patronId)
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't seem like this returns anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the only purpose of this function is to throw an error if something goes wrong. Maybe a more descriptive comment would clear that up?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like getScholarRoomByPatronId returns a location code, which you use to determine delivery locations

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, added a return

}
}

const makeRelevanceReport = (params) => (r, ind) => {
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate that this came from even more confusing code, but let's make it clearer what's happening here through documentation and better var names?

lib/resources.js Outdated
itemUri: (uri) => {
return { term: { 'items.uri': uri } }
}
const [resp] = Promise.all([fetchItems, lookupPatronType])
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem right. I think you probably want to destructure as const [items, scholarRoomCode] = ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


app.logger.debug('Resources#itemsByFilter', body)
return app.esClient.search(body)
.then((resp) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still using promise chaining. can you update to make async/await?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use async and cleaned up

lib/resources.js Outdated
itemUri: (uri) => {
return { term: { 'items.uri': uri } }
}
const [resp] = Promise.all([fetchItems, lookupPatronType])
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this Promise.all needs to be await-ed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep

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