-
Notifications
You must be signed in to change notification settings - Fork 87
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
feat: address field #7966
base: develop
Are you sure you want to change the base?
feat: address field #7966
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 995 Passed, 1 Skipped, 2m 46.47s Total duration (4m 16.62s time saved) |
merged with develop
Datadog ReportBranch report: ✅ 0 Failed, 710 Passed, 0 Skipped, 1m 30.29s Total duration (5m 45.35s time saved) |
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 in general, have suggestions on import references and function flow.
_id: new ObjectId().toHexString(), | ||
question: `Address question`, | ||
answerArray: [ | ||
'blockNumber_161', |
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.
Note to remove prefixes_
export const DecryptedRow = memo( | ||
({ row, attachmentDecryptionKey }: DecryptedRowProps): JSX.Element => { | ||
console.log(row) |
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.
A stray console.log
here.
} | ||
|
||
// handle postal code additions | ||
if (arr && arr[arr.length - 1]) |
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.
Would recommend avoiding implicit references. i.e., the last value of the array to be postal code
.
Perhaps array destructing would be terser here as we would be able to avoid referencing by arr[n]
const arr = [
"blockNumber_161",
"streetName_BUKIT BATOK STREET 11",
"buildingName_",
"levelNumber_",
"unitNumber_",
"postalCode_650161",
];
const [
blockNumber,
streetName,
buildingName,
levelNumber,
unitNumber,
postalCode,
] = arr;
/*
[LOG]: {
"blockNumber": "blockNumber_161",
"streetName": "streetName_BUKIT BATOK STREET 11",
"buildingName": "buildingName_",
"levelNumber": "levelNumber_",
"unitNumber": "unitNumber_",
"postalCode": "postalCode_650161"
}
*/
// handle leve;/unit number additions | ||
if (arr && arr[arr.length - 2] && arr[arr.length - 3]) { | ||
const combinedUnit = '#' + arr[arr.length - 3] + '-' + arr[arr.length - 2] | ||
arr.splice(arr.length - 3, 2, combinedUnit) |
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.
Generally, we try to avoid in-place modification as it can easily lead to mistakes.
* responses format: | ||
* ["blockNumber_161","streetName_BUKIT BATOK STREET 11","buildingName_","levelNumber_","unitNumber_","postalCode_650161"] |
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.
We can use jsdocs
to annotate input format.
* responses format: | |
* ["blockNumber_161","streetName_BUKIT BATOK STREET 11","buildingName_","levelNumber_","unitNumber_","postalCode_650161"] | |
* @param responses ["blockNumber_161","streetName_BUKIT BATOK STREET 11","buildingName_","levelNumber_","unitNumber_","postalCode_650161"] |
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.
We can also annotate return
values
@@ -49,6 +49,7 @@ export const CSP_CORE_DIRECTIVES = { | |||
'https://*.google-analytics.com', | |||
'https://*.analytics.google.com', | |||
'https://*.googletagmanager.com', | |||
'https://www.onemap.gov.sg/api/common/elastic/search?searchVal=*&returnGeom=Y&getAddrDetails=Y', |
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.
We can whitelist a more general API.
The intention of having this is to protect our users from CORS via unsolicited requests to random sites. (i.e., our web app shouldn't need to call evil.com
as part of the normal application usage).
Since we do intent to call onemap.gov.sg
we can have it to be https://www.onemap.gov.sg/api/
instead.
|
||
/** | ||
* @param responses answerArray from address field | ||
* @returns human-readable format of address, similar to handleAddressResponseDisplay (mutations.ts) |
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.
If the function's input and output are the same, could we move them to a shared utils for better reuse?
method: 'GET', | ||
headers: { | ||
'Content-Type': 'application/json', | ||
Cookie: '_toffsuid=rB8uPWZEP/wp9bsoBHUZAg==', // check this |
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.
We can likely omit this cookie.
import FormLabel from '~components/FormControl/FormLabel' | ||
import Input from '~components/Input' | ||
|
||
import { verifyAddress } from '../../../../../src/app/services/address/address.service' |
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.
FE code should not reference BE code. If there's a need we'll have to move them to shared
.
Specifically for this function, it should exist as a FE service. We can pair on creating this.
@@ -0,0 +1,44 @@ | |||
/** |
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.
See comment on frontend/src/templates/Field/Address/AddressField.tsx
to move this BE service to FE service
Problem
Closes FRM-1931
There is no current good way to collect address information for forms. Current users are either (1) creating 1 text field to grab address information or (2) creating multiple text fields to grab separated address information (postal code, block number, street name etc.). These can lead to multiple problems:
Solution
Build a new basic field focused on collecting local address information. Explicily collect
Additionally:
provide auto-complete of address population using OneMap API to get address info (block number and street name) based on postal code for ease of input for users
allow editing after auto-complete for user input flexibility
enforce type-checking for particular address fields
![image](https://private-user-images.githubusercontent.com/44297674/407176392-e9a9765e-36fe-4a5b-a30a-0346165424ce.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzg4NzE0MzksIm5iZiI6MTczODg3MTEzOSwicGF0aCI6Ii80NDI5NzY3NC80MDcxNzYzOTItZTlhOTc2NWUtMzZmZS00YTViLWEzMGEtMDM0NjE2NTQyNGNlLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMDYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjA2VDE5NDUzOVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWY4ZTBjMDgwM2Q0ZDRlZTljOWQ1MGI2Njk2NTM3YTMwYTQ3Zjg1OWIxMmIxN2I4YTYwYWE5ZTIyZWI4OTM3OTQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.AtdkDktMreJToqKEE0y3lpmocfg1QARgAo7-TSpg4Rk)
address output is handled 2 different ways depending on use case
161, BUKIT BATOK STREET 11, #1-2, SINGAPORE 650161
Breaking Changes
Features:
Screenshots:
![image](https://private-user-images.githubusercontent.com/44297674/407184645-43986d78-5cde-4302-a320-4324d1d6f185.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzg4NzE0MzksIm5iZiI6MTczODg3MTEzOSwicGF0aCI6Ii80NDI5NzY3NC80MDcxODQ2NDUtNDM5ODZkNzgtNWNkZS00MzAyLWEzMjAtNDMyNGQxZDZmMTg1LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMDYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjA2VDE5NDUzOVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTY0ZmY4YTA2M2I4YTAxOTc2ZTcyOTBkNDYwMzM1OTY0ZjM3M2E0NDAwMWM0OGQ3YmU3MWZhZTcyZTMxYjI3YWMmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.354rfPPNh3WIyTHejUx05XQ3YGcAZqRsIg0Y3ByEaZI)
![image](https://private-user-images.githubusercontent.com/44297674/407183987-0e0abb1b-28e6-44e8-908d-6a4464611275.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzg4NzE0MzksIm5iZiI6MTczODg3MTEzOSwicGF0aCI6Ii80NDI5NzY3NC80MDcxODM5ODctMGUwYWJiMWItMjhlNi00NGU4LTkwOGQtNmE0NDY0NjExMjc1LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMDYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjA2VDE5NDUzOVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWUxMTk4ZTkwYTRkZTQ4MzFmMGQyYjlmOWVhNDU1MjI1ZjVmYjgxZmMzMGYyYWNkYWIxMWI4MGE0NDVkNGY2YmQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.ZwLEii0KzGwMhjTqmN4V3nOZPqE2jJTwqAgJqlYX7qY)
Tests
(to check email notification display)