Skip to content

Provide filled out item data when returning hold list #224

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.

Sign up for GitHub

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

Merged
kenoir merged 8 commits into from
Aug 13, 2021
Merged

Provide filled out item data when returning hold list #224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6082a62
Provide filled out item data when returning hold list
kenoir Aug 12, 2021
9b2e276
Apply auto-formatting rules
weco-bot Aug 12, 2021
e2edb84
use correct namespace for SierraPatronNumber
kenoir Aug 12, 2021
aaf731f
Merge branch 'fill-item-requests-api' of github.com:wellcomecollectio…
kenoir Aug 12, 2021
87aa0f6
to name things wongly is to add the misfortune of the world
kenoir Aug 12, 2021
dee9e42
Apply auto-formatting rules
weco-bot Aug 12, 2021
67383fc
add better todo comment
kenoir Aug 12, 2021
ad8f155
Merge branch 'fill-item-requests-api' of github.com:wellcomecollectio…
kenoir Aug 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions common/stacks/src/main/scala/weco/api/stacks/models/StacksUserHolds.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
package weco.api.stacks.models

import weco.catalogue.internal_model.identifiers.{CanonicalId, SourceIdentifier}

import weco.catalogue.internal_model.identifiers.{IdState, SourceIdentifier}
import java.time.Instant

import weco.catalogue.internal_model.work.Item

case class StacksUserHolds(
userId: String,
holds: List[StacksHold]
Expand All @@ -13,7 +14,7 @@ case class StacksHold(
sourceIdentifier: SourceIdentifier,
pickup: StacksPickup,
status: StacksHoldStatus,
canonicalId: Option[CanonicalId] = None
item: Option[Item[IdState.Identified]] = None
)

case class StacksHoldStatus(
Expand Down
10 changes: 6 additions & 4 deletions common/stacks/src/main/scala/weco/api/stacks/models/display/DisplayResultsList.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import weco.api.stacks.models.{
StacksPickupLocation,
StacksUserHolds
}
import weco.catalogue.display_model.models.{DisplayIdentifier, DisplayItem}
import weco.catalogue.display_model.models.DisplayItem

object DisplayResultsList {
def apply(
Expand All @@ -28,9 +28,11 @@ case class DisplayResultsList(
object DisplayRequest {
def apply(hold: StacksHold): DisplayRequest = {
DisplayRequest(
item = new DisplayItem(
id = hold.canonicalId.map { _.underlying },
identifiers = Some(List(DisplayIdentifier(hold.sourceIdentifier)))
// TODO: This .get should always be Some here
Copy link
Contributor Author

@kenoir kenoir Aug 12, 2021

Choose a reason for hiding this comment

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

I'll refactor this next, but to speed up getting this data into the front-end we could merge this PR, deploy and put the refactor into a new PR.

// TODO: but we should refactor to remove it!
item = DisplayItem(
item = hold.item.get,
includesIdentifiers = true
),
pickupDate = hold.pickup.pickUpBy,
pickupLocation = DisplayLocationDescription(
Expand Down
16 changes: 11 additions & 5 deletions requests/src/main/scala/weco/api/requests/responses/LookupPendingRequests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
package weco.api.requests.responses

import akka.http.scaladsl.server.Route
import weco.api.search.elasticsearch.ElasticsearchError
import weco.api.stacks.models.display.DisplayResultsList
import weco.api.search.rest.CustomDirectives
import weco.api.stacks.models.StacksHold
import weco.api.stacks.services.{ItemLookup, SierraService}
import weco.catalogue.internal_model.identifiers.IdState
import weco.catalogue.internal_model.work.Item
import weco.sierra.models.identifiers.SierraPatronNumber

import scala.concurrent.{ExecutionContext, Future}
Expand All @@ -21,16 +25,18 @@ trait LookupPendingRequests extends CustomDirectives {
userHolds <- sierraService.getStacksUserHolds(patronNumber)

holdsWithCatalogueIds <- Future.sequence(
userHolds.right.get.holds.map { hold =>
userHolds.right.get.holds.map { hold: StacksHold =>
itemLookup
.bySourceIdentifier(hold.sourceIdentifier)
.map {
case Left(elasticError) =>
warn(s"Unable to look up $hold in Elasticsearch")
case Left(elasticError: ElasticsearchError) =>
Copy link
Contributor Author

@kenoir kenoir Aug 12, 2021

Choose a reason for hiding this comment

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

Previously we just chucked this error, not sure why - i've added it to the warning here.

warn(
s"Unable to look up $hold in Elasticsearch: ${elasticError}"
Copy link
Contributor

@alexwlchan alexwlchan Aug 12, 2021

Choose a reason for hiding this comment

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

Let's not address it here for the sake of getting this PR out quickly, but having a sourceIdentifier on the hold feels a bit odd to me – a hold is a thing in Sierra and could have a source identifier, it's just not the identifier we're using here.

Copy link
Contributor

@alexwlchan alexwlchan Aug 12, 2021

Choose a reason for hiding this comment

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

What will happen in this branch if we can't find the item?

Copy link
Contributor Author

@kenoir kenoir Aug 13, 2021

Choose a reason for hiding this comment

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

but having a sourceIdentifier on the hold feels a bit odd to me

I agree, we use it to look up the item later and fill the Item option on StacksHold which itself feels wrong. I expect that it'll get removed refactoring out the .get.

What will happen in this branch if we can't find the item?

Good point. I think this is actually a case that will blow up the .get on the option added in this PR (theStacksHold is returned with a None for an Item. It's still quite unlikely but we should handle this by doing a multi-item get and returning a list of the items we can get while logging that things are missing with a warn.

)
hold

case Right(item) =>
hold.copy(canonicalId = Some(item.id.canonicalId))
case Right(item: Item[IdState.Identified]) =>
hold.copy(item = Some(item))
}
}
)
Expand Down
7 changes: 6 additions & 1 deletion requests/src/test/scala/weco/api/requests/RequestsApiFeatureTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,16 @@ class RequestsApiFeatureTest
)
)

val titleString = randomAlphanumeric(length = 20)

val item = createIdentifiedItemWith(
sourceIdentifier = SourceIdentifier(
identifierType = SierraSystemNumber,
value = itemNumber.withCheckDigit,
ontologyType = "Item"
)
),
locations = List.empty,
title = Some(titleString)
)

val lookup = new MemoryItemLookup(items = Seq(item))
Expand All @@ -98,6 +102,7 @@ class RequestsApiFeatureTest
| "type" : "Identifier"
| }
| ],
| "title" : "${titleString}",
Copy link
Contributor

@alexwlchan alexwlchan Aug 12, 2021

Choose a reason for hiding this comment

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

nit:

Suggested change
| "title" : "${titleString}",
| "title" : "$titleString",

| "locations" : [
| ],
| "type" : "Item"
Expand Down