-
Notifications
You must be signed in to change notification settings - Fork 6
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
5452 User Management Console: Manage organizations for PLR #116
base: master
Are you sure you want to change the base?
Conversation
connected backend to database
simplified loadOrganizations in user detail and user search. organization controller now takes in database name from the application.yaml file. OrganizationsController: col names are variables, taken from application.yaml files.
backend/src/main/java/ca/bc/gov/hlth/mohums/controller/OrganizationsController.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/ca/bc/gov/hlth/mohums/controller/OrganizationsController.java
Outdated
Show resolved
Hide resolved
organizations now load on the screen as soon as it opens. deleted button to "search" for organizations and button to download results. Removed mentions of organization update.
backend/src/main/java/ca/bc/gov/hlth/mohums/controller/OrganizationsController.java
Outdated
Show resolved
Hide resolved
fixed issue where the format that user components use to save organizations is not the same with how they use the organizations
removed orgupdate related code + edited formatting
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.
Hi Valery, just a few minor tweaks, and it's done for now. All the remaining work is out of scope.
You've done a great job on this, and I can't find anything you missed or I would have done differently. Thank you for making it concise: it's easy to understand and review.
I'm glad it's consistent with the rest of the codebase. While it's possible you copied some of our bad habits, that's preferable to making too many changes, because it's easier to fix a codebase where everything is consistent (even consistently wrong) than one where every developer is doing their own thing. That said, please let us know if you see room for improvement, as we're fairly new to Vue.js.
backend/src/main/java/ca/bc/gov/hlth/mohums/controller/OrganizationsController.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/ca/bc/gov/hlth/mohums/controller/OrganizationsController.java
Outdated
Show resolved
Hide resolved
public void createOrganization(@RequestBody JSONObject body) throws SQLException { | ||
jdbcTemplate.update("insert into " + database + " ("+organization_id + ", " + organization_name + ") values (?,?)", | ||
body.get("id"), body.get("name")); | ||
} |
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'll mention something here, but don't bother changing it because hopefully we'll get to use AWS cloud storage for this which will use a different API anyhow.
Building SQL query strings using user-provided values is vulnerable to SQL injection attacks. You can read about the attack and prevention strategies at that link.
jdbcTemplate.update("insert into " + database + " ("+organization_id + ", " + organization_name + ") values (?,?)", | ||
body.get("id"), body.get("name")); | ||
} | ||
|
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.
In the future we'll probably add more validation logic. You've validated the org ID and name on the client-side (browser), but typically we'd also want to validate inputs on the backend and database. No change necessary for now, it's out of scope, but it's just something to keep in mind.
(e.g. Using the browser's network tools, it's possible to edit the HTTP POST request and create an organization without a name.)
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.
"Approved", but do not merge as we need to implement storage.
Making the database:
The backend's application.yaml file has a few fields that should be filled based on this database: