OBPIH-6969 Improve approach to product creation and stock data (endpoints) #5480
Conversation
alannadolny
requested review from
ewaterman,
awalkowiak,
kchelstowski and
SebastianLib
github-actions
bot
added
the
domain: backend
|
can you add some context to the PR? How do these new APIs help with e2e tests? Are they going to call the APIs? Why couldn't e2e tests use the existing non-API importers? |
|
@ewaterman, I wanted to add some context after resolving the issue that I am facing in the e2e repository. Those endpoints are created because of the need to improve data creation when we need to handle a lot of rows at once. It's needed in some tests that Katarzyna has to do in the future. It's done using an API to improve the performance of that action. When there is a need to do it more frequently, I mean resetting the data through import before every test, then this approach will be much faster. |
| } | ||
|
|
||
| if (!product.save(flush: true)) { | ||
| Product savedProduct = product.save(flush: true) |
There was a problem hiding this comment.
nitpick: I think it'd still work the way it originally was and so this assignment to savedProduct might not be needed. You can still call importedProducts.add(product) later and I believe it'd contain the saved product. I'm not 100% sure though
There was a problem hiding this comment.
You're right, started thinking in JavaScript way on the backend 😄
| * Inventory API endpoints | ||
| */ | ||
|
|
||
| "/api/locations/$id/inventories/import"(parseRequest: true) { |
There was a problem hiding this comment.
why not /api/facilities/$facilityId/... (instead of /locations) like we've been doing for other APIs?
Also is the parseRequest required?
There was a problem hiding this comment.
I can see that we mixed /facilities vs /locations (16 usages vs 14 usages), but I am going to follow the facility pattern
| transactionDate(nullable: false, | ||
| validator: { value -> value <= new Date() }) | ||
| validator: { | ||
| value -> value <= new Date(System.currentTimeMillis() + 1000) |
There was a problem hiding this comment.
why is this needed? With this change you can now make transactions 1 second in the future. I don't think we want that
There was a problem hiding this comment.
In the importData method, the baseline date is the current date, and the adjustment date is the baseline date + 1. When the import happens quickly, the adjustment date is in the future, so the validation fails. The logic in importData doesn't seem right to me in that context. I am going to subtract one second while creating ImportDataCommand, unless you have nothing against creating a baseline transaction with the date -1s, and the adjustment just on the transaction date.
| } | ||
|
|
||
| return inventoryService.findAndUpdateOrCreateInventoryItem(product, lotNumber, expirationDate) | ||
| return inventoryService.findAndUpdateOrCreateInventoryItem(product, lotNumber, expirationDateRaw) |
There was a problem hiding this comment.
suggestion: I personally don't see this refactoring as cleaner since it now needs to call inventoryService.findAndUpdateOrCreateInventoryItem all over the place. If you're really committed to not using else if conditions then I suggest doing this instead:
Date expirationDate = parseExpirationDate(expirationDateRaw)
return inventoryService.findAndUpdateOrCreateInventoryItem(product, lotNumber, expirationDate)
Date parseExpirationDate(Object expirationDateRaw) {
if (expirationDateRaw instanceof String) {
Date date = EXPIRATION_DATE_FORMAT.parse(expirationDateRaw)
Calendar calendar = Calendar.getInstance()
calendar.setTime(expirationDate)
return calendar.getTime()
}
if (expirationDateRaw instanceof Date) {
return expirationDateRaw
}
if (expirationDateRaw instanceof LocalDate) {
return expirationDateRaw.toDate()
}
return null
}
|
|
||
| return parser.collect { record -> | ||
| record.toMap().collectEntries { k, v -> | ||
| [(toCamelCase(k)): v] |
There was a problem hiding this comment.
Why do we want to modify the input data? What does it give us?
There was a problem hiding this comment.
The headers of CSV that we are using are like "Product Code", etc., so this is parsing it to "productCode" to work with the import method
There was a problem hiding this comment.
What if someone mistypes Porduct Code? Will it try to map to porductCode, which would later result in some potential issues? Or there will be an exception meaningful to the user, or will he be stuck trying to figure out why his file is not being imported?
There was a problem hiding this comment.
The importData method will throw an error in the same way as it is done on the import on Grails pages
|
|
||
| private Location parseBinLocation(String binLocationName, Location parentLocation) { | ||
| if (binLocationName == null) { | ||
| if (!binLocationName) { |
There was a problem hiding this comment.
What about if we have a bin location with a "" name? I know we've had issues with blanks vs nulls before so I'd be weary of treating them both as equivalent to null.
Nvm I checked and locations cannot be blank so this is fine.
alannadolny
force-pushed
the
OBPIH-6969
branch
from
1cc1b8a to
23f7306
Compare
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
No description provided.