feat: add confirmation dialog for imports

[betodealmeida]

SUMMARY

Confirm overwrite before importing an assert that already exists.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

Tested importing an already existing database.

ADDITIONAL INFORMATION

• Has associated issue:
• Changes UI
• Requires DB Migration.
• Confirm DB Migration upgrade and downgrade tested.
• Introduces new feature or API
• Removes existing feature or API

[codecov-io]

Codecov Report

Merging #11983 (683a78c) into master (5d8ecc0) will increase coverage by 0.81%.
The diff coverage is 40.81%.

@@            Coverage Diff             @@
##           master   #11983      +/-   ##
==========================================
+ Coverage   67.04%   67.86%   +0.81%     
==========================================
  Files         948      948              
  Lines       46193    46229      +36     
  Branches     4405     4418      +13     
==========================================
+ Hits        30972    31374     +402     
+ Misses      15095    14748     -347     
+ Partials      126      107      -19     

Flag	Coverage Δ
cypress	54.43% <38.29%> (+5.79%)	⬆️
javascript	62.92% <39.13%> (-0.04%)	⬇️
python	64.57% <ø> (+0.01%)	⬆️

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

Impacted Files	Coverage Δ
...perset-frontend/src/views/CRUD/chart/ChartList.tsx	81.45% <ø> (ø)
...rontend/src/views/CRUD/dashboard/DashboardList.tsx	76.14% <ø> (+0.91%)	⬆️
...tend/src/views/CRUD/data/database/DatabaseList.tsx	86.00% <ø> (+10.00%)	⬆️
...ontend/src/views/CRUD/data/dataset/DatasetList.tsx	68.78% <ø> (ø)
superset-frontend/src/views/CRUD/hooks.ts	63.18% <24.24%> (+1.31%)	⬆️
...rset-frontend/src/components/ImportModal/index.tsx	75.00% <75.00%> (+0.75%)	⬆️
...src/explore/components/controls/MetricsControl.jsx	89.65% <0.00%> (+0.57%)	⬆️
superset/db_engine_specs/presto.py	82.40% <0.00%> (+0.64%)	⬆️
...erset-frontend/src/SqlLab/components/SqlEditor.jsx	51.76% <0.00%> (+1.17%)	⬆️
... and 51 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 5d8ecc0...683a78c. Read the comment docs.

[ktmud]

While this works, we may want to look for other confirmation prompts in the future once i18n becomes more serious.

[betodealmeida]

While this works, we may want to look for other confirmation prompts in the future once i18n becomes more serious.

Sorry, what do you mean?

I just wrote #11987 to improve DELETE, but it would be nice to have more hints for translators to indicate that the strings should match.

[ktmud]

I mean we shouldn't ask non-English speaking users to type English words. #11987 seems like a right step, but as you noted, the translation may not match.

What we should have been doing for a long time is to add unique translation keys for each UI piece and let English reads from the translation file, too.

[hughhhh]

nit: deconstruct these vars

const { loading, passwordsNeeded, alreadyExists } = state;

[betodealmeida]

Here we're returning state:

return {
  state: {
    loading: state.loading,
    passwordsNeeded: state.passwordsNeeded,
    alreadyExists: state.alreadyExists,
  },
  importResource,
};

So I think we could just return:

return {
  state,
  importResource,
}

But I think we want to return a copy of state here, which is why we redefine it ­— I followed the same logic from useSingleViewResource:

https://github.com/apache/incubator-superset/blob/4d329071a1651994574c3ad1f26e32b56d9ff810/superset-frontend/src/views/CRUD/hooks.ts#L302-L305

@riahk any thoughts here?

[riahk]

Was discussing this with @nytai who originally wrote this logic for useListViewResource. I think the original intent was explicitly defining the state variables because some of that hook's state variables are strictly internal. However, in useSingleViewResource we're returning all the state variables, so it should be fine to just import the entire state object (I will probably refactor the hook to do this at some point).

It looks like that's the case here as well so you can easily just import the entire state object like you suggested above.

[betodealmeida]

Thanks, @riahk! I've updated the code.

[hughhhh]

we could make this more modular by making the message live in the component and change the resource name. For example, One or more {resourceName} to be imported already exist. then we wouldn't have to pass the message through the props

[betodealmeida]

The problem is that we need to use plural here ("charts") and resourceLabel is singular, and we can't build the plural programmatically. For example, we could try to do this:

t('One or more %ss to be imported already exist.', resourceLabel);

But in Italian (eg) the plural of chart — grafico — is "grafici", and it would break.

We could change the constructor to receive resourceName and resourcePluralName, but I thought it would be better to have components define the error message, since it allows for customization.