MODBULKOPS-334 - Preventing record update with values from different tenants #278
Conversation
This reverts commit 0fe27cb.
| @@ -34,15 +46,27 @@ public List<HoldingsNote> convertToObject(String value) { | |||
|
|
|||
| @Override | |||
| public String convertToString(List<HoldingsNote> object) { | |||
| return object.stream() | |||
| var str = object.stream() | |||
There was a problem hiding this comment.
No need to introduce local variable str here - return result.
| noteTypeName = ItemReferenceHelper.service().getNoteTypeNameById(itemNote.getItemNoteTypeId(), itemNote.getTenantId()); | ||
| } catch (NotFoundException e) { | ||
| log.error("Item note type with id = {} not found : {}", itemNote.getItemNoteTypeId(), e.getMessage()); | ||
| noteTypeName = ""; |
There was a problem hiding this comment.
Initialise this variable before try-catch block as empty string.
| @@ -8,8 +8,10 @@ | |||
| import lombok.NoArgsConstructor; | |||
| import lombok.With; | |||
| import org.folio.bulkops.domain.dto.IdentifierType; | |||
| import org.folio.bulkops.domain.dto.TenantNotePair; | |||
There was a problem hiding this comment.
TenantNotePair can be renamed to ExtendedNoteTypeName o follow general rule when including tenant information.
| noteTypeName = HoldingsReferenceHelper.service().getNoteTypeNameById(note.getHoldingsNoteTypeId()); | ||
| } catch (NotFoundException e) { | ||
| log.error("Holding note type with id = {} not found : {}", note.getHoldingsNoteTypeId(), e.getMessage()); | ||
| noteTypeName = ""; |
There was a problem hiding this comment.
String noteType name can be initialised as empty string before try-catch block.
| return (Updater<ExtendedHoldingsRecord>) electronicAccessUpdaterFactory.updater(option, action); | ||
| } else if (REPLACE_WITH == action.getType()) { | ||
| return extendedHoldingsRecord -> { | ||
| var locationId = action.getUpdated(); | ||
| var tenant = getTenantFromAction(action); |
There was a problem hiding this comment.
Tenant is used only if forPreview = true. This can be moved inside if-statement.
| @@ -122,14 +134,16 @@ private void validateReplacement(UpdateOptionType option, Action action) throws | |||
| } | |||
|
|
|||
| if (Set.of(PERMANENT_LOCATION, TEMPORARY_LOCATION).contains(option)) { | |||
| try { | |||
| itemReferenceService.getLocationById(newId); | |||
| var tenant = getTenantFromAction(action); | |||
There was a problem hiding this comment.
Looks this is duplication. var tenant = getTenantFromAction(action); can be calculated outside if (Set.of(PERMANENT_LOCATION, TEMPORARY_LOCATION).contains(option)) statement.
| var ruleTenants = rule.getRuleDetails().getTenants(); | ||
| var actionTenants = action.getTenants(); | ||
| if (nonNull(ruleTenants) && !ruleTenants.isEmpty() && nonNull(actionTenants) && !actionTenants.isEmpty()) { |
There was a problem hiding this comment.
Hi @obozhko-folio , it is better to have here separate method to understand why such checking is needed.
|
MODBULKOPS-334 - Preventing record update with values from different tenants
Purpose
Align logic before commit stage.
Approach
Use rule and action tenants from content update.
TODOS and Open Questions
Learning
Pre-Merge Checklist:
Before merging this PR, please go through the following list and take appropriate actions.
If there are breaking changes, please STOP and consider the following:
Ideally all of the PRs involved in breaking changes would be merged in the same day to avoid breaking the folio-testing environment. Communication is paramount if that is to be achieved, especially as the number of intermodule and inter-team dependencies increase.
While it's helpful for reviewers to help identify potential problems, ensuring that it's safe to merge is ultimately the responsibility of the PR assignee.