OBPIH-6674 Unable to update bc and unit price through PO import when other items inluced in invoice#4846
Conversation
alannadolny
requested review from
awalkowiak,
drodzewicz,
ewaterman and
kchelstowski
github-actions
bot
added
the
domain: backend
kchelstowski
left a comment
kchelstowski
left a comment
There was a problem hiding this comment.
I think we should revisit as soon as possible our ddl and add the constraints to the database. This is probably the reason why the duplicate exists - it must've been updated/inserted through the DB. I can't see any unique constraint on the budget_code table.
|
|
||
| // There can be more than one budget code with the same code, despite the fact that the code is unique from the domain perspective. | ||
| // As a fix for that we would like to have only one active budget code with the same code, so we have to find only active | ||
| // budget codes. In case when there is only one budget code, and this one is inactive we have to throw a validation error, |
There was a problem hiding this comment.
if you say so, why don't you throw this exception if the foundBudgetCodes.first() would be inactive?
There was a problem hiding this comment.
It's threw a few lines below
| // budget codes. In case when there is only one budget code, and this one is inactive we have to throw a validation error, | ||
| // so we can't just look for BudgetCode.findAllByCodeAndActive(code, true); | ||
| List<BudgetCode> foundBudgetCodes = BudgetCode.findAllByCode(code) | ||
| BudgetCode budgetCode = foundBudgetCodes.size() > 1 ? foundBudgetCodes.find { it.active } : foundBudgetCodes.first() |
There was a problem hiding this comment.
If we have a case with multiple active, then we still might have the issue with accidentally updating the budget code on the item, perhaps it would be worth throwing an exception if we have multiple active budget codes with the same code?
|
I agree with Kacper that I'd like to prioritize a proper fix for this. I was curious to know how many cases of duplicate code that we have in prod: and the offenders in detail with only Given that it seems to be a very rare occurrence maybe we can fix what we have on prod manually (though we'd still potentially need a migration to fix any non-PIH environments). While I think we should add unique constraint, I don't see a simple way to fix existing data via migration given that we'd need a way to pick which code should be the active one, which isn't immediately obvious from just looking at the data. |
| throw new IllegalArgumentException("Found more than one active budget code with the same code.") | ||
| } | ||
|
|
||
| BudgetCode budgetCode = activeBudgetCodes.size() == 0 ? foundBudgetCodes[0] : activeBudgetCodes.first() |
There was a problem hiding this comment.
This still feels somewhat clunky in that if there are multiple inactive codes (and no active) we just pick whatever the first one is. Why do we allow the user to set an inactive budget code? Shouldn't it simply error if there's no active code that matches?
nitpick: you're mixing your access methods here ([0] and x.first())
There was a problem hiding this comment.
you're mixing your access methods here ([0] and x.first())
I did it on purpose, the .first method throws an error when there is no element, [0] doesn't so it will be null, and then we will get the information that we didn't find any budget code.
If there is more than one inactive budget code, we will get the error that the budget code is inactive, independently of which one inactive budget code the system will pick.
There was a problem hiding this comment.
oh neat I didn't actually know that groovy made index-based accesses null safe. 👍
…other items inluced in invoice (openboxes#4846)
…other items inluced in invoice (openboxes#4846)
4 participants
No description provided.