-
-
Notifications
You must be signed in to change notification settings - Fork 465
OBLS-284 allow to update barcode, send notification about update #5597
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.
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
base: feature/obaf-integration
Are you sure you want to change the base?
Conversation
| action = [GET: "details"] | ||
| } | ||
|
|
||
| "/api/mobile/products/$id/barcode" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a change request related to this endpoint in mobile repo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
grails-app/controllers/org/pih/warehouse/api/MobileProductApiController.groovy
Show resolved
Hide resolved
| return | ||
| } | ||
|
|
||
| if (oldUpc?.trim() == newUpc) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this check needed? Do we care if there was an actual change or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say so - it prevents spamming the user with unnecessary emails and saves resources.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olewandowski1 The reason I asked was because I was not sure about i don't like status: 'no_change' approach (since this is not our convention in such cases, and is not idiomatic REST), and because I was not fully sure how we'd like to have it done instead (if it should be a 204 in this case or a 200 with additional info), I thought it is just not required now, and we can get rid of it for now and revisit later if needed. On the other hand, if you just don't want to send the notification, you could check this on the notification send level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kk, makes sense to me now, I'll correct it
Link to GitHub issue or Jira ticket: https://openboxes.atlassian.net/browse/OBLS-284
Description: