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.

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

Make Table Columns & Metrics Bulk-actionable #3929

Merged
merged 1 commit into from Nov 26, 2017

Conversation

alanmcruickshank
Copy link
Contributor

Adds the DeleteMixin to both the sqlalchemy table column view and the SQL metric view to allow mass column management. Very useful for removing large numbers of auto-generated fields.

@mistercrunch mistercrunch merged commit 17635e1 into apache:master Nov 26, 2017
@rhunwicks
Copy link
Contributor

@alanmcruickshank @mistercrunch I'm not certain (because I am not a JavaScript developer), but I think I have a problem caused by this on my instance where attempting to delete Metrics gives an error like:

        Traceback (most recent call last):
  File "/usr/local/lib/python3.6/site-packages/flask/app.py", line 1982, in wsgi_app
    response = self.full_dispatch_request()
  File "/usr/local/lib/python3.6/site-packages/flask/app.py", line 1614, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/usr/local/lib/python3.6/site-packages/flask/app.py", line 1517, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "/usr/local/lib/python3.6/site-packages/flask/_compat.py", line 33, in reraise
    raise value
  File "/usr/local/lib/python3.6/site-packages/flask/app.py", line 1612, in full_dispatch_request
    rv = self.dispatch_request()
  File "/usr/local/lib/python3.6/site-packages/flask/app.py", line 1598, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/usr/local/lib/python3.6/site-packages/flask_appbuilder/views.py", line 574, in action_post
    return action.func(items)
  File "/usr/src/app/superset/views/base.py", line 284, in muldelete
    self._delete(item.id)
AttributeError: 'NoneType' object has no attribute 'id'

If I look at the network requests issued by the browser, I can see:

POST http://localhost:8000/tablecolumninlineview/action_post 500

I.e. the browser is posting to tablecolumninlineview even though the Metrics tab is the one selected.

I can see that selecting the Delete option from the Actions drop down is running:

onclick="return modelActions.execute_multiple('muldelete','Delete all Really?');"

But I can't see how it work's out which model it is using.

It should be easy to test on your instance(s) just by looking at the network requests the browser is issuing and working out if they apply to the correct model in all cases.

@rhunwicks
Copy link
Contributor

rhunwicks commented Dec 3, 2017

Note that if there are actually Columns for the same Table that have the same pk as the Metrics you are deleting, then I think it will delete those instead and return successfully.

@alanmcruickshank
Copy link
Contributor Author

@rhunwicks Looking at the request it sends, it's not currently dangerous - i.e. it won't delete Tables with the same pk (it's passing through a variable with the wrong name to do that - hence the error).

You are however completely right that this isn't working as intended. Thanks for reporting!

I'll take a look at it this week.

@rhunwicks
Copy link
Contributor

rhunwicks commented Dec 3, 2017

@alanmcruickshank on my setup I think it is deleting Columns that have the same pk as the Metric you are trying to delete, rather than the Table. It's not very likely, but I think it is dangerous - because I noticed it when I tried to delete all the Metrics and found that I had deleted all the columns for the Table instead.

@alanmcruickshank
Copy link
Contributor Author

@rhunwicks Right you are - I've just been digging into why this happens.

The issue we've got is tied in with FAB. On clicking the apply actions button it submits a hidden form called #action_form, and in this case there are several of those to choose from. One for TableColumnInlineView and one for SqlMetricInlineView. Depending on which form you're on it might pick the wrong one.

I've added a pull request to roll this back for now. I'll also raise an issue with FAB to see if we can get this fixed.

michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.21.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.21.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants