-
Notifications
You must be signed in to change notification settings - Fork 13
#1702 - Allow institution to specify custom regulatory body #1763
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
#1702 - Allow institution to specify custom regulatory body #1763
Conversation
Worked on displaying other regulating institution in the Institution Profile
…y_custom_regulatory_body' into 1702_allow_institution_to_specify_custom_regulatory_body
Removed my changes related to prettier from .eslintrc.js which I mistakenly committed previously
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.
please add description and UI screenshots
...es/packages/backend/apps/db-migrations/src/sql/Institution/Add-col-other-regulating-body.sql
Outdated
Show resolved
Hide resolved
sources/packages/backend/libs/sims-db/src/entities/institution.model.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/apps/api/src/route-controllers/institution/models/institution.dto.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/apps/api/src/services/institution/institution.service.model.ts
Outdated
Show resolved
Hide resolved
| -- Add other_regulating_body column to institutions table | ||
| ALTER TABLE sims.institutions ADD COLUMN IF NOT EXISTS other_regulating_body VARCHAR(100); | ||
|
|
||
| COMMENT ON COLUMN sims.institutions.other_regulating_body IS 'Allows the user to enter the description if the option other is selected from the regulating body dropdown'; No newline at end of file |
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.
Please add a period at the end of the sentence.
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.
Thanks. Will do.
andrewsignori-aot
left a comment
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.
Nice and great first PR. @ann-aot and @dheepak-aot covered pretty much everything. I will be waiting for the changes to review again.
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.
Great job @sh16011993 . The other devs already addressed the issues. One thing I noticed: please let the person who commented to resolve the conversation.
Actually I was talking to @ann-aot at the time I resolved the specific conversation with her. We discussed on the call and then resolved it. |
@andrewsignori-aot I have updated the code with the review comments. |
| @IsNotEmpty() | ||
| regulatingBody: string; | ||
| @ValidateIf((e) => e.regulatingBody === "other") | ||
| @IsNotEmpty() |
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.
Same max length validation needs to added here as well.
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 apologize, I missed it.
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.
No problem.
| regulatingBody: string; | ||
|
|
||
| /** | ||
| * Other regulating body |
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.
|
Thanks for doing the changes. Added 2 more comments. |
dheepak-aot
left a comment
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.
Thanks for doing the changes @sh16011993. Nice beginning.
andrepestana-aot
left a comment
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.
LGTM
sources/packages/forms/src/form-definitions/institutionprofilecreation.json
Outdated
Show resolved
Hide resolved
sources/packages/forms/src/form-definitions/institutionprofile.json
Outdated
Show resolved
Hide resolved
sources/packages/backend/apps/api/src/route-controllers/institution/models/institution.dto.ts
Show resolved
Hide resolved
andrewsignori-aot
left a comment
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.
Thanks for doing the changes. Please take a look on the last minor comments.
|
Kudos, SonarCloud Quality Gate passed!
|
andrewsignori-aot
left a comment
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.
Thanks for doing the changes, it looks good 👍
ann-aot
left a comment
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.
Good start and good work @sh16011993 👍 Thanks for doing the changes









institutionprofilecreation.jsonform to add Other as a value to the Institution regulating body dropdown and the Other Regulating Body text field.other_regulating_bodycolumn to the database and created the appropriate database migration scripts along with the required SQL script to add theother_regulating_bodycolumn and also the SQL script to remove the column if the migration needs to be reverted.institutionprofile.jsonform to display Other regulating body field in the Institution profile on the AEST portal.Updating the field:

After the update:
