[RTI][TOOL] Enable multiple emails and contact nos for receivers#152
[RTI][TOOL] Enable multiple emails and contact nos for receivers#152ChanukaUOJ wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request transitions the receiver contact information from single string fields to JSONB arrays for both emails and contact numbers. This change involves updates to the SQL schema, seed data, Pydantic models, and the service layer. Review feedback highlights that the removal of UNIQUE constraints on these fields makes existing error handling logic in the service layer unreachable and renders the conflict-related unit tests ineffective. Additionally, it is recommended to use generic SQLAlchemy JSON types instead of dialect-specific JSONB to maintain database flexibility and to ensure contact numbers are stored without hyphens per project standards.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request migrates the receiver contact information from single string fields to JSONB lists for both emails and contact numbers across the backend and frontend. Key changes include database schema updates, Pydantic model adjustments, and the introduction of a TagInput component in the frontend for managing multiple entries. Feedback focuses on maintaining data consistency by removing hyphens from stored phone numbers, restoring regex validation for contact lists using Annotated, cleaning up unreachable error handling in the service layer, and improving type safety by avoiding 'as any' in the frontend validation logic.
| position_id: UUID = Field(foreign_key="positions.id", description="ID of the position") | ||
| institution_id: UUID = Field(foreign_key="institutions.id", description="ID of the institution") | ||
| email: Optional[str] = Field(None, unique=True, description="Email of the receiver") | ||
| emails: Optional[List[str]] = Field(default_factory=list, sa_column=Column(JSON), description="List of email of the receiver") |
There was a problem hiding this comment.
| emails: Optional[List[str]] = Field(default_factory=list, sa_column=Column(JSON), description="List of email of the receiver") | |
| emails: Optional[List[str]] = Field(default_factory=list, sa_column=Column(JSON), description="List of emails of the receiver") |
| emails: Optional[List[str]] = Field(default_factory=list, sa_column=Column(JSON), description="List of email of the receiver") | ||
| address: Optional[str] = Field(None, description="Address of the receiver") | ||
| contact_no: Optional[str] = Field(None, unique=True, description="Contact number of the receiver") | ||
| contact_nos: Optional[List[str]] = Field(default_factory=list, sa_column=Column(JSON), description="List of contact number of the receiver") |
There was a problem hiding this comment.
| contact_nos: Optional[List[str]] = Field(default_factory=list, sa_column=Column(JSON), description="List of contact number of the receiver") | |
| contact_nos: Optional[List[str]] = Field(default_factory=list, sa_column=Column(JSON), description="List of contact numbers of the receiver") |
| ((SELECT id FROM positions WHERE name = 'Department Head' LIMIT 1), (SELECT id FROM institutions WHERE name = 'Public Service Commission' LIMIT 1), 'head.psc@gov.lk', 'Gampaha', NULL); | ||
| INSERT INTO receivers (position_id, institution_id, emails, address, contact_nos) VALUES | ||
| ((SELECT id FROM positions WHERE name = 'Information Officer' LIMIT 1), (SELECT id FROM institutions WHERE name = 'Ministry of Health' LIMIT 1), '["io.health@gov.lk"]', NULL, '["0112444555"]'), | ||
| ((SELECT id FROM positions WHERE name = 'Designated Officer' LIMIT 1), (SELECT id FROM institutions WHERE name = 'Department of Education' LIMIT 1), '["do.edu@gov.lk"]', NULL, '[]'), |
There was a problem hiding this comment.
Can we not keep empty values for contact nos or emails as NULL? Does it have to be an empty list?
| position_id: Optional[UUID] = Field(None, alias="positionId", description="ID of the position") | ||
| institution_id: Optional[UUID] = Field(None, alias="institutionId", description="ID of the institution") | ||
| email: Optional[EmailStr] = Field(None, description="Email of the receiver") | ||
| emails: Optional[List[EmailStr]] = Field(None, description="List of receiver Emails") |
There was a problem hiding this comment.
| emails: Optional[List[EmailStr]] = Field(None, description="List of receiver Emails") | |
| emails: Optional[List[EmailStr]] = Field(None, description="List of receiver emails") |
There was a problem hiding this comment.
This file is called receiver.py but the file in response_models is receivers.py
| email: Optional[str] = Field( | ||
| None, description="Email of the receiver" | ||
| emails: Optional[List[str]] = Field( | ||
| None, description="Email list of the receiver" |
There was a problem hiding this comment.
| None, description="Email list of the receiver" | |
| None, description="List of receiver emails" |
| serialization_alias="contactNo", | ||
| description="Contact number of the receiver" | ||
| serialization_alias="contactNos", | ||
| description="Contact number list of the receiver" |
There was a problem hiding this comment.
| description="Contact number list of the receiver" | |
| description="List of receiver contact numbers" |
| position_id: UUID = Field(..., alias="positionId", description="ID of the position") | ||
| institution_id: UUID = Field(..., alias="institutionId", description="ID of the institution") | ||
| email: Optional[EmailStr] = Field(None, description="Email of the receiver") | ||
| emails: Optional[List[EmailStr]] = Field(None, description="List of receiver Emails") |
There was a problem hiding this comment.
| emails: Optional[List[EmailStr]] = Field(None, description="List of receiver Emails") | |
| emails: Optional[List[EmailStr]] = Field(None, description="List of receiver emails") |
| position: PositionShortResponse = Field(..., description="Position object of the receiver") | ||
| institution: InstitutionShortResponse = Field(..., description="Institution object of the receiver") | ||
| email: Optional[str] = Field(None, description="Email of the receiver") | ||
| emails: Optional[List[str]] = Field(None, description="Email list of the receiver") |
There was a problem hiding this comment.
| emails: Optional[List[str]] = Field(None, description="Email list of the receiver") | |
| emails: Optional[List[str]] = Field(None, description="List of receiver emails") |
| emails: Optional[List[str]] = Field(None, description="Email list of the receiver") | ||
| address: Optional[str] = Field(None, description="Address of the receiver") | ||
| contact_no: Optional[str] = Field(None, serialization_alias="contactNo", description="Contact number of the receiver") | ||
| contact_nos: Optional[List[str]] = Field(None, serialization_alias="contactNos", description="Contact number list of the receiver") |
There was a problem hiding this comment.
| contact_nos: Optional[List[str]] = Field(None, serialization_alias="contactNos", description="Contact number list of the receiver") | |
| contact_nos: Optional[List[str]] = Field(None, serialization_alias="contactNos", description="List of receiver contact numbers") |
No description provided.