COR-487: search page #395

internetti · 2022-03-28T10:56:15Z

list all recipients in table on search page.

used the Recipient.get function in the Recipient.list function, to be able to show the names in the table. turned off the fetching of the subforms for that usecase.

frontend/apps/core-app/src/screens/RecipientListScreen/RecipientListScreen.container.tsx

neb42

I think the data loading and table data is wrong here. You're currently getting the ancestors of the leaf node and showing a table for each (with the wrong fields for them). I think it'll be best to merge in the leaf node pr and then change it to do this:

Read leaf node forms from context
Select first leaf node from list (temporary until we have a UI to choose between leaf nodes)
List records for leaf node
Render a single table for leaf node

We can also probably look at rendering less table columns. Maybe something like 5 columns, starting at the root of FormWithRecord and working down.

I don't think the table is growing to the full width. If you use a form with less columns they wont grow to fit the space.

neb42 · 2022-05-03T08:15:45Z

frontend/packages/core-api-client/src/types/client/Record.ts

+export type RecordListRequest = {
+  databaseId: string;
+  formId: string;
+  subforms?: boolean;


I'd change this to something like fetchSubforms

neb42 · 2022-05-03T08:18:10Z

frontend/packages/core-api-client/src/types/client/Recipient.ts

 import { RecordLookup } from './Record';

 export type Recipient = Record;
 export type RecipientList = RecordList;
+export type RecipientListResponse = Response<FormLookup, RecipientList>;


I don't think this is used

neb42 · 2022-05-03T08:19:48Z

frontend/packages/core-api-client/src/testUtils/mockData/Record.ts

+  id: `record${index}`,
+  values: form.fields.map((f) => {
+    return {
+      value: `value-${f.id}`,
+      fieldId: f.id,
+    };
+  }),
+  formId: form.id,
+  databaseId: 'databaseId',


For these fake ids it'd be good to be consistent with the format. My preference is for foo-${id} for ids and foo ${name} for named things (eg. name, description), but happy with whatever as long as it's consistent.

neb42 · 2022-05-03T08:29:52Z

frontend/packages/core-api-client/src/client/__tests__/Recipient.test.ts

+      });
+    });
+
+    describe('error', () => {


Error test for when this.get fails

neb42 · 2022-05-03T08:35:01Z

...screens/RecipientRegistrationScreen/__tests__/RecipientRegistrationScreen.component.test.tsx

@@ -29,7 +29,7 @@ jest.mock('../../../components/Recipient/RecipientViewer', () => {
  };
 });

-const makeFormWithRecord = (i: number): FormWithRecord<Recipient> => {
+export const makeFormWithRecord = (i: number): FormWithRecord<Recipient> => {


Should this be moved to the mock data rather than just exported?

neb42 · 2022-05-03T09:29:00Z

frontend/apps/core-app/src/components/RecipientListTable/RecipientListTableContainer.tsx

@@ -10,11 +10,13 @@ import { RecipientListTableEntry, SortedFilteredTable } from './types';

 type Props<T extends Record> = {
  data: FormWithRecord<T>[][];
+  form: FormDefinition;


I don't think this needs to be passed in. We can calculate the columns from FormWithRecord

thought so initially too, but data might be an empty array, then we should still be able to show the table header, need the form for that

For that state I'd typically replace the table with some form of placeholder text (the subform table does something like this)

neb42 · 2022-05-04T09:35:54Z

frontend/packages/core-api-client/src/client/Recipient.ts

+  public list = async ({
+    formId,
+    databaseId,
+  }: FormLookup): Promise<FormWithRecord<Recipient>[][]> => {


I think this change is worth doing outside of this pr as a refactor later.

I'm thinking FormWithRecord<Recipient>[][] isn't the best return type here as we're duplicating a lot of data as the forms are the same for each record. We could probably reduce the api calls made by doing a list for each form and then connecting the records together.

Something like:

{ forms: FormDefinition[]; records: Record<string, Record>[]; // Different Record types }

or

{ forms: FormDefinition[]; records: Record[][]; }

or maybe FormWithRecord<Recipient>[][] is the best return type as it's used by the frontend widely, but how we build it could be changed.

yeah, the current structure is definitely not ideal, we should do this when we find time

neb42

I haven't tested this yet as I'm having local tunnel issues

neb42 · 2022-05-05T11:40:29Z

frontend/apps/core-app/src/components/RecipientListTable/RecipientListTableRow.tsx

-  onRowClick: (id: string) => void;
+  handleRowClick: () => void;


Why have you renamed this prop? I think the name was correct.

I'd typically name event function props onSomething because it is what happens when that event occurs. The function being passed in I'd typically name handleSomething as it is handling the event.

neb42 · 2022-05-05T12:10:54Z

frontend/apps/core-app/src/components/RecipientListTable/RecipientListTableComponent.tsx

-        <VStack>
-          <FlatList data={rows} renderItem={renderRow} />
-        </VStack>
+    <Box mb="63px">


There's a few spacing things in here that should use a spacing token. Here either mb={16} or mb="nrc_?". I do think we should move our nrc spacing to the proper spacing naming though.

Also I think if the designs use a number that isn't a multiple of 5 then we should just round.

neb42 · 2022-05-05T12:19:21Z

frontend/apps/core-app/src/components/RecipientListTable/createTableColumns.ts


 import { RecipientListTableEntry } from './types';

 export const createTableColumns = (
-  data: FormWithRecord<Record>[][],
+  recipientWithForm: FormWithRecord<Recipient>[] | null,


Why not just pass the form in here?

because I need all of the forms of one recipient, this was the easiest way to get them

neb42 · 2022-05-05T12:20:10Z

frontend/apps/core-app/src/components/RecipientListTable/RecipientListTableContainer.tsx

  );
  const memoizedColumns = React.useMemo(
-    () => createTableColumns(data),
-    [JSON.stringify(data)],
+    () => createTableColumns(recipientState.data && recipientState.data[0]),


If you just pass the form in then this can be the form from the props

neb42 · 2022-05-05T12:20:32Z

frontend/apps/core-app/src/screens/RecipientListScreen/RecipientListScreen.component.tsx

+    <ScrollView bg="white">
+      <Box bg="secondary.100" width="100%" my="16px" alignItems="center">
+        <Box maxWidth={1180} width="100%">
+          <Box mr="auto" mt="26px" mb="42px" maxWidth={580} width="100%">


neb42 · 2022-05-05T12:21:52Z

frontend/apps/core-app/src/screens/RecipientListScreen/RecipientListScreen.component.tsx

-    </Styles.Container>
+    <ScrollView bg="white">
+      <Box bg="secondary.100" width="100%" my="16px" alignItems="center">
+        <Box maxWidth={1180} width="100%">


Does this not need px?

neb42 · 2022-05-05T12:22:52Z

frontend/apps/core-app/src/screens/RecipientListScreen/RecipientListScreen.component.tsx

+      </Box>
+
+      <Box maxWidth={1180} mx="auto" width="100%">
+        {forms?.map((form) => (


This should have !isLoading otherwise if we had some refresh functionality this would show both the table and skeleton.

neb42 · 2022-05-05T12:27:27Z

...ps/core-app/src/screens/RecipientListScreen/__tests__/RecipientListScreen.component.test.tsx

@@ -2,10 +2,64 @@ import React from 'react';



The tests in this file seem out of date and aren't really testing anything

neb42 · 2022-05-05T12:29:33Z

...ps/core-app/src/screens/RecipientListScreen/__tests__/RecipientListScreen.container.test.tsx

+          : [() => Promise.resolve(), { data, loading: false, error: null }],
+      );
+      const { toJSON } = render(<RecipientListScreenContainer />);
+      expect(toJSON()).toMatchSnapshot();


I don't think snapshot tests are that useful for containers. What you want to test is that given "x" the component is rendered with the correct props. To do this I normally mock the component to return something like:

<View> <Text>{stringProps}</Text> <Text>{JSON.stringify(objectProp)}</Text> </View>

rational-terraforming-golem · 2022-05-09T17:28:52Z

@internetti: Adding label do-not-merge/contains-merge-commits because PR contains merge commits, which are not allowed in this repository.
Use git rebase to reapply your commits on top of the target branch. Detailed instructions for doing so can be found here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rational-terraforming-golem · 2022-05-09T17:28:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: internetti

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [internetti]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sonarcloud · 2022-05-11T09:06:10Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

rational-terraforming-golem bot requested review from ludydoo and neb42 March 28, 2022 10:56

rational-terraforming-golem bot added approved size/XL labels Mar 28, 2022

internetti changed the base branch from main to COR-491_tableComponent March 28, 2022 10:56

rational-terraforming-golem bot added size/L and removed size/XL labels Mar 28, 2022

internetti force-pushed the COR-491_tableComponent branch 2 times, most recently from 9bc9c2e to 4e580a2 Compare April 25, 2022 13:10

internetti force-pushed the COR-487_searchPage branch from 261a7de to 404eef4 Compare April 25, 2022 13:29

internetti marked this pull request as draft April 25, 2022 14:35

rational-terraforming-golem bot added the do-not-merge/work-in-progress label Apr 25, 2022

internetti force-pushed the COR-487_searchPage branch 2 times, most recently from f99cfcb to 4bd3c4a Compare April 29, 2022 10:05

rational-terraforming-golem bot added size/XXL and removed size/L labels Apr 29, 2022

internetti marked this pull request as ready for review April 29, 2022 11:07

rational-terraforming-golem bot removed the do-not-merge/work-in-progress label Apr 29, 2022

internetti commented Apr 29, 2022

View reviewed changes

frontend/apps/core-app/src/screens/RecipientListScreen/RecipientListScreen.container.tsx Outdated Show resolved Hide resolved

neb42 reviewed May 3, 2022

View reviewed changes

internetti changed the base branch from COR-491_tableComponent to main May 3, 2022 13:35

neb42 reviewed May 4, 2022

View reviewed changes

internetti force-pushed the COR-487_searchPage branch from 4bd3c4a to c8b4d44 Compare May 5, 2022 09:49

internetti changed the base branch from main to recipient-form-leaf-nodes May 5, 2022 09:49

neb42 reviewed May 6, 2022

View reviewed changes

internetti added 5 commits May 9, 2022 09:45

COR-487: list recipients in table

32f2483

COR-487: rewrite client list function

4810541

COR-487: start styling search page, show table header for 0 results

7260337

COR-487: style result number

7e99750

COR-487: style search page

7ba95b1

internetti added 8 commits May 9, 2022 09:45

COR-487: Record.list tests

e05615f

COR-487: Recipient.list tests

5aecc80

COR-487: container tests

c1fa442

COR-487: refactor, remove table context

b798199

COR-487: hide key fields, pass record id to click handler

a50de46

COR-487: clean up

aa9e6a0

COR-487: tests, WIP

827f067

COR-487: tests, PR comments

77b0414

internetti force-pushed the COR-487_searchPage branch from c8b4d44 to 77b0414 Compare May 9, 2022 17:28

rational-terraforming-golem bot added the do-not-merge/contains-merge-commits label May 9, 2022

COR-487: more tests

f28346d

internetti marked this pull request as draft May 11, 2022 07:50

rational-terraforming-golem bot added the do-not-merge/work-in-progress label May 11, 2022

COR-487: fix client tests

ca394cc

internetti marked this pull request as ready for review May 11, 2022 09:06

rational-terraforming-golem bot removed the do-not-merge/work-in-progress label May 11, 2022

rational-terraforming-golem bot requested a review from neb42 May 11, 2022 09:06

internetti removed the do-not-merge/contains-merge-commits label May 11, 2022

internetti changed the base branch from recipient-form-leaf-nodes to main May 11, 2022 09:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

COR-487: search page #395

COR-487: search page #395

internetti commented Mar 28, 2022 •

edited by jira bot

Loading

neb42 left a comment

neb42 May 3, 2022

neb42 May 3, 2022

neb42 May 3, 2022

neb42 May 3, 2022

neb42 May 3, 2022

neb42 May 3, 2022

internetti May 3, 2022

neb42 May 3, 2022

neb42 May 4, 2022

internetti May 5, 2022

neb42 left a comment

neb42 May 5, 2022

neb42 May 5, 2022

neb42 May 5, 2022

internetti May 9, 2022

neb42 May 5, 2022

neb42 May 5, 2022

neb42 May 5, 2022

neb42 May 5, 2022

neb42 May 5, 2022

neb42 May 5, 2022

rational-terraforming-golem bot commented May 9, 2022

rational-terraforming-golem bot commented May 9, 2022

sonarcloud bot commented May 11, 2022

		onRowClick: (id: string) => void;
		handleRowClick: () => void;

COR-487: search page #395

Are you sure you want to change the base?

COR-487: search page #395

Conversation

internetti commented Mar 28, 2022 • edited by jira bot Loading

neb42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neb42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rational-terraforming-golem bot commented May 9, 2022

rational-terraforming-golem bot commented May 9, 2022

sonarcloud bot commented May 11, 2022

internetti commented Mar 28, 2022 •

edited by jira bot

Loading