fix(AlertReportModal): Text Area Change

[AAfghahi]

SUMMARY

There was an error report where users were reporting that sql values that they were inputting into the TextAreaControl section would linger even when they closed out and went to a new report, or even created a new report. This was because we were storing the sql in state, but not clearing state when a modal was closed.

This state was created to stop cursor errors here: #15820. I believe that @geido made this change.

The cursor errors were occurring because we added a debouncer to the ace-editor here: #10837

In investigating this, I was wondering why we had a debouncer for the ace editor to begin with? The input is not using an API, and looking through the different places where TextAreaControl is used, none of them make an API call. So this PR deletes the debouncer, which fixes both the cursor back issue and also gets rid of the state.value //prop.value difference.

However, I wanted to make sure that getting rid of this debouncer is fine @ktmud and @geido ? Where are the places that we should check to make sure?

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

CleanShot.2021-10-06.at.11.24.23.mp4

After:
https://user-images.githubusercontent.com/48933336/138182251-f5d5dfc0-c7dc-4066-a39a-b1b2b1b0cffd.mov

TESTING INSTRUCTIONS

Create an alert, exit out. Create another alert, there should not be any sql in the sql input form.

ADDITIONAL INFORMATION

• Has associated issue:
• 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

[geido]

@AAfghahi I would like to get the test env up. Please let me know when the CI passes. Thank you

[AAfghahi]

@geido
I just fixed this test, so it should be passing. But I will ping you when the tests are fully passing (hoping that they are passing by the time you see this message ;) )

[codecov]

Codecov Report

Merging #17176 (8846c7f) into master (dd71035) will increase coverage by 0.07%.
The diff coverage is 94.59%.

❗ Current head 8846c7f differs from pull request most recent head 5bab212. Consider uploading reports for the commit 5bab212 to get more accurate results

@@            Coverage Diff             @@
##           master   #17176      +/-   ##
==========================================
+ Coverage   76.95%   77.03%   +0.07%     
==========================================
  Files        1038     1037       -1     
  Lines       55612    55621       +9     
  Branches     7590     7594       +4     
==========================================
+ Hits        42798    42845      +47     
+ Misses      12564    12526      -38     
  Partials      250      250              

Flag	Coverage Δ
javascript	71.15% <96.82%> (+0.13%)	⬆️

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

Impacted Files	Coverage Δ
...t-frontend/src/components/AsyncAceEditor/index.tsx	88.00% <ø> (-0.47%)	⬇️
...nd/src/dashboard/components/nativeFilters/types.ts	100.00% <ø> (ø)
...perset-frontend/src/explore/components/Control.tsx	76.47% <ø> (ø)
...nts/controls/ConditionalFormattingControl/types.ts	100.00% <ø> (ø)
...rc/explore/components/controls/TextAreaControl.jsx	91.66% <ø> (+9.31%)	⬆️
superset/annotation_layers/schemas.py	100.00% <ø> (ø)
superset/connectors/sqla/models.py	87.39% <75.00%> (-0.09%)	⬇️
...Filters/FilterBar/FilterControls/FilterControl.tsx	94.87% <88.88%> (-5.13%)	⬇️
...end/src/components/Datasource/DatasourceEditor.jsx	76.09% <100.00%> (+3.98%)	⬆️
...onfigModal/FiltersConfigForm/FiltersConfigForm.tsx	69.32% <100.00%> (+0.07%)	⬆️
... and 18 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 dd71035...5bab212. Read the comment docs.

[eschutho]

/testenv up

[github-actions]

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

[ktmud]

In general it's preferable to always add debounce to text inputs because even if they don't trigger remote API calls, they could still trigger expensive re-rendering. Not sure if this will work, but an alternative solution here could be to re-render the modal completely when users go to a new report, which can probably be achieved by setting a uniq key to the TextAreaControl component.

[AAfghahi]

@ktmud Hey! So I followed your advice and added a key to the AlertReportModal which really helped thank you for that! I ended up not doing the key to the TextAreaControl and instead put it on the entire modal.

Regarding your point on the debouncer, doing some research I found that the react-ace editor has its own internal state for value and also has a built in debouncer, so I made the component into an uncontrolled component (though it still spreads props so the other places that use it will still be able to pass in value).

This did require us to bump up react-ace because our current version had a known bug that finished. One thing to note is that this does drop bracejs as a dependency, but we have bracejs in our package.json so I don't think it will affect us in that way.

[eschutho]

this looks like it will never change value if you type in it?

[AAfghahi]

You mentioned this in our pair programming as a potential issue so I tested it. I removed language from where we are invoking it in the AlertReportModal and got this

[eschutho]

does this generate a new key each time it rerenders?

[AAfghahi]

yes, and I actually moved it over to the AlertReportModal because of this. Earlier I had done this and the state issue had still persisted, but when I test it now it is working fine. So I must have originally tested it during a bad iteration.

[eschutho]

There was a bug that I fixed on sql lab where if you passed in value as null (not undefined), sql lab would crash. Can you test to see if this was fixed with the new version of aceeditor?

[eschutho]

This look really good. Just a few small questions since this component is used throughout the app.

[eschutho]

/testenv up FEATURE_ALERT_REPORTS=true

[github-actions]

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

[rosemarie-chiu]

@AAfghahi @eschutho I'm not sure if this is related to this fix, I found that I no longer able to see the query on the virtual dataset after this code change.

Steps:

1. In Sql editor, run a query and explore save as new dataset
2. Check the sql on the dataset and not able to see anything
3. Only able to see the query on that virutal dataset when clicking on view in sql lab

sql.editor.of.dataset.mov

I tested on other Ephemeral env and do not reproduce this, so I suspect it is caused by this PR.

✅ regression test creating new alert, edit alert sql editor field not overwrites
✅ regression test sql editor save query, update saved query, multiple query tab

[eschutho]

/testenv up ENABLE_TEMPLATE_PROCESSING=True FEATURE_ALERT_REPORTS=true

[eschutho]

@AAfghahi @eschutho I'm not sure if this is related to this fix, I found that I no longer able to see the query on the virtual dataset after this code change.

Thanks @rosemarie-chiu! Nice catch... We'll take a look.

[github-actions]

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

[AAfghahi]

@rosemarie-chiu you're amazing thank you! I made a change that catches that instance.

[eschutho]

/testenv up ENABLE_TEMPLATE_PROCESSING=True FEATURE_ALERT_REPORTS=true

[eschutho]

looks great!

[github-actions]

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

[rosemarie-chiu]

LGTM! QA sign-off tested:
✅ SQL editor in sql lab
✅ SQL editor in alert
✅ SQL editor in dataset edit modal

[github-actions]

Ephemeral environment shutdown and build artifacts deleted.

[Renderz]

I wonder why to remove 'props.value' from TextArea, which should always render the value be given.
Suppose I want to manually change the datasource instead of typing in TextArea itself, value should be injected from props, and should also be rendered correctly.