Bug #6880

[Workbench] Remove delete functionality from Users table

Added by Ward Vandewege over 4 years ago. Updated over 4 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Radhika Chippada
Category:
Workbench
Target version:
Start date:
08/06/2015
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
0.5

Description

Story

Removing users from Arvados is impractical. Workbench currently offers a delete ("trash") button in the users table, but it basically never works, because the API server refuses the request. Remove this entire column of buttons from the users index in Workbench.

Original bug report

There's a delete icon next to each user on the admin users page. Clicking it does nothing, because the ajax call fails (without reporting an error to the user):

#<ActiveRecord::DeleteRestrictionError: Cannot delete record because of dependent logs>
/var/www/arvados-api/shared/vendor_bundle/ruby/2.1.0/gems/activerecord-3.2.17/lib/active_record/associations/builder/has_many.rb:63:in `block in define_restrict_dependency_method'
/var/www/arvados-api/shared/vendor_bundle/ruby/2.1.0/gems/activesupport-3.2.17/lib/active_support/callbacks.rb:506:in `_run__3002657973961559811__destroy__2941283170573855482__callbacks'
/var/www/arvados-api/shared/vendor_bundle/ruby/2.1.0/gems/activesupport-3.2.17/lib/active_support/callbacks.rb:405:in `__run_callback'
/var/www/arvados-api/shared/vendor_bundle/ruby/2.1.0/gems/activesupport-3.2.17/lib/active_support/callbacks.rb:385:in `_run_destroy_callbacks'
/var/www/arvados-api/shared/vendor_bundle/ruby/2.1.0/gems/activesupport-3.2.17/lib/active_support/callbacks.rb:81:in `run_callbacks'
/var/www/arvados-api/shared/vendor_bundle/ruby/2.1.0/gems/activerecord-3.2.17/lib/active_record/callbacks.rb:254:in `destroy'
/var/www/arvados-api/shared/vendor_bundle/ruby/2.1.0/gems/activerecord-3.2.17/lib/active_record/transactions.rb:254:in `block in destroy'
/var/www/arvados-api/shared/vendor_bundle/ruby/2.1.0/gems/activerecord-3.2.17/lib/active_record/transactions.rb:313:in `block in with_transaction_returning_status'
/var/www/arvados-api/shared/vendor_bundle/ruby/2.1.0/gems/activerecord-3.2.17/lib/active_record/connection_adapters/abstract/database_statements.rb:192:in `transaction'
/var/www/arvados-api/shared/vendor_bundle/ruby/2.1.0/gems/activerecord-3.2.17/lib/active_record/transactions.rb:208:in `transaction'
/var/www/arvados-api/shared/vendor_bundle/ruby/2.1.0/gems/activerecord-3.2.17/lib/active_record/transactions.rb:311:in `with_transaction_returning_status'
/var/www/arvados-api/shared/vendor_bundle/ruby/2.1.0/gems/activerecord-3.2.17/lib/active_record/transactions.rb:254:in `destroy'
/var/www/arvados-api/current/app/controllers/application_controller.rb:133:in `destroy'


Subtasks

Task #6917: Review branch 6880-remove-user-delete-buttonResolvedRadhika Chippada


Related issues

Related to Arvados - Bug #3628: [Workbench] The "delete" button in users page is not workingDuplicate

Associated revisions

Revision 427d9052
Added by Radhika Chippada over 4 years ago

closes #6880
closes #6916
Merge branch '6880-remove-user-delete-button'

History

#1 Updated by Ward Vandewege over 4 years ago

  • Description updated (diff)

#2 Updated by Ward Vandewege over 4 years ago

  • Description updated (diff)

#3 Updated by Brett Smith over 4 years ago

So what do you want?

  • The Workbench delete interface gets removed
  • The Workbench delete interface reports the error clearly
  • The Workbench delete interface does more work to make the delete succeed (this might be difficult or even impossible, haven't dug into it)
  • The API server does more work to make the delete succeed
  • We change the semantics of API deletes (and a "deleted" attribute to users or whatever)

#4 Updated by Radhika Chippada over 4 years ago

When I originally identified this issue #3628, Tom suggested that we should not provide the delete button in the users page.

#5 Updated by Brett Smith over 4 years ago

  • Subject changed from [API] deleting users from the admin interface is broken to [Workbench] Remove delete functionality from Users table
  • Description updated (diff)
  • Category set to Workbench
  • Target version changed from Bug Triage to 2015-08-19 sprint
  • Story points set to 0.5

Adjusting per Radhika's comment.

#6 Updated by Radhika Chippada over 4 years ago

  • Assigned To set to Radhika Chippada

#7 Updated by Radhika Chippada over 4 years ago

  • Status changed from New to In Progress

#8 Updated by Radhika Chippada over 4 years ago

While testing this very simple "remove Delete button" implementation, I noticed that the "Home" link is shown on "all" pages in admin settings. Filed #6916.

Fixing this also as part of this issue. And adding controller tests, which should have been written when #6234 was initially implemented.

#9 Updated by Radhika Chippada over 4 years ago

Tom said in IRC:

11:14 radhika: we really need to kick the "writing logic and local variables in views" habit. This is how copy-and-paste starts. Perhaps we can have a "deletable?" instance method (defaulting to calling editable?, and override in user to return false), and call that from the delete_object_button partial? Then the generic view can go back to being generic.

#10 Updated by Radhika Chippada over 4 years ago

Branch 6880-remove-user-delete-button at 467b636f7d1b34f7695f55af972ae90132fc8063

  • Contains fix for this issue as well as 6916. The tests and the logic were too close and hence was productive to in one stretch
  • Added a deletable? to arvados_base.rb per Tom's suggestion and used it in delete_object_button partial
  • While at it, also created a show_home_button partial and moved the logic to show the Home button into it

#11 Updated by Tom Clegg over 4 years ago

I think this

if object.editable? and object.deletable?

should just be

if object.deletable?

(if a model someday claims that it is deletable but not editable, then it would be appropriate for the view to offer a "delete" button)

The "=false" part of expect_home_link=false and expect_delete_link=false in the tests seems to be unused (which is a good thing) so could be dropped.

With those, LGTM, thanks!

#12 Updated by Radhika Chippada over 4 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 0 to 100

Applied in changeset arvados|commit:427d9052d59ca7819acba9fb2e5f381d3e44a53e.

Also available in: Atom PDF