Skip to content
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

[api][web][sims-120] Add institution location form and API to save the data to sims data base after form validation #425

Merged
merged 4 commits into from
May 7, 2021

Conversation

ann-aot
Copy link
Contributor

@ann-aot ann-aot commented May 7, 2021

…save the data to sims data base after form validation

…save the data to sims data base after form validation
@ann-aot ann-aot changed the title [api][web][sim-120] Add institution location form and API to save the data to sims data base after form validation [api][web][sims-120] Add institution location form and API to save the data to sims data base after form validation May 7, 2021
@@ -16,3 +16,4 @@ COMMENT ON TABLE institutions_users IS 'This is a pivot table to capture the rel
COMMENT ON COLUMN institutions_users.id IS 'Auto-generated sequential primary key column';
COMMENT ON COLUMN institutions_users.user_id IS 'Foreign key reference to users table users';
COMMENT ON COLUMN institutions_users.institution_id IS 'Foreign key reference to users table institutions';
1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider not including this file in the PR if the only change is the line break.

@@ -29,7 +29,6 @@ export default {
const formDefinition = await ApiClient.DynamicForms.getFormDefinition(
props.formName,
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider not including this file in the PR if the only change is the line break.

locationName: string;
postalZipCode: string;
provinceState: string;
submit: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider removing the submit from here since it is not part of the business model.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, at any point we won't be using the submit variable right? okay I will remove it

locationName: string;
postalZipCode: string;
provinceState: string;
submit: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider removing the submit from here since it is not part of the business model.

async create(
@Body() payload: InstitutionLocationType,
@UserToken() userToken: IUserToken,
): Promise<GetInstitutionLocationDto> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that this method should return a Promise since it is returning only the id.


@Post()
async create(
@Body() payload: InstitutionLocationType,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are trying to keep all objects received/returned by the API as DTO. I would recommend moving this to the DTO level.

Copy link
Contributor

@fwpushan fwpushan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check why test run is unsuccessful.

provinceState: string,
postalZipCode: string,
country: string,
applicationId: string,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any particular reason these properties be present here?

  • applicationId: string,
  • pid: string,
  • process_pid: string

Copy link
Collaborator

@andrewsignori-aot andrewsignori-aot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @ann-aot, looks good 👍

@ann-aot ann-aot merged commit 11cbfe1 into main May 7, 2021
@ann-aot ann-aot deleted the feature/institution-location-creation-poc-120 branch May 7, 2021 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants