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

#2134 - Increase the max length of operating and legal operating name to 250 #2140

Conversation

dheepak-aot
Copy link
Collaborator

Increase the max length in formio and db for operating name and legal operating name

  • DB Migations
  • Form IO validation update.

Institution Creation exceeding 250 characters

image

Institution Creation within 250 characters

image

@dheepak-aot dheepak-aot self-assigned this Jul 27, 2023
@dheepak-aot dheepak-aot added DB DB migration involved Form.io Form IO definitions changed. Hotfix PR created for hotfix labels Jul 27, 2023
Copy link
Collaborator

@guru-aot guru-aot left a comment

Choose a reason for hiding this comment

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

LGTM, nice work @dheepak-aot

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.

Nice work, looks good 👍

@@ -395,7 +396,7 @@
"validate": {
"required": true,
"minLength": "",
"maxLength": 100,
"maxLength": 250,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice catch updating this form also.
Can we have an AC added to the ticket to make it clear that the update process by the Ministry is also a scenario to be tested by the business?
@JasonCTang FYI

@@ -346,7 +346,8 @@
"addons": [],
"inputType": "text",
"id": "eiloy9e",
"defaultValue": ""
"defaultValue": "",
"isNew": false
Copy link
Contributor

@ann-aot ann-aot Jul 27, 2023

Choose a reason for hiding this comment

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

just wondering, are we not adding the max length to legalOperatingName. same for the other form

Copy link
Collaborator

@andrewsignori-aot andrewsignori-aot Jul 27, 2023

Choose a reason for hiding this comment

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

We consider that @ann-aot but the legalOperatingName is displayed here for info only. The value is retrieved from the token directly, not from the UI, so there is no real point in adding a validation here since we have the confirmation about the 250 characters.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the value is read from the response of bceid web service.

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@andrewsignori-aot , I just thought twice, we are re-using the same form(institution profile) to create institution by ministry right. should we use the validation then?

image

Copy link
Contributor

Choose a reason for hiding this comment

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

mm, ya... but I was wondering, what if they increased the length (and we have 250), and now there is a new institution with more than 250 characters of legal name, then the same error which we saw today will happen again right? but if we add the max length here, at least the user will not be able to submit due to validation and the user will at least know the issue right?

but, I am good with what we have in the PR, just sharing my thoughts

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that makes sense @dheepak-aot, nice catch.

Copy link
Collaborator

@sh16011993 sh16011993 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thank You for the quick fix.

Copy link
Contributor

@ann-aot ann-aot left a comment

Choose a reason for hiding this comment

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

NIce work @dheepak-aot and nice catch

@github-actions
Copy link

Backend Unit Tests Coverage Report

Totals Coverage
Statements: 17.89% ( 2127 / 11892 )
Methods: 8.31% ( 126 / 1516 )
Lines: 20.68% ( 1863 / 9009 )
Branches: 10.1% ( 138 / 1367 )

@github-actions
Copy link

E2E Workflow Workers Coverage Report

Totals Coverage
Statements: 49.81% ( 267 / 536 )
Methods: 41.56% ( 32 / 77 )
Lines: 55.33% ( 218 / 394 )
Branches: 26.15% ( 17 / 65 )

@github-actions
Copy link

E2E Queue Consumers Coverage Report

Totals Coverage
Statements: 71% ( 399 / 562 )
Methods: 61.97% ( 44 / 71 )
Lines: 72.97% ( 351 / 481 )
Branches: 40% ( 4 / 10 )

@github-actions
Copy link

E2E SIMS API Coverage Report

Totals Coverage
Statements: 50.18% ( 3543 / 7061 )
Methods: 45.63% ( 418 / 916 )
Lines: 55.38% ( 2915 / 5264 )
Branches: 23.84% ( 210 / 881 )

@dheepak-aot dheepak-aot merged commit 1afe022 into hotfix/v1.1.1 Jul 27, 2023
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@dheepak-aot dheepak-aot deleted the fix/#2134-update-operating-and-legal-operating-name-length branch August 1, 2023 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DB DB migration involved Form.io Form IO definitions changed. Hotfix PR created for hotfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants