fix(home): missing key and invalid dates in Recents cards #13291

ktmud · 2021-02-23T00:49:15Z

SUMMARY

Cleaning up tech debt in Welcome recents cards and add proper typing.
Fixes missing React children key and invalid dates bug (PR fix: tests errors and warnings - iteration 6 (#12212) #12227 seems to have brought back Bug Last edited dates shown as invalid on welcome screen #12893 )
Make Last modified xxx format consistent. Let Recent activity cards use backend humanized relative dates, too.
Change the copy from "Last modified" to "Last viewed" for Viewed cards because that's what they are.

Alternative solution to #13225

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

TEST PLAN

Manual verification on the home page

ADDITIONAL INFORMATION

codecov-io · 2021-02-23T00:59:01Z

Codecov Report

Merging #13291 (29a538c) into master (974f447) will increase coverage by 2.79%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #13291      +/-   ##
==========================================
+ Coverage   77.14%   79.93%   +2.79%     
==========================================
  Files         865      298     -567     
  Lines       44961    24285   -20676     
  Branches     5415        0    -5415     
==========================================
- Hits        34686    19413   -15273     
+ Misses      10152     4872    -5280     
+ Partials      123        0     -123

Flag	Coverage Δ
cypress	`?`
javascript	`?`
python	`79.93% <100.00%> (-0.88%)`	⬇️

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

Impacted Files	Coverage Δ
superset/views/core.py	`71.82% <100.00%> (-3.56%)`	⬇️
superset/sql_validators/postgres.py	`50.00% <0.00%> (-50.00%)`	⬇️
superset/views/database/views.py	`62.69% <0.00%> (-24.88%)`	⬇️
superset/databases/commands/create.py	`83.67% <0.00%> (-8.17%)`	⬇️
superset/databases/commands/update.py	`85.71% <0.00%> (-8.17%)`	⬇️
superset/sql_validators/base.py	`93.33% <0.00%> (-6.67%)`	⬇️
superset/db_engine_specs/sqlite.py	`90.62% <0.00%> (-6.25%)`	⬇️
superset/db_engine_specs/base.py	`79.95% <0.00%> (-6.12%)`	⬇️
superset/views/database/forms.py	`83.33% <0.00%> (-5.56%)`	⬇️
superset/db_engine_specs/presto.py	`82.47% <0.00%> (-5.56%)`	⬇️
... and 577 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 974f447...29a538c. Read the comment docs.

junlincc · 2021-02-23T01:55:35Z

@yardz Hey Bruno, Jesse opened a PR as alternative of #13225, please review. 🙏

cc @pkdotson

yardz · 2021-02-23T02:07:06Z

superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx

+const TRANS_LAST_VIEWED = `Last viewed %s`;
+const TRANS_LAST_MODIFIED = `Last modified %s`;
+
+const getEntityTitle = (e: ActivityObject) => {


@ktmud Just one question. Are you sure these are the returns?
As I don't know much about the application, I'm very cautious about typing
If you're sure, I agree

It's replicating the same logic as the deleted code. Just with some typing help.

yardz · 2021-02-23T02:07:33Z

superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx

+  return e.item_title || UNTITLED;
+};
+
+const getEntityIconName = (e: ActivityObject) => {


Same thing here (and for the other functions)

yardz · 2021-02-23T02:08:06Z

superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx

+          onClick={() => {
+            window.location.href = url;
+          }}
+          key={url}


yardz · 2021-02-23T02:09:15Z

superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx

+/**
+ * Recent activity objects fetched by `getRecentAcitivtyObjs`.
+ */
+type ActivityObject =


@ktmud Are you sure it's just these types?

Yes, as ActivityTable renders ActivityData and ActivityData comes from getRecentAcitivtyObjs, which then calls these API endpoints.

ktmud · 2021-02-23T02:18:40Z

superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx

+const getEntityTitle = (e: ActivityObject) => {
+  if ('dashboard_title' in e) return e.dashboard_title || UNTITLED;
+  if ('slice_name' in e) return e.slice_name || UNTITLED;
+  if ('label' in e) return e.label || UNTITLED;


This fixes the case when saved queries don't have a title (label).

Before

After

etr2460

a few comments, mostly style related or questions to verify

etr2460 · 2021-02-23T02:14:49Z

superset-frontend/src/types/Chart.ts

 };
+
+export default Chart;


Yikes, i don't even want to know why we have a Slice and a Chart type...

I think one has stricter typing following the backend API response; another is for temporary chart state on the client side (we should probably consolidate these two).

etr2460 · 2021-02-23T02:16:03Z

superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx

+// translation keys for last action on
+const TRANS_LAST_VIEWED = `Last viewed %s`;
+const TRANS_LAST_MODIFIED = `Last modified %s`;


does this actually work? I would've guessed you'd need to put the actual strings in the t() functions

Yes, I did put it into t() later. Just making the translation keys constants because they are used in multiple statements. I can move them closer to the actual t() calls to reduce confusion.

i was more wondering if the job that pulls all the strings to be translated is smart enough to know that these strings end up in a t() function

etr2460 · 2021-02-23T02:16:57Z

superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx

+          onClick={() => {
+            window.location.href = url;
+          }}
+          key={url}


is this guaranteed to be unique? The same item can't show up twice in the recent activity section?

Yes, it's guaranteed. The recent_activity API removes duplicates and results for other tabs are just regular filtered lists which has guaranteed unique items.

etr2460 · 2021-02-23T02:18:16Z

superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx

+const TRANS_LAST_VIEWED = `Last viewed %s`;
+const TRANS_LAST_MODIFIED = `Last modified %s`;
+
+const getEntityTitle = (e: ActivityObject) => {


I know this is just a refactor, but e is a pretty bad variable name (i read it as error or event first, not entity). Can we rename throughout to entity?

pull-request-size bot added the size/L label Feb 23, 2021

ktmud mentioned this pull request Feb 23, 2021

fix: missing key on welcome page #13225

Closed

ktmud force-pushed the fix-welcomepage-key branch from 0c951fc to b33fe7f Compare February 23, 2021 00:57

ktmud changed the title ~~fix(home): missing key and inval dates in Recents cards~~ fix(home): missing key and invalid dates in Recents cards Feb 23, 2021

fix(home): missing key and invalid dates in Recents cards

d9fde12

ktmud force-pushed the fix-welcomepage-key branch from b33fe7f to d9fde12 Compare February 23, 2021 01:08

junlincc added the home:recent Related to the "recent" section of the Homepage label Feb 23, 2021

yardz reviewed Feb 23, 2021

View reviewed changes

ktmud commented Feb 23, 2021

View reviewed changes

etr2460 approved these changes Feb 23, 2021

View reviewed changes

Address PR comments

29a538c

ktmud merged commit af3a460 into apache:master Feb 23, 2021

ktmud deleted the fix-welcomepage-key branch February 23, 2021 04:30

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(home): missing key and invalid dates in Recents cards #13291

fix(home): missing key and invalid dates in Recents cards #13291

ktmud commented Feb 23, 2021 •

edited

Loading

codecov-io commented Feb 23, 2021 •

edited

Loading

junlincc commented Feb 23, 2021 •

edited

Loading

yardz Feb 23, 2021

ktmud Feb 23, 2021

yardz Feb 23, 2021

yardz Feb 23, 2021

yardz Feb 23, 2021

ktmud Feb 23, 2021

ktmud Feb 23, 2021

etr2460 left a comment

etr2460 Feb 23, 2021

ktmud Feb 23, 2021

etr2460 Feb 23, 2021

ktmud Feb 23, 2021

etr2460 Feb 23, 2021

etr2460 Feb 23, 2021

ktmud Feb 23, 2021

etr2460 Feb 23, 2021

ktmud Feb 23, 2021

fix(home): missing key and invalid dates in Recents cards #13291

fix(home): missing key and invalid dates in Recents cards #13291

Conversation

ktmud commented Feb 23, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

TEST PLAN

ADDITIONAL INFORMATION

codecov-io commented Feb 23, 2021 • edited Loading

Codecov Report

junlincc commented Feb 23, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Before

After

etr2460 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

ktmud commented Feb 23, 2021 •

edited

Loading

codecov-io commented Feb 23, 2021 •

edited

Loading

junlincc commented Feb 23, 2021 •

edited

Loading