example data hook #52

elisadinsmore · 2022-11-17T15:19:02Z

No description provided.

codesandbox · 2022-11-17T15:19:04Z

Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

paddymul · 2022-11-17T15:20:34Z

examples/FieldHooks/ComputeFieldHooks/removeExtraSpaces.ts

+const removeExtraSpaces = new Sheet('removeExtraSpaces', {
+  departmentName: TextField({
+    compute: (value: any) => {
+      return value.replace(/\s{2,}/g, ' ')


My regex is rusty, does this only remove space of two or more. maybe add a comment

paddymul · 2022-11-17T15:22:13Z

examples/FieldHooks/ComputeFieldHooks/removeExtraSpaces.ts

+      return value.replace(/\s{2,}/g, ' ')
+    },
+  }),
+})


I would like to see tests that show/verify what happens for the following string. Reason about "is this what you really want to happen"
123456789012345
" asdf"
"asdf "
" as df" - does this replace only one sequence of spaces?
"Paddy Mullen" - does this remove the single character

paddymul · 2022-11-17T15:26:11Z

to get the full testing stuff (WIP from other branches) run the following commands

git pull
git checkout feat/simple-constraints # checkout this branch, will be setup to automatically track from remote
git checkout feat/DataHookExamples # go back to your branch
git checkout feat/simple-cosntraints -- jest.config.js src/utils/testing/SheetTester.ts # pull just those files into your branch
git commit jest.config.js src/utils/testing/SheetTester.ts -m "updated SheetTester"

run tests with
npm run test
or npm run test remove to run any testname matching remove*

paddymul · 2022-11-22T18:41:00Z

examples/FieldHooks/ComputeFieldHooks/RemoveSymbolsCompute.spec.ts

+import { SheetTester } from '../../../src/utils/testing/SheetTester'
+
+//This hook checks for and removes special characters in a string using RegEx.
+const removeSymbolsCompute = new Sheet('removeSymbolsCompute', {


I would rename this to RemoveSymbolsSheet, with a sheetname of RemoveSymbolsSheet

elisadinsmore · 2022-12-19T18:38:23Z

examples/hooks/RecordHooks/conditionallyModifyField.spec.ts

+  {
+    recordCompute: (record) => {
+      //ensuring the accountId is returned as a string
+      const accountNum = record.get('accountId') as string


@paddymul I needed to ensure I was returning the accountId field as a string here otherwise TypeScript was concerned that the TPrimitive type could be a number, boolean, string, or null. Is there a better way to ensure this is a string to be able to test properly?

paddymul · 2022-12-21T00:43:32Z

examples/hooks/FieldHooks/ComputeFieldHooks/dateFormat.spec.ts

+  (update: string) =>
+  (value: string): string => {
+    try {
+      return format(new Date(value), update)
+    } catch (err) {
+      return value
+    }
+  }
+


I don't think this code actually runs. it looks like a function that returns a function. Are you sure this is what you wanted?

oh, I see, that's waht you meant to do. Well done, you should mention this.

I'll be honest, this is code from @hansjhoffman that I rewrote for this example so I don't know how to best explain it 😆

This is also an example that I might remove with the launch of SmartDateField, since that would cover the basically same formatting changes this is capable of handling.

elisadinsmore · 2022-12-21T13:28:43Z

examples/hooks/FieldHooks/ValidateFieldHooks/formatPhoneNumber.spec.ts

+    const res = await testSheet.testRecord(inputRow)
+    expect(res).toMatchObject(expectedOutputRow)
+  })
+


@paddymul this hook is waiting on the ability to test the messages that are returned, since it's a Validate hook. That's why my tests are incomplete here.

paddymul · 2022-12-28T16:08:48Z

examples/hooks/FieldHooks/ComputeFieldHooks/dateFormat.spec.ts

+    console.log(new Date(inputRow.joinDate))
+    const expectedOutputRow = { joinDate: '12/31/2020' }
+    const res = await testSheet.testRecord(inputRow)
+    expect(res).toMatchObject(expectedOutputRow)


Stylistic thing, I don't like defining the expected input and output so far apart.

Instead I would have the last lines as this

const inputRow = { joinDate: '12-31-2020' } const expectedOutputRow = { joinDate: '12/31/2020' } expect(res).toMatchObject(expectedOutputRow)

or even

expect( await testSheet.testRecord({joinDate:'12-31-2020' }).toMatchObject({joinDate:'12/31/2020'})

Generally for a bunch of test cases, you want the important code that differs between the cases to jump out at you, the boilerplate should fade away.

paddymul · 2022-12-28T16:12:23Z

examples/hooks/FieldHooks/ComputeFieldHooks/dateFormat.spec.ts

+    const inputRow = { joinDate: '14 Jan 2022' }
+    const expectedOutputRow = { joinDate: '01/14/2022' }
+    const res = await testSheet.testRecord(inputRow)
+    expect(res).toMatchObject(expectedOutputRow)


Ideally we would have a method on sheetTester so you can test an individual field, not even caring about the name of the field. so you could just say

fieldTester = new FieldTester(TextField({ compute: formatDate('MM/dd/yyyy'), })) expect(transformField('14 Jan 2022')).toBe('01/14/2022')

paddymul · 2022-12-28T16:21:28Z

examples/hooks/FieldHooks/ComputeFieldHooks/dateFormat.spec.ts

+    const expectedOutputRow = { joinDate: '12242022' }
+    const res = await testSheet.testRecord(inputRow)
+    expect(res).toMatchObject(expectedOutputRow)
+  })


I would change your format function for this example, so that if it gets an unexpected input, it throws the error. Don't swallow errors and continue processing.

If any field hook throws an exception, the platform-sdk, will keep the original value of the field (double check the original value is kept, not the value to that point), and set a message on that cell of the error thrown.

I would rewrite your compute function so that it throws an error on unexpected input, then use testSheet.testMessage to verify you are getting the message (related to the error thrown) you expect for this field and input.

Take a look here https://github.com/FlatFilers/platform-sdk-starter/blob/main/examples/fields/SmartDateField.spec.ts#L136-L159 but make it simpler and more explicit for this field

paddymul · 2022-12-28T16:35:48Z

examples/hooks/FieldHooks/ComputeFieldHooks/removeExtraSpaces.spec.ts

+    expect(res).toMatchObject(expectedOutputRow)
+  })
+
+  test('Test set of numbers', async () => {


please change the name. this isn't a set of numbers

paddymul · 2022-12-28T16:40:13Z

examples/hooks/RecordHooks/conditionallyEmptyField.spec.ts

+  'Test Sheet',
+  {
+    paymentStatus: TextField(),
+    amount: NumberField(),


Side note. we are encouraging all Fields to be called with at least an empty object {} so this becomes
NumberField({})

paddymul · 2022-12-28T16:43:19Z

examples/hooks/RecordHooks/conditionallyEmptyField.spec.ts

+    amount: NumberField(),
+  },
+  {
+    //because this hook is comparing multiple fields and both modifying a field and adding a message, it must be run as a recordHook rather than a fieldHook


I would recommend:
//because this hook is comparing multiple fields and both modifying a field, it must be run as a recordHook rather than a fieldHook

Adding a message at the same time shouldn't be promoted as a reason to use recordCompute... The only way you could properly set the warning message for 'payment' status is by looking at 'paymentStatus' and 'amount', that's why you want recordCompute.

paddymul · 2022-12-28T16:48:07Z

examples/hooks/RecordHooks/conditionallyModifyField.spec.ts

+  },
+  {
+    recordCompute: (record) => {
+      //ensuring the accountId is returned as a string


because accountID is a TextField we know it will be string or possibly null. Given our existing typing implemenation, we only know that accountID could be one of null, string, number, date, boolean. Try this code instead:

//make sure we actually have a string, then continue processing const accountID = record.get('accountID') if(typeof accountID === 'string') { // this type guard assures typescript that we are dealing with a string if (FIELDS['accountId'].regex.test(accountNum) === false) { record.set('accountStatus', false) record.addError('accountId', FIELDS.accountId.message) } else { record.set('accountStatus', true) }

I would recommend explicitly handling the else case, where accountID is null.

paddymul · 2022-12-28T16:51:13Z

examples/hooks/RecordHooks/conditionallyModifyField.spec.ts

+  const testSheet = new SheetTester(TestWorkbook, 'testSheet')
+  test('21 Characters', async () => {
+    // for this inputRow
+    const inputRow = { accountId: '1234567890asdfghjkjhg', accountStatus: true }


These are tests that acutally need testRecord because they deal with multiple fields.

example data hook

7e1f352

paddymul reviewed Nov 17, 2022

View reviewed changes

elisadinsmore added 6 commits November 17, 2022 13:37

added tests

cbde009

updated SheetTester

16be29d

update removeExtraSpaces file

efd4983

remove old file

c3d9fd6

added comments

81ceaee

New Hook Example

a4001f5

paddymul reviewed Nov 22, 2022

View reviewed changes

elisadinsmore added 4 commits November 22, 2022 15:00

reorganized and new hook example

3d5298b

adding partial validate example, missing tests

0888557

add record hook example

fd54de0

Conditionally Modify Field Example

957fb1b

elisadinsmore commented Dec 19, 2022

View reviewed changes

paddymul reviewed Dec 21, 2022

View reviewed changes

elisadinsmore commented Dec 21, 2022

View reviewed changes

paddymul reviewed Dec 28, 2022

View reviewed changes

merged in latest main

2bde1a5

example data hook #52

Are you sure you want to change the base?

example data hook #52

Uh oh!

Conversation

elisadinsmore commented Nov 17, 2022

Uh oh!

codesandbox bot commented Nov 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paddymul Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paddymul commented Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

paddymul Nov 17, 2022 •

edited

Loading

paddymul commented Nov 17, 2022 •

edited

Loading