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.

Sign up for GitHub

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

Jump to bottom

Added extension field with a max size of 254 #421

Merged
merged 5 commits into from
Sep 24, 2024
Merged

Added extension field with a max size of 254 #421

merged 5 commits into from
Sep 24, 2024

Conversation

jcadam14
Copy link
Contributor

Closes #420

  • Added Alembic script to add the extension column to contact_info
  • Added extension field to the contact info DAO and DTO
  • Added pytests to test the two things above.
  • Field is optional

@jcadam14 jcadam14 self-assigned this Sep 24, 2024
@jcadam14 jcadam14 linked an issue Sep 24, 2024 that may be closed by this pull request
Add extension to ContactInfo #420
Closed
@github-actions GitHub Actions
Copy link

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)		Lines missing
  src/sbl_filing_api/entities/models
  dao.py
  dto.py
Project Total  

This report was generated by python-coverage-comment-action

hkeeler
hkeeler reviewed Sep 24, 2024
View reviewed changes
Copy link
Member

@hkeeler hkeeler left a comment

Choose a reason for hiding this comment

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

Overall looks good. Just the one naming nitpick and a question.

src/sbl_filing_api/entities/models/dao.py Outdated
@@ -90,6 +90,7 @@ class ContactInfoDAO(Base):
hq_address_zip: Mapped[str] = mapped_column(String(5))
email: Mapped[str]
phone_number: Mapped[str]
extension: Mapped[str] = mapped_column(String(254), nullable=True)
Copy link
Member

Choose a reason for hiding this comment

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

Could you make this phone_ext. I think that's what we agreed to and the last meeting, and I think what @shindigira is coding to on cfpb/sbl-frontend#961.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

src/sbl_filing_api/entities/models/dto.py Outdated
@@ -60,6 +60,7 @@ class ContactInfoDTO(BaseModel):
hq_address_zip: str
email: str
phone_number: str
extension: str | None = Field(None, max_length=254)
Copy link
Member

Choose a reason for hiding this comment

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

What happens when max_length is exceeded? I can see from the test that it raises a 422, which seems appropriate. Does Pydantic give a meaningful error too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You'll get a response json that looks like:
{'error_name': 'Request Validation Failure', 'error_detail': "[{'type': 'string_too_long', 'loc': ('body', 'phone_ext'), 'msg': 'String should have at most 254 characters', 'input': '12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890', 'ctx': {'max_length': 254}}]"}

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. OK, I think that is fine as far as this PR goes. I don't love these big Pydantic barf error messages, but that's an issue for another day. 😄

@jcadam14 jcadam14 merged commit fb78e33 into main Sep 24, 2024
5 checks passed
@shindigira shindigira mentioned this pull request Sep 25, 2024
Merged
shindigira added a commit to cfpb/sbl-frontend that referenced this pull request Sep 30, 2024
@shindigira
[Point of Contact] Phone extension field (#961)
7368475 
## Note
Be sure the pull the latest from
[_sbl-filing_](cfpb/sbl-filing-api#421) via
`yarn update`

closes #945

## Changes

- feat(Point of Contact): Added Phone Extension field to Point of
Contact Field
- chore(Point of Contact): Phone Extension breakpoint of 600px
- feat(Sign and Submit): Added Phone Extension field to Verification
field
- chore(Sign and Submit): Matched order of fields to be the same as
Point of Contact
- feat(e2e): Added Phone Extension field to e2e test
- style(DisplayField): added `work-break: break-all;`
- content(Sign and Submit): added 'Extension'

## How to test this PR

### First Test
1. Upload a filing and navigate all the way to _Point of Contact_
2. Verify the Phone Extension in both mobile and desktop (901 px and
_above_)
3. Open the Network tab, submit and verify the request and response
jsons contain the phone extension (i.e. `phone_ext`)

### Second Test
1. Navigate all the way to _Sign and Submit_
2. Verify the `DisplayField` contains the submitted phone extension


### Third Test
1. Run `yarn run test:e2e`
2. Run the _sign-and-submit_ test to ensure it passes

## Screenshots
| PoC (Desktop) | PoC (Mobile) | Sign and Submit |
| -------- | ------- | ------- |
|<img width="533" alt="Screenshot 2024-09-30 at 9 07 06 AM"
src="https://github.com/user-attachments/assets/2210a4d8-e088-4cad-9d78-392aabc0de41">|<img
width="516" alt="Screenshot 2024-09-30 at 9 07 20 AM"
src="https://github.com/user-attachments/assets/cc203268-85bf-4826-8adc-d4740d1a91ea">|<img
width="632" alt="Screenshot 2024-09-30 at 2 18 43 PM"
src="https://github.com/user-attachments/assets/d18a1416-1cbc-494f-a561-d3cd44df22ba">|
billhimmelsbach pushed a commit to cfpb/sbl-frontend that referenced this pull request Oct 1, 2024
@shindigira @billhimmelsbach
[Point of Contact] Phone extension field (#961)
1b58910 
Be sure the pull the latest from
[_sbl-filing_](cfpb/sbl-filing-api#421) via
`yarn update`

closes #945

- feat(Point of Contact): Added Phone Extension field to Point of
Contact Field
- chore(Point of Contact): Phone Extension breakpoint of 600px
- feat(Sign and Submit): Added Phone Extension field to Verification
field
- chore(Sign and Submit): Matched order of fields to be the same as
Point of Contact
- feat(e2e): Added Phone Extension field to e2e test
- style(DisplayField): added `work-break: break-all;`
- content(Sign and Submit): added 'Extension'

1. Upload a filing and navigate all the way to _Point of Contact_
2. Verify the Phone Extension in both mobile and desktop (901 px and
_above_)
3. Open the Network tab, submit and verify the request and response
jsons contain the phone extension (i.e. `phone_ext`)

1. Navigate all the way to _Sign and Submit_
2. Verify the `DisplayField` contains the submitted phone extension

1. Run `yarn run test:e2e`
2. Run the _sign-and-submit_ test to ensure it passes

| PoC (Desktop) | PoC (Mobile) | Sign and Submit |
| -------- | ------- | ------- |
|<img width="533" alt="Screenshot 2024-09-30 at 9 07 06 AM"
src="https://github.com/user-attachments/assets/2210a4d8-e088-4cad-9d78-392aabc0de41">|<img
width="516" alt="Screenshot 2024-09-30 at 9 07 20 AM"
src="https://github.com/user-attachments/assets/cc203268-85bf-4826-8adc-d4740d1a91ea">|<img
width="632" alt="Screenshot 2024-09-30 at 2 18 43 PM"
src="https://github.com/user-attachments/assets/d18a1416-1cbc-494f-a561-d3cd44df22ba">|
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hkeeler hkeeler hkeeler approved these changes

@billhimmelsbach billhimmelsbach Awaiting requested review from billhimmelsbach

@nargis-sultani nargis-sultani Awaiting requested review from nargis-sultani

Assignees

@jcadam14 jcadam14

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add extension to ContactInfo
2 participants
@jcadam14 @hkeeler