#1685 Custom options for @pixiebrix/data/get and @pixiebrix/data/put bricks #1750

BALEHOK · 2021-10-25T20:16:46Z

Resolves #1685

DB Selector

Create DB modal

src/devTools/editor/fields/DatabaseGetOptions.tsx

src/components/createMenuListWithAddButton.tsx

twschiller · 2021-10-26T11:22:50Z

src/components/createMenuListWithAddButton.tsx

+import React from "react";
+import { Button } from "react-bootstrap";
+
+type MenuListWithAddButtonProps = {


If we're not exporting the component and there's just one component in the file, I think it's OK to use "OwnProps" or another short type name

In many cases the props type is called {ComponentName}Props. It always makes a naming pair of component-props and does not depend on exported members.

I'm for consistency. Why not to settle on the convention now. Options:

Do not define a type at all unless you need to export... or the type definition is huge (10+ line) example.

Always define the props type. Call it OwnProps if not exported, call it {ComponentName}Props if/when you export the type.

Always define the props type and call it {ComponentName}Props.

@twschiller
cc @BLoe

My thoughts are that if the type is not exported, I don't much care what you name it, I care much, much more about how the type is structured, so I can read it and understand what's going on with a component.

If it's exported it should be {ComponentName}Props, that's about the only strict rule I'd feel strongly about here.

src/devTools/editor/fields/DatabaseCreateModal.tsx

twschiller · 2021-10-26T11:28:39Z

src/devTools/editor/fields/DatabaseCreateModal.tsx

+    });
+
+    if ("error" in result) {
+      console.error(result.error);


Fix error handling

twschiller · 2021-10-26T11:29:26Z

src/devTools/editor/fields/DatabaseCreateModal.tsx

+
+  const [addDatabaseToGroup] = useAddDatabaseToGroupMutation();
+
+  const onSave = async ({ name, organizationId, groupId }: DatabaseConfig) => {


See useUserAction

appBaseQuery doesn't throw, it returns a result with an error -> useUserAction will not handle the error properly.

useUserAction tries to get the error message from AxiosError, but appBaseQuery kinda obfurscates the error -> useUserAction logic doesn't help.

onSave conditionally makes 2 requests and handles the result of each. There's no need to update the UI state either.

Not sure howuseUserAction can help here?

src/types/contract.ts

src/services/api.ts

twschiller · 2021-10-26T11:32:55Z

src/services/api.ts

    }),
    getOrganizations: builder.query<Organization[], void>({
      query: () => ({ url: "/api/organizations/", method: "get" }),
+      providesTags: ["Organizations"],
+      transformResponse: (
+        baseQueryReturnValue: Array<components["schemas"]["Organization"]>


We've been referring to the type exported from the the contract.ts file instead of using components["schema"] in code

Also, make any manual updates to the schema in contracts.ts instead of using any below. See the examples in contract.ts that use "&" to refine the types from swagger.ts

make any manual updates to the schema in contracts.ts

This is my intention.
API returns Organization without isAdmin (or role) property. src/types/contract.ts add it at line 40. But this is only on the types level.
Here transformResponse reserves the object from API (which is actually of type Array<components["schemas"]["Organization"]>) and converts it to what we want in the Extension project - Organization defined in contracts.ts.

twschiller · 2021-10-26T11:33:33Z

src/services/api.ts

+      ): Organization[] =>
+        baseQueryReturnValue.map((apiOrganization) => ({
+          ...apiOrganization,
+          /*


Use //-style comments. Your IDE should have a shortcut for commenting multiple lines

twschiller · 2021-10-26T11:39:16Z

src/services/api.ts

+        meta: { organizationId },
+        includeRequestData: true,
+      }),
+      providesTags: ["Groups"],


Don't these tags need to include the id of the organization? https://redux-toolkit.js.org/rtk-query/api/createApi#providestags. Or does the meta automatically handle that?

Meta is a means of getting request parameter in the transformResponse function.
I think you're right, the organization id should be included in tags.

twschiller · 2021-10-26T11:43:49Z

src/devTools/editor/fields/DatabaseCreateModal.tsx

+  const onSave = async ({ name, organizationId, groupId }: DatabaseConfig) => {
+    const result = await createDatabase({
+      name,
+      organizationId,


Is organizationId "" at this point? Or is it null? See comment on the type definition

organizationId can be an ID or empty (null, undefined, blank).

twschiller · 2021-10-26T11:45:49Z

src/devTools/editor/fields/DatabaseCreateModal.tsx

+    onClose();
+  };
+
+  let organizationOptions = (organizations || [])


Extract into helper method, e.g., selectOrganizationOptions(organizations)

Use the null coalescing operator ??, not ||

twschiller · 2021-10-26T12:13:42Z

src/devTools/editor/hooks/useDatabaseOptions.ts

+              (o) => o.id === db.organization_id
+            );
+            const dbName = organization
+              ? `${db.name} [${organization.name}]`


See http://github.com/pixiebrix/pixiebrix-extension/blob/feature/1685-data-get-put/src/hooks/auth.ts#L114-L114 for our current convention for showing organization name in options drop downs (it uses an mdash)

…-put

twschiller · 2021-10-27T12:20:26Z

src/components/form/widgets/SelectWidget.tsx

@@ -78,6 +80,9 @@ const SelectWidget = <TOption extends Option<TOption["value"]>>({
    >);
  };

+  const selectValue =
+    options?.find((option: TOption) => value === option.value) || null;


Use the null coalescing operator ??, not ||

twschiller

@BALEHOK this is ready to merge once you make one small improvement

When you add a database with create database, it should be selected from the dropdown

Also, some general comments:

Use the nullish coalescing operator: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing_operator
The role logic is sufficient but isn't completely correct because it doesn't account for the Developer or Restricted team roles. I added a warning in both places about role so that someone in the future doesn't make a mistake with them

BALEHOK · 2021-10-27T18:16:50Z

src/devTools/editor/fields/DatabaseCreateModal.tsx

+  organizationId: string | null;

  /**
-   * Id of a Group for shared DB.
-   * Blank for a Personal DB
+   * UUID of a Group for a shared DB, or null/empty for a personal DB.
   */
-  groupId: string;
+  groupId: string | null;


@twschiller This refers to the rule strictNullChecks. It is disabled so any property can be null or undefined. Seems that <type> | null is not really consistent with the code style, it isn't widely adopted on the project.

This refers to the rule strictNullChecks

I'm not sure I'm following? Even though strictNullChecks is not enabled, we've always been marking which fields are optional with the (?) or accept null. Otherwise the type is inconsistent with the documentation above it

BALEHOK added 9 commits October 22, 2021 12:39

DBGet Options

ff2a260

Loading databases

3481196

DB selector

4a84bab

Add new option

4193eb4

renamig

326a7f1

Create DB modal

6c29c28

Calling Create DB API

38c3b78

trying to set service

d54d0d4

fetching groups

cc09dfc

BALEHOK requested a review from twschiller October 25, 2021 20:16

BALEHOK commented Oct 25, 2021

View reviewed changes

src/devTools/editor/fields/DatabaseGetOptions.tsx Outdated Show resolved Hide resolved

Storing groups

d053df8

twschiller reviewed Oct 26, 2021

View reviewed changes

src/components/createMenuListWithAddButton.tsx Outdated Show resolved Hide resolved

twschiller reviewed Oct 26, 2021

View reviewed changes

src/devTools/editor/fields/DatabaseCreateModal.tsx Outdated Show resolved Hide resolved

twschiller reviewed Oct 26, 2021

View reviewed changes

src/types/contract.ts Outdated Show resolved Hide resolved

twschiller reviewed Oct 26, 2021

View reviewed changes

src/services/api.ts Outdated Show resolved Hide resolved

twschiller reviewed Oct 26, 2021

View reviewed changes

#1685: add AppServiceField

78bee4d

twschiller reviewed Oct 26, 2021

View reviewed changes

BALEHOK added 3 commits October 26, 2021 15:13

Some improvements

eca417b

code review fixes

0ef3440

dbOptions helper

a8e8f37

BALEHOK requested a review from twschiller October 26, 2021 19:18

BALEHOK marked this pull request as ready for review October 27, 2021 08:18

Merge remote-tracking branch 'origin/main' into feature/1685-data-get…

34c4d18

…-put

twschiller reviewed Oct 27, 2021

View reviewed changes

twschiller added 2 commits October 27, 2021 08:38

#1685: clarify types/code comments

a54d470

#1685: add cancel button to modal; align right to match other modals

51044e4

twschiller approved these changes Oct 27, 2021

View reviewed changes

twschiller added this to the 1.4.5 milestone Oct 27, 2021

BALEHOK commented Oct 27, 2021

View reviewed changes

BALEHOK added 2 commits October 27, 2021 20:36

Select new DB

f3af4fd

Fetching common component. Protect from unomunted state updates

1998629

twschiller merged commit 732f978 into main Oct 28, 2021

twschiller deleted the feature/1685-data-get-put branch October 28, 2021 12:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1685 Custom options for @pixiebrix/data/get and @pixiebrix/data/put bricks #1750

#1685 Custom options for @pixiebrix/data/get and @pixiebrix/data/put bricks #1750

BALEHOK commented Oct 25, 2021 •

edited

twschiller Oct 26, 2021

BALEHOK Oct 26, 2021

BLoe Oct 26, 2021

twschiller Oct 26, 2021 •

edited by BALEHOK

twschiller Oct 26, 2021

BALEHOK Oct 26, 2021 •

edited

twschiller Oct 26, 2021 •

edited

BALEHOK Oct 26, 2021 •

edited

twschiller Oct 26, 2021 •

edited

twschiller Oct 26, 2021

BALEHOK Oct 26, 2021

twschiller Oct 26, 2021

BALEHOK Oct 26, 2021

twschiller Oct 26, 2021

twschiller Oct 26, 2021 •

edited

twschiller Oct 27, 2021

twschiller left a comment •

edited by BALEHOK

BALEHOK Oct 27, 2021

twschiller Oct 28, 2021 •

edited


		const [addDatabaseToGroup] = useAddDatabaseToGroupMutation();

		const onSave = async ({ name, organizationId, groupId }: DatabaseConfig) => {

#1685 Custom options for @pixiebrix/data/get and @pixiebrix/data/put bricks #1750

#1685 Custom options for @pixiebrix/data/get and @pixiebrix/data/put bricks #1750

Conversation

BALEHOK commented Oct 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller Oct 26, 2021 • edited by BALEHOK

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BALEHOK Oct 26, 2021 • edited

Choose a reason for hiding this comment

twschiller Oct 26, 2021 • edited

Choose a reason for hiding this comment

BALEHOK Oct 26, 2021 • edited

Choose a reason for hiding this comment

twschiller Oct 26, 2021 • edited

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

twschiller Oct 26, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller left a comment • edited by BALEHOK

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller Oct 28, 2021 • edited

Choose a reason for hiding this comment

BALEHOK commented Oct 25, 2021 •

edited

twschiller Oct 26, 2021 •

edited by BALEHOK

BALEHOK Oct 26, 2021 •

edited

twschiller Oct 26, 2021 •

edited

BALEHOK Oct 26, 2021 •

edited

twschiller Oct 26, 2021 •

edited

twschiller Oct 26, 2021 •

edited

twschiller left a comment •

edited by BALEHOK

twschiller Oct 28, 2021 •

edited