fix: Owners selection in dataset edit UX #17063

hughhhh · 2021-10-11T20:14:27Z

SUMMARY

The original dataset edit screen didn't populate owners and wasn't allowing users to properly query the db for potentially new owners.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

edit-owners.mov

TESTING INSTRUCTIONS

Goto datasets
Click on edit dataset in one of list items
Goto settings
Change owners
Save
Make sure owners in list reflect owners you chose

ADDITIONAL INFORMATION

Has associated issue: closes Not Able to Update DataSet Owner in Superset #15530 closes [edit_dataset]can not see full list of users in edit dataset dropdown #16883
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2021-10-11T20:49:13Z

Codecov Report

Merging #17063 (7c679b9) into master (11d52cb) will decrease coverage by 0.21%.
The diff coverage is 90.47%.

❗ Current head 7c679b9 differs from pull request most recent head a45e94c. Consider uploading reports for the commit a45e94c to get more accurate results

@@            Coverage Diff             @@
##           master   #17063      +/-   ##
==========================================
- Coverage   76.89%   76.67%   -0.22%     
==========================================
  Files        1031     1031              
  Lines       55183    55188       +5     
  Branches     7505     7505              
==========================================
- Hits        42432    42316     -116     
- Misses      12499    12620     +121     
  Partials      252      252

Flag	Coverage Δ
hive	`?`
mysql	`81.92% <100.00%> (+<0.01%)`	⬆️
postgres	`81.93% <100.00%> (+<0.01%)`	⬆️
presto	`?`
python	`82.02% <100.00%> (-0.42%)`	⬇️
sqlite	`81.61% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ontend/src/views/CRUD/data/dataset/DatasetList.tsx	`65.92% <0.00%> (ø)`
...erset-frontend/src/datasource/DatasourceEditor.jsx	`73.85% <92.85%> (ø)`
...perset-frontend/src/datasource/DatasourceModal.tsx	`75.00% <100.00%> (ø)`
superset/connectors/base/models.py	`88.25% <100.00%> (+0.11%)`	⬆️
superset/views/core.py	`76.94% <100.00%> (+0.03%)`	⬆️
superset/db_engines/hive.py	`0.00% <0.00%> (-85.19%)`	⬇️
superset/db_engine_specs/hive.py	`69.76% <0.00%> (-17.06%)`	⬇️
superset/db_engine_specs/presto.py	`83.47% <0.00%> (-6.91%)`	⬇️
superset/views/database/mixins.py	`81.03% <0.00%> (-1.73%)`	⬇️
superset/connectors/sqla/models.py	`85.59% <0.00%> (-1.66%)`	⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11d52cb...a45e94c. Read the comment docs.

eschutho · 2021-10-11T20:28:20Z

superset-frontend/src/datasource/DatasourceEditor.jsx

@@ -374,6 +374,40 @@ const defaultProps = {
  onChange: () => {},
 };

+function OwnersSelector({ datasource, onChange }) {
+  const selectedOwners = datasource.owners.map(owner => ({


will datasource and owners always have a value?

it should always have value since the top level component has datasource as is required:
https://github.com/apache/superset/pull/17063/files/51e6c3b96222397059051a082d8fe257bd2bc788#diff-a17a45fe0d0a8a3079cfe67ede4def65af02cef20ba94e9ee827b62edf51df23R367

ok, great. then per the below comment, it looks like selectedOwners will never be null or undefined, so you don't need the || []

eschutho · 2021-10-11T20:28:56Z

superset-frontend/src/datasource/DatasourceEditor.jsx

+      ariaLabel={t('Select owners')}
+      mode="multiple"
+      name="owners"
+      value={selectedOwners || []}


will selectedOwners ever be null? Unless there's an error above in the map, this value will already default to []

eschutho · 2021-10-11T20:29:32Z

superset-frontend/src/datasource/DatasourceEditor.jsx

+      value={selectedOwners || []}
+      options={loadOptions}
+      onChange={newOwners => {
+        onChange(newOwners);


you should be able to just pass onChange as the function instead of writing a new one

eschutho · 2021-10-11T21:20:55Z

superset-frontend/src/datasource/DatasourceEditor.jsx

+      })),
+      totalCount: response.json.count,
+    }));
+  });


I believe if you don't pass any dependencies to useMemo, it will still recalculate on every rerender. Do you need the useMemo here? It looks like all it will do on rerender is create the query variable.

for some reason the select component fails with out the useMemo we should look into that more but lets leave it for now

I'm going to push back here... let's try to understand why useMemo is needed? My understanding is the same as Elizabeth's, without the dependency array it's a no-op.

We've had a lot of quality issues lately, let's not commit code that we don't understand what's doing.

Talking to @michael-s-molina about useMemo vs. useCallback, if you fire the function with the same parameters (search, page, pageSize in this case) this code won't re run. So it is better for us to use useMemo since we'll be having the parameters for populating the select with owners options.

I believe if you don't pass any dependencies to useMemo, it will still recalculate on every rerender.

You're correct. If no array is provided, a new value will be computed on every render. In this case, we are passing a function to the options prop of the Select and this prop is used by useEffect hooks which means we don't want to recompute it unless the dependencies change. We could use a useCallback here but we also don't want to execute the function if it receives the same parameters. That's why we generally use useMemo with a dependency array (sometimes empty, sometimes not).

Just to let you know, we are already working on the documentation for each prop of the component and the different modes of operation. We're also going to update the Storybook with these docs.

betodealmeida

Looks good, I just left 2 small nits.

betodealmeida · 2021-10-11T21:32:12Z

superset-frontend/src/datasource/DatasourceEditor.jsx

+      onChange={newOwners => {
+        onChange(newOwners);
+      }}


Nit:

Suggested change

onChange={newOwners => {

onChange(newOwners);

}}

onChange={onChange}

betodealmeida · 2021-10-11T21:35:29Z

superset-frontend/src/datasource/DatasourceEditor.jsx

+  const loadOptions = useMemo(() => (search = '', page, pageSize) => {
+    const query = rison.encode({ filter: search, page, page_size: pageSize });
+    return SupersetClient.get({
+      endpoint: `/api/v1/chart/related/owners?q=${query}`,


The result is the same, but let's use dataset here instead of chart:

Suggested change

endpoint: `/api/v1/chart/related/owners?q=${query}`,

endpoint: `/api/v1/dataset/related/owners?q=${query}`,

yousoph · 2021-10-11T21:57:47Z

/testenv up

github-actions · 2021-10-11T21:59:45Z

@yousoph Ephemeral environment spinning up at http://35.86.147.0:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

rosemarie-chiu · 2021-10-11T22:18:12Z

I'm not able to select user from the dropdown.

hughhhh · 2021-10-12T00:32:30Z

/testenv up

github-actions · 2021-10-12T00:36:39Z

@hughhhh Ephemeral environment spinning up at http://34.209.136.120:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

hughhhh · 2021-10-12T00:49:01Z

/testenv up

github-actions · 2021-10-12T00:53:06Z

@hughhhh Ephemeral environment spinning up at http://34.217.1.111:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

hughhhh · 2021-10-12T04:44:09Z

/testenv up

github-actions · 2021-10-12T04:48:39Z

@hughhhh Ephemeral environment spinning up at http://54.201.92.110:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

betodealmeida · 2021-10-12T16:07:44Z

superset/connectors/base/models.py

+            "owners": [
+                {"label": f"{owner.first_name} {owner.last_name}", "value": owner.id}
+                for owner in self.owners
+            ],


label and value are concerns tied to the component you're using in the frontend. Let's discuss this a bit more.

I assume we have endpoints where we can get full user information, including first name and last name. The component should fetch from there and build label and value however it needs.

betodealmeida · 2021-10-12T20:46:55Z

superset-frontend/src/datasource/DatasourceEditor.jsx

+      })),
+      totalCount: response.json.count,
+    }));
+  }, []);


Curious to what the error is when you just use the function directly?

Using useCallback is basically the same as useMemo, though now you have an empty array of dependencies so it will memoize forever, which is what we want.

i had the wrong function schema 😓 rookie move

Let's clarify the concepts a little bit (https://reactjs.org/docs/hooks-reference.html)

useCallback will return a memoized version of the callback that only changes if one of the dependencies has changed. This is useful when passing callbacks to optimized child components that rely on reference equality to prevent unnecessary renders.

useMemo will only recompute the memoized value when one of the dependencies has changed. This optimization helps to avoid expensive calculations on every render.

Curious to what the error is when you just use the function directly?

If we pass the function directly, this means that every time the component renders, it will create a new instance of the function, which will trigger the useEffects inside the Select component, which will call the function and execute it unnecessarily.

If we use useCallback it means that the function instance will be the same between renders, avoiding unnecessary executions. But if while interacting with the component, this function is called two or more times with the same search, page, and pageSize, useCallback won't prevent unnecessary executions. That's why we use useMemo. One example of this scenario is if we change a property like useFetchOnlyOnSearch which will trigger a new call to the function with the same parameters.

junlincc · 2021-10-12T21:55:01Z

does this PR fix #15530 and #16883?

sadpandajoe · 2021-10-12T22:34:27Z

🏷️ 2021.40

hughhhh · 2021-10-12T22:35:08Z

does this PR fix #15530 and #16883?

Yup :)

junlincc · 2021-10-12T22:36:11Z

Yup :)

Thanks! I will update the the pr description accordingly. 🙏

…/edit-owners

github-actions · 2021-10-13T00:28:17Z

Ephemeral environment shutdown and build artifacts deleted.

* boilerplate * update owner select component * this is working * update onchange * refactorig * you need to useMemo or things break * update test * prettier * move logic into bootstrap data endpoint * address concerns * oops * oops * fix test (cherry picked from commit 959fd76)

* boilerplate * update owner select component * this is working * update onchange * refactorig * you need to useMemo or things break * update test * prettier * move logic into bootstrap data endpoint * address concerns * oops * oops * fix test

boilerplate

490232c

pull-request-size bot added the size/M label Oct 11, 2021

hughhhh force-pushed the hugh/edit-owners branch 4 times, most recently from d24364f to 9c92b7e Compare October 11, 2021 20:24

update owner select component

51e6c3b

hughhhh force-pushed the hugh/edit-owners branch from 9c92b7e to 51e6c3b Compare October 11, 2021 20:27

eschutho reviewed Oct 11, 2021

View reviewed changes

betodealmeida approved these changes Oct 11, 2021

View reviewed changes

this is working

ce7c8b7

update onchange

f3cc172

hughhhh added 2 commits October 11, 2021 20:37

refactorig

0b9bf0c

you need to useMemo or things break

e956466

hughhhh added 2 commits October 11, 2021 21:26

update test

75a0c4e

prettier

f5a77cb

betodealmeida requested changes Oct 12, 2021

View reviewed changes

hughhhh added 3 commits October 12, 2021 13:37

move logic into bootstrap data endpoint

8f02a5c

address concerns

ba84ca4

oops

09f8205

oops

3b32394

betodealmeida approved these changes Oct 12, 2021

View reviewed changes

fix test

447b5ac

superset-github-bot bot added the preset:2021.40 label Oct 12, 2021

Merge branch 'master' of https://github.com/apache/superset into hugh…

a45e94c

…/edit-owners

hughhhh merged commit 959fd76 into master Oct 13, 2021

geido mentioned this pull request Oct 20, 2021

fix(Explore): Undefined owners #17167

Merged

9 tasks

eschutho added the v1.4 label Oct 26, 2021

nytai mentioned this pull request Feb 8, 2022

Owner names not rendered in React Datasource editor #11680

Closed

3 tasks

mistercrunch added 🍒 1.4.0 🍒 1.4.1 🍒 1.4.2 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024

mistercrunch deleted the hugh/edit-owners branch March 26, 2024 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Owners selection in dataset edit UX #17063

fix: Owners selection in dataset edit UX #17063

hughhhh commented Oct 11, 2021 •

edited by junlincc

Loading

codecov bot commented Oct 11, 2021 •

edited

Loading

eschutho Oct 11, 2021

hughhhh Oct 11, 2021

eschutho Oct 11, 2021 •

edited

Loading

eschutho Oct 11, 2021

eschutho Oct 11, 2021

eschutho Oct 11, 2021 •

edited

Loading

hughhhh Oct 12, 2021

betodealmeida Oct 12, 2021

hughhhh Oct 12, 2021

michael-s-molina Oct 12, 2021

michael-s-molina Oct 12, 2021 •

edited

Loading

betodealmeida left a comment

betodealmeida Oct 11, 2021

betodealmeida Oct 11, 2021

yousoph commented Oct 11, 2021

github-actions bot commented Oct 11, 2021

rosemarie-chiu commented Oct 11, 2021

hughhhh commented Oct 12, 2021

github-actions bot commented Oct 12, 2021

hughhhh commented Oct 12, 2021

github-actions bot commented Oct 12, 2021

hughhhh commented Oct 12, 2021

github-actions bot commented Oct 12, 2021

betodealmeida Oct 12, 2021 •

edited

Loading

betodealmeida Oct 12, 2021

betodealmeida Oct 12, 2021

hughhhh Oct 12, 2021

michael-s-molina Oct 12, 2021

junlincc commented Oct 12, 2021

sadpandajoe commented Oct 12, 2021

hughhhh commented Oct 12, 2021

junlincc commented Oct 12, 2021

github-actions bot commented Oct 13, 2021

	endpoint: `/api/v1/chart/related/owners?q=${query}`,
	endpoint: `/api/v1/dataset/related/owners?q=${query}`,

fix: Owners selection in dataset edit UX #17063

fix: Owners selection in dataset edit UX #17063

Conversation

hughhhh commented Oct 11, 2021 • edited by junlincc Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Oct 11, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho Oct 11, 2021 • edited Loading

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

michael-s-molina Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

betodealmeida left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yousoph commented Oct 11, 2021

github-actions bot commented Oct 11, 2021

rosemarie-chiu commented Oct 11, 2021

hughhhh commented Oct 12, 2021

github-actions bot commented Oct 12, 2021

hughhhh commented Oct 12, 2021

github-actions bot commented Oct 12, 2021

hughhhh commented Oct 12, 2021

github-actions bot commented Oct 12, 2021

betodealmeida Oct 12, 2021 • edited Loading

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

junlincc commented Oct 12, 2021

sadpandajoe commented Oct 12, 2021

hughhhh commented Oct 12, 2021

junlincc commented Oct 12, 2021

github-actions bot commented Oct 13, 2021

hughhhh commented Oct 11, 2021 •

edited by junlincc

Loading

codecov bot commented Oct 11, 2021 •

edited

Loading

eschutho Oct 11, 2021 •

edited

Loading

eschutho Oct 11, 2021 •

edited

Loading

michael-s-molina Oct 12, 2021 •

edited

Loading

betodealmeida Oct 12, 2021 •

edited

Loading