OBGM-753 Empty string validation on name and code of organization#4416
Conversation
drodzewicz
commented
Dec 12, 2023
drodzewicz
commented
- added blank GORM validators to prevent from inserting empty string values
- added a consdition to search for exsiting organization by code and name which are not empty strings
drodzewicz
requested review from
alannadolny,
awalkowiak,
jmiranda and
kchelstowski
awalkowiak
added
the
warn: do not merge
jmiranda
left a comment
jmiranda
left a comment
There was a problem hiding this comment.
As mentioned in the ticket, we just want to do the data clean up for now. We'll save code changes for when we actually come up with the scope and acceptance criteria.
|
|
||
| Organization findOrCreateOrganization(String name, String code, List<RoleType> roleTypes) { | ||
| Organization organization = Organization.findByCodeOrName(code, name) | ||
| Organization organization = org.pih.warehouse.core.Organization.find { |
There was a problem hiding this comment.
This doesn't feel right. I think I'd rather throw an exception if the code or name are empty than change all finders in the system to perform this type of search. Note that what we come up with here might be used throughout the system since there are a lot of places where we want to do a lookup by code or name.
If we clean up the OBNAV data and prevent users from entering blank values into code and name, then that should probably be enough.
jmiranda
changed the title
drodzewicz
force-pushed
the
OBGM-753-organization-not-created-assigned-when-create-supplier-location-type
branch
from
22cd0f0 to
8cce77b
Compare
drodzewicz
changed the base branch from
feature/upgrade-to-grails-3.3.10
to
release/0.9.0-hotfix1
|
Assuming the data in the database is correct and fixed (without organizations that have invalid empty code), the issue should not reoccur anymore, the changes in the PR should ensure that it would be impossible to create an organization with an empty code or name and throw an error if both passed arguments to |
drodzewicz
added
the
status: needs discussion
| minSize: Holders.grailsApplication.config.openboxes.identifier.organization.minSize, | ||
| maxSize: Holders.grailsApplication.config.openboxes.identifier.organization.maxSize) | ||
| name(nullable: false, maxSize: 255) | ||
| name(nullable: false, blank: false, maxSize: 255) |
There was a problem hiding this comment.
I assume whenever we make such a change, we should always align GORM constraints with DB constraints, hence a migration that adds this constraint should be added.
I'm not sure how the ddl would act in this case, because we do not have anything like "NOT EMPTY" in SQL and should probably do something like:
CHECK (name <> '')There was a problem hiding this comment.
yep but we should keep in mind that CHECK is not available for MySQL 5.7 and need to make sure that we are going to be using either version 8.^ or MariaDB
|
|
||
| Organization findOrCreateOrganization(String name, String code, List<RoleType> roleTypes) { | ||
| if (!name.trim() && !code.trim()) { | ||
| throw new IllegalArgumentException("Can't perform a search on Organization with both code and name arguments being an empty strings") |
There was a problem hiding this comment.
Change message to "Must provide non-empty code and name"
- added blank GORM validators to prevent from inserting empty string values - added a consdition to search for exsiting organization by code and name which are not empty strings
drodzewicz
force-pushed
the
OBGM-753-organization-not-created-assigned-when-create-supplier-location-type
branch
from
8cce77b to
7188025
Compare
drodzewicz
changed the base branch from
release/0.9.0-hotfix1
to
feature/upgrade-to-grails-3.3.10
drodzewicz
force-pushed
the
OBGM-753-organization-not-created-assigned-when-create-supplier-location-type
branch
from
7188025 to
1a1093c
Compare
|
I pushed all of the changes that were discussed on the tech-huddle, so this PR can be finally reviewed |
awalkowiak
removed
warn: do not merge
| return findOrCreateOrganization(name, code, []) | ||
| } | ||
|
|
||
| Organization findOrganization(String code, String name) { |
There was a problem hiding this comment.
Switch the order of arguments here and in the caller so as not to confuse us later on.
| eq("name", name) | ||
| ne("name", "") | ||
| isNotNull("name") | ||
| order("dateCreated", "asc") |
There was a problem hiding this comment.
I am ok with dateCreated, but we might want to highlight this in the ticket as something that can change if desired.
Do you want the most recently created, most recently updated, or maybe some other criteria to determine which record is returned? Or would you prefer to throw an exception if more than one record was returned? If you want an exception then we'll need to add a unique constraint on party.name so that users cannot enter bad data. That would also need to be thoroughly investigated i.e. is there ever a case where we need / want a party with the same name, different code. My assumption is yes.
There was a problem hiding this comment.
The current implementation of findOrganization just replicates what already existing with the dynamic query. But we should ask Kelsey and Manon whether this is appopriate since it basically says "return a record if we match on code or name" but the name could match multiple records that aren't at all related to the code that was passed in. Do we want to constrain the results by the unique code as well and create new records if that doesn't match.
There was a problem hiding this comment.
This discussion was moved to ticket https://pihemr.atlassian.net/browse/OBGM-753
drodzewicz
force-pushed
the
OBGM-753-organization-not-created-assigned-when-create-supplier-location-type
branch
from
1a1093c to
3506bdd
Compare
jmiranda
deleted the
OBGM-753-organization-not-created-assigned-when-create-supplier-location-type
branch
4 participants
