Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: missing key on welcome page #13225

Closed
wants to merge 4 commits into from
Closed

fix: missing key on welcome page #13225

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e479456
bugfix: missing key on welcome page
yardz Feb 18, 2021
86d31c5
Update superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx
yardz Feb 22, 2021
7c4314f
fix: import order
yardz Feb 22, 2021
470ef98
fix: unique keys in the different tabs
yardz Feb 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 10 additions & 67 deletions superset-frontend/src/views/CRUD/welcome/ActivityTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,31 +17,16 @@
* under the License.
*/
import React from 'react';
import moment from 'moment';

import { styled, t } from '@superset-ui/core';

import Loading from 'src/components/Loading';
import ListViewCard from 'src/components/ListViewCard';
import SubMenu from 'src/components/Menu/SubMenu';
import ActivityTableRow from './ActivityTableRow';
import { ActivityData } from './Welcome';
import { mq, CardStyles } from '../utils';
import { mq } from '../utils';
rusackas marked this conversation as resolved.
Show resolved Hide resolved
import EmptyState from './EmptyState';

interface ActivityObjects {
action?: string;
item_title?: string;
slice_name: string;
time: string;
changed_on_utc: string;
url: string;
sql: string;
dashboard_title: string;
label: string;
id: string;
table: object;
item_url: string;
}

interface ActivityProps {
user: {
userId: string | number;
Expand Down Expand Up @@ -85,25 +70,6 @@ export default function ActivityTable({
setActiveChild,
activityData,
}: ActivityProps) {
const getFilterTitle = (e: ActivityObjects) => {
if (e.dashboard_title) return e.dashboard_title;
if (e.label) return e.label;
if (e.url && !e.table) return e.item_title;
if (e.item_title) return e.item_title;
return e.slice_name;
};

const getIconName = (e: ActivityObjects) => {
if (e.sql) return 'sql';
if (e.url?.includes('dashboard') || e.item_url?.includes('dashboard')) {
return 'nav-dashboard';
}
if (e.url?.includes('explore') || e.item_url?.includes('explore')) {
return 'nav-charts';
}
return '';
};

const tabs = [
{
name: 'Edited',
Expand Down Expand Up @@ -139,35 +105,6 @@ export default function ActivityTable({
});
}

const renderActivity = () => {
const getRecentRef = (e: ActivityObjects) => {
if (activeChild === 'Viewed') {
return e.item_url;
}
return e.sql ? `/superset/sqllab?savedQueryId=${e.id}` : e.url;
};
return activityData[activeChild].map((e: ActivityObjects) => (
<CardStyles
onClick={() => {
window.location.href = getRecentRef(e);
}}
key={e.id}
>
<ListViewCard
loading={loading}
cover={<></>}
url={e.sql ? `/superset/sqllab?savedQueryId=${e.id}` : e.url}
title={getFilterTitle(e)}
description={`Last Edited: ${moment(
e.changed_on_utc,
'MM/DD/YYYY HH:mm:ss',
)}`}
avatar={getIconName(e)}
actions={null}
/>
</CardStyles>
));
};
if (loading) return <Loading position="inline" />;
return (
<>
Expand All @@ -178,7 +115,13 @@ export default function ActivityTable({
/>
<>
{activityData[activeChild]?.length > 0 ? (
<ActivityContainer>{renderActivity()}</ActivityContainer>
<ActivityContainer>
<ActivityTableRow
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Does it makes sense to call this ActivityCard ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave this name following the logic "table->row". If you look at ActivityTableRow it receives an array and returns several "cards" inside a Fragment.

I had thought of "ActivityList" (list of cards right?), but I chose to keep it as "ActivityTableRow" as there are some improvements to be made in the props and these components are very related...

Copy link
Member

@ktmud ktmud Feb 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TableRow sounds like a row of a table, but it actually contains the whole table content for a selected tab. We should probably rename other XxxTable to simply Recents, Dashboards, Charts and SavedQueries to be consistent with section titles, then the single tab content can be named as RecentsList, DashboardList, etc.

Copy link
Contributor Author

@yardz yardz Feb 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TableRow sounds like a row of a table, but it actually contains the whole table content for a selected tab. We should probably rename other XxxTable to simply Recents, Dashboards, Charts and SavedQueries to be consistent with section titles, then the single table content can be named as RecentsList, DashboardList, etc.

I agree that the names are not good. But I think this is a task for another PR, the purpose of this was just to solve the problem of the keys and not to solve "all problems" related to these files.

Example: These files also need test files, the types need to be better defined, etc.
None of this is the scope of this PR, just as renaming the "ActivityTable" does not seem to me to be scope for this PR.

@ktmud

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

activityList={activityData[activeChild]}
activeChild={activeChild}
loading={loading}
/>
</ActivityContainer>
) : (
<EmptyState tableName="RECENTS" tab={activeChild} />
)}
Expand Down
112 changes: 112 additions & 0 deletions superset-frontend/src/views/CRUD/welcome/ActivityTableRow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
/**
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
import React from 'react';
import moment from 'moment';
import ListViewCard from 'src/components/ListViewCard';
import { CardStyles } from '../utils';

interface ActivityObjects {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably have separate type for each entity, then use union type for "activity object".

type ActivityObject = RecentSlice | RecentDashboard

Copy link
Contributor Author

@yardz yardz Feb 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but again, this PR is not to improve the existing types but to solve a bug. Refactoring and any other type of improvement is not within the scope of this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one could be related since accurate typing may have an impact on how unique keys are chosen. Since this is not a user-facing bug, it might also make sense to do the type refactoring first (either in this or another PR) before merging this fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ktmud Do you prefer a buggy code and a more complex code than a simpler and no bug code, because the type is not perfect?

Sorry it doesn't make any sense to me.

Copy link
Member

@ktmud ktmud Feb 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take a look at #13291

  1. Adding typing won't necessary make the code more complex and it does help identify and simplify potential solutions (as demonstrated in my PR).
  2. Current code wasn't buggy to the end users (on the key problem at least).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Adding typing won't necessary make the code more complex and it does help identify and simplify potential solutions (as demonstrated in my PR).

I didn't talk about the type but about the original component, which had 1 more component inside it. (2 components in 1).
This is a more complex component and much more difficult to test.

  1. Current code (for React key at least) wasn't buggy to the end users.

If the user didn't see it then the bug doesn't exist?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey guys sorry i haven't been following the threads. based on the conversation above, my understanding is that
1)we are not fixing UI bug, meaning I wouldn't notice it as an end user. 2)there might be a more comprehensive solution to solve this issue.
@yardz I prioritize code quality + cleaning up technical debt in Q1-Q2, more than even fixing an actual UI bug. Let's make sure if refractor is necessary, we are not simply putting a bandaid on the code, but solve it from the root, also not get distracted by not prioritized tasks.
would you mind addressing @ktmud comment if that's not too much of work, or we can park this PR and move on.

cc @rusackas

Copy link
Member

@ktmud ktmud Feb 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it does not impact the users, then there is no urgency to merge your fix. I personally feel typing is a bigger fish to catch than solving a developer warning that is not visible to users. You may disagree but there is no need to be sarcastic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1)we are not fixing UI bug, meaning I wouldn't notice it as an end user.

This is not necessarily true. Just because our test data set is not generating a noticeable error does not mean that with a different data set this error is imperceptible. This key problem generates a lot of errors when a component is rerendered.

2)there might be a more comprehensive solution to solve this issue.

From what I saw, there are some requests that are not correctly typed (and are in js). A definitive and responsible solution would involve much more work and several PRs. I wouldn't risk putting a "definitive" type without having done this work before it...
I believe that this component needs some refinement "cycles" to achieve the "definitive" result

action?: string;
item_title?: string;
slice_name: string;
time: string;
changed_on_utc: string;
url: string;
sql: string;
dashboard_title: string;
label: string;
id: string;
table: object;
item_url: string;
}

interface ActivityProps {
activeChild: string;
loading: boolean;
activityList: ActivityObjects[];
}

const getRecentRef = (activeChild: string, activity: ActivityObjects) => {
if (activeChild === 'Viewed') {
return activity.item_url;
}
return activity.sql
? `/superset/sqllab?savedQueryId=${activity.id}`
: activity.url;
};
const getFilterTitle = (activity: ActivityObjects) => {
if (activity.dashboard_title) return activity.dashboard_title;
if (activity.label) return activity.label;
if (activity.url && !activity.table) return activity.item_title;
if (activity.item_title) return activity.item_title;
return activity.slice_name;
};

const getIconName = (activity: ActivityObjects) => {
if (activity.sql) return 'sql';
if (
activity.url?.includes('dashboard') ||
activity.item_url?.includes('dashboard')
) {
return 'nav-dashboard';
}
if (
activity.url?.includes('explore') ||
activity.item_url?.includes('explore')
) {
return 'nav-charts';
}
return '';
};

const ActivityTableRow: React.FC<ActivityProps> = ({
loading,
activeChild,
activityList,
}) => (
<>
{activityList.map(activity => (
<CardStyles
key={`${activity.item_title}__${activity.time}`}
onClick={() => {
window.location.href = getRecentRef(activeChild, activity);
}}
>
<ListViewCard
loading={loading}
cover={<></>}
url={
activity.sql
? `/superset/sqllab?savedQueryId=${activity.id}`
: activity.url
}
title={getFilterTitle(activity)}
description={`Last Edited: ${moment(
activity.changed_on_utc,
'MM/DD/YYYY HH:mm:ss',
)}`}
avatar={getIconName(activity)}
actions={null}
/>
</CardStyles>
))}
</>
);

export default ActivityTableRow;