refactor: ScheduleQueryButton into functional component #13443

AAfghahi · 2021-03-03T18:45:01Z

SUMMARY

Changed the ScheduleQueryButton into a TypeScript function using react hooks.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

eschutho · 2021-03-04T22:37:38Z

docker/pythonpath_dev/superset_config.py

+}
+WTF_CSRF_ENABLED = False
+ENABLE_SCHEDULED_EMAIL_REPORTS = True
+ENABLE_ALERTS = True


you'll want to move this into you superset_config in your root folder so that it's not committed.

is that a folder that I have to create? I don't see it.

hughhhh · 2021-03-08T19:30:56Z

superset-frontend/src/SqlLab/components/ScheduleQueryButton.tsx

+  const [label, setLabel] = useState(defaultLabel);
+  const [showSchedule, setShowSchedule] = useState(false);
+  let saveModal: any;
+  // this is for the ref that is created in the modal trigger. the modal is created in order to use the .close() function on it.


is this comment still valid?

hughhhh · 2021-03-08T19:32:20Z

superset-frontend/src/SqlLab/components/ScheduleQueryButton.tsx

+  less_equal: (a: number, b: number) => a <= b,
+};
+
+function getJSONSchema() {


Suggested change

function getJSONSchema() {

const getJSONSchema = () => {

hughhhh · 2021-03-08T19:32:56Z

superset-frontend/src/SqlLab/components/ScheduleQueryButton.tsx

+  return {};
+}
+
+function getUISchema() {


Suggested change

function getUISchema() {

const getUISchema = () => {

hughhhh · 2021-03-08T19:33:23Z

superset-frontend/src/SqlLab/components/ScheduleQueryButton.tsx

+  return window.featureFlags.SCHEDULED_QUERIES?.UISCHEMA;
+}
+
+function getValidationRules() {


Suggested change

function getValidationRules() {

const getValidationRules = () => {

hughhhh · 2021-03-10T12:16:28Z

superset-frontend/src/SqlLab/components/ScheduleQueryButton.tsx

+import './ScheduleQueryButton.less';
+import Button from 'src/components/Button';
+
+const validators = {


nit: move this to the /src/utils directory

eschutho · 2021-03-19T18:21:02Z

docker/pythonpath_dev/superset_config.py

@@ -78,7 +78,6 @@ class CeleryConfig(object):

 CELERY_CONFIG = CeleryConfig
 SQLLAB_CTAS_NO_LIMIT = True
-


just for code cleanliness, I would revert this change.

eschutho · 2021-03-19T18:27:37Z

superset-frontend/src/SqlLab/components/ScheduleQueryButton.tsx

+  padding-bottom: ${({ theme }) => theme.gridUnit * 2}px;
+`;
+
+const ButtonComponent = styled.div`


where did these styles come from? Are they new or copied? Can we bring the theming into it?

I copied them from what the styled divs looked like. It is because I switched this from being a div to being a button component because there were TypeScript issues with the div component having an onClick and a disabled option.

You can also (for consistency) call this StyledButtonComponent and then instead of use a div to wrap the button, you can do styled(Button)` and then you don't have to wrap this around the button element, but just use this as the tag.
Example:

superset/superset-frontend/src/explore/components/DataTableControl.tsx

Line 38 in 9a05d6a

export const CopyButton = styled(Button)`

(Assume it's called StyledCopyButton though :))

eschutho · 2021-03-19T18:30:53Z

superset-frontend/src/SqlLab/components/ScheduleQueryButton.tsx

+  schema,
+  dbId,
+  onSchedule = () => {},
+  scheduleQueryWarning = null,


per the props above this will never be undefined so you're not going to hit this default. If you feel you need this default because there's not enough type checking going on with the other files, then you can set the typescript prop as optional, but not sure if you need it. You may have to look through the code to see if it's there intentionally.

eschutho · 2021-03-19T18:31:08Z

superset-frontend/src/SqlLab/components/ScheduleQueryButton.tsx

+  dbId,
+  onSchedule = () => {},
+  scheduleQueryWarning = null,
+  tooltip = '',


same thing here

eschutho · 2021-03-19T18:33:06Z

superset-frontend/src/SqlLab/components/ScheduleQueryButton.tsx

+  const [description, setDescription] = useState('');
+  const [label, setLabel] = useState(defaultLabel);
+  const [showSchedule, setShowSchedule] = useState(false);
+  let saveModal: any;


suggestion: can you make this a type of ref or undefined?

Done! What's the reasoning behind it being undefined though?

eschutho · 2021-03-19T18:36:37Z

superset-frontend/src/SqlLab/components/ScheduleQueryButton.tsx

+  const [showSchedule, setShowSchedule] = useState(false);
+  let saveModal: any;
+
+  const onScheduleSubmit = ({ formData }: Record<string, any>) => {


does this formData have a type? Can you do something like ({ formData }: {formData: HTMLFormData})? That's just a guess on the type

is this a second deconstruction? {formData: HTMLFormData}

eschutho · 2021-03-19T19:29:46Z

superset-frontend/src/featureFlags.ts

+  VERSIONED_EXPORT?: boolean;
+  GLOBAL_ASYNC_QUERIES?: boolean;
+  ENABLE_TEMPLATE_PROCESSING?: boolean;
+  ENABLE_EXPLORE_DRAG_AND_DROP?: boolean;


You should be able to do this instead:

export type FeatureFlagMap = { [key in Exclude<FeatureFlag, FeatureFlag.SCHEDULED_QUERIES>]?: boolean; } & { SCHEDULED_QUERIES?: ScheduleQueriesProps; };

this looks way better, thank you so much!

eschutho

few other suggestions..

eschutho · 2021-03-19T22:33:40Z

superset-frontend/src/SqlLab/components/ScheduleQueryButton.tsx

+            <Button
+              onClick={() => setShowSchedule(!showSchedule)}
+              buttonSize="small"
+              tooltip={tooltip}


I think we lost disabled here.. also you can add buttonStyle="link" and it will get you most of the styles that you need.

codecov · 2021-03-23T14:25:01Z

Codecov Report

Merging #13443 (872f808) into master (452b530) will increase coverage by 0.24%.
The diff coverage is 28.12%.

❗ Current head 872f808 differs from pull request most recent head 318ba2a. Consider uploading reports for the commit 318ba2a to get more accurate results

@@            Coverage Diff             @@
##           master   #13443      +/-   ##
==========================================
+ Coverage   77.16%   77.40%   +0.24%     
==========================================
  Files         933      933              
  Lines       47383    47190     -193     
  Branches     5928     5878      -50     
==========================================
- Hits        36563    36528      -35     
+ Misses      10672    10520     -152     
+ Partials      148      142       -6

Flag	Coverage Δ
cypress	`56.26% <14.81%> (+0.03%)`	⬆️
javascript	`63.55% <28.12%> (+0.08%)`	⬆️

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

Impacted Files	Coverage Δ
...perset-frontend/src/components/FormLabel/index.tsx	`100.00% <ø> (ø)`
...tend/src/SqlLab/components/ScheduleQueryButton.tsx	`28.12% <28.12%> (ø)`
superset-frontend/src/chart/chartAction.js	`78.60% <0.00%> (+9.85%)`	⬆️
superset-frontend/src/chart/chartReducer.ts	`66.19% <0.00%> (+17.59%)`	⬆️
superset-frontend/src/middleware/asyncEvent.ts	`78.03% <0.00%> (+24.48%)`	⬆️
superset-frontend/src/dashboard/index.jsx	`100.00% <0.00%> (+60.00%)`	⬆️
superset-frontend/src/explore/index.jsx	`100.00% <0.00%> (+62.50%)`	⬆️

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 452b530...318ba2a. Read the comment docs.

eschutho · 2021-03-23T21:40:34Z

@hughhhh will finish the review on this.. thanks @AAfghahi for everything!

…)" This reverts commit 2c3d9e9.

pull-request-size bot added the size/XL label Mar 3, 2021

AAfghahi force-pushed the ScheduleQueryButton branch 2 times, most recently from e31d8f9 to adfa61d Compare March 4, 2021 22:17

eschutho reviewed Mar 4, 2021

View reviewed changes

pull-request-size bot added size/L and removed size/XL labels Mar 4, 2021

AAfghahi force-pushed the ScheduleQueryButton branch from e3a7513 to b3a288a Compare March 4, 2021 23:54

hughhhh reviewed Mar 8, 2021

View reviewed changes

AAfghahi force-pushed the ScheduleQueryButton branch from b3a288a to f3a1f35 Compare March 8, 2021 23:27

hughhhh reviewed Mar 10, 2021

View reviewed changes

hughhhh approved these changes Mar 10, 2021

View reviewed changes

junlincc added the preset-io label Mar 18, 2021

eschutho reviewed Mar 19, 2021

View reviewed changes

AAfghahi force-pushed the ScheduleQueryButton branch 3 times, most recently from b15046c to 4834d50 Compare March 19, 2021 19:53

pull-request-size bot added size/XL and removed size/L labels Mar 19, 2021

eschutho force-pushed the ScheduleQueryButton branch from 4834d50 to f3a1f35 Compare March 19, 2021 20:25

pull-request-size bot added size/L and removed size/XL labels Mar 19, 2021

eschutho requested changes Mar 19, 2021

View reviewed changes

eschutho reviewed Mar 19, 2021

View reviewed changes

AAfghahi force-pushed the ScheduleQueryButton branch 2 times, most recently from 8e0d4f7 to f5d99a7 Compare March 22, 2021 19:05

pull-request-size bot added size/XL and removed size/L labels Mar 22, 2021

AAfghahi force-pushed the ScheduleQueryButton branch from f5d99a7 to 8cd3177 Compare March 22, 2021 20:23

pull-request-size bot added size/L and removed size/XL labels Mar 22, 2021

AAfghahi force-pushed the ScheduleQueryButton branch from 8cd3177 to e85195e Compare March 22, 2021 20:24

AAfghahi force-pushed the ScheduleQueryButton branch from 42e9ddf to 90473e3 Compare March 23, 2021 14:29

AAfghahi added 5 commits March 23, 2021 10:35

initial change to functional component

f676d67

changed into TypeScript

8b2f44f

changed file configs

82d3f70

worked on changes

5336e87

updated superset-ui

318ba2a

AAfghahi force-pushed the ScheduleQueryButton branch from 90473e3 to 318ba2a Compare March 23, 2021 14:37

hughhhh merged commit 2c3d9e9 into apache:master Mar 24, 2021

hughhhh added a commit that referenced this pull request Mar 24, 2021

Revert "refactor: ScheduleQueryButton into functional component (#13443…

ee106f2

…)" This reverts commit 2c3d9e9.

AAfghahi mentioned this pull request Mar 24, 2021

refactor: New schedule query button #13786

Closed

6 tasks

allanco91 pushed a commit to allanco91/superset that referenced this pull request May 21, 2021

refactor: ScheduleQueryButton into functional component (apache#13443)

d6080c7

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

refactor: ScheduleQueryButton into functional component #13443

refactor: ScheduleQueryButton into functional component #13443

AAfghahi commented Mar 3, 2021

eschutho Mar 4, 2021

AAfghahi Mar 4, 2021

hughhhh Mar 8, 2021

hughhhh Mar 8, 2021

hughhhh Mar 8, 2021 •

edited

Loading

hughhhh Mar 8, 2021

hughhhh Mar 10, 2021

eschutho Mar 19, 2021

eschutho Mar 19, 2021 •

edited

Loading

AAfghahi Mar 19, 2021

eschutho Mar 19, 2021

eschutho Mar 19, 2021

eschutho Mar 19, 2021

eschutho Mar 19, 2021

AAfghahi Mar 22, 2021

eschutho Mar 19, 2021

AAfghahi Mar 22, 2021

eschutho Mar 19, 2021 •

edited

Loading

AAfghahi Mar 19, 2021

eschutho left a comment

eschutho Mar 19, 2021

codecov bot commented Mar 23, 2021 •

edited

Loading

eschutho commented Mar 23, 2021

	function getValidationRules() {
	const getValidationRules = () => {

		@@ -78,7 +78,6 @@ class CeleryConfig(object):

		CELERY_CONFIG = CeleryConfig
		SQLLAB_CTAS_NO_LIMIT = True

refactor: ScheduleQueryButton into functional component #13443

refactor: ScheduleQueryButton into functional component #13443

Conversation

AAfghahi commented Mar 3, 2021

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

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

hughhhh Mar 8, 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

eschutho Mar 19, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho Mar 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eschutho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 23, 2021 • edited Loading

Codecov Report

eschutho commented Mar 23, 2021

hughhhh Mar 8, 2021 •

edited

Loading

eschutho Mar 19, 2021 •

edited

Loading

eschutho Mar 19, 2021 •

edited

Loading

codecov bot commented Mar 23, 2021 •

edited

Loading