I added function of soft-delete. #2878

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
7 participants

I added function of soft-delete.
So, please consider merging from my repository if possible.

This code support general gems of soft-delete.

  • paranoia
  • acts_as_paranoid

example

ActiveAdmin.register Nanika do
  actions :all
  soft_delete_actions
end

nanika

added action and batch of soft-delete and restore and hard-delete.

problem

  • I created config/locales/en.yml and ja.yml, because I can't translate locale file without them.

I'm poor at English.
If you can't understand what I mean, please let me know.
Sorry for any inconvinience I may cause you.

Owner

seanlinsley commented Feb 17, 2014

First off, thank you for implementing such a great feature! I should have the time to review this in the coming week.

Owner

timoschilling commented Oct 24, 2014

Please smash all double changes into one. Often you go from A to B to C, change it to from A to C, its better to see what you are changing.

And please make a rebase onto master.

Owner

timoschilling commented Nov 2, 2014

hard delete should be the same like delete, no extra translations / abilities / action for that.

alfa-jpn commented Nov 4, 2014

Sorry for the late reply.
I will fix the issues, and send pullrequest.

Owner

timoschilling commented Nov 7, 2014

Yes, please do that, but don't create a new PR. Force push this one.

@timoschilling timoschilling commented on an outdated diff Nov 7, 2014

lib/active_admin/soft_delete.rb
+module ActiveAdmin
+ module SoftDelete
+ module DSL
+ # Add soft delete actions to resource.
+ # (paranoia and acts_as_paranoid)
+ #
+ # @param opts [Hash] options.
+ # default options
+ # {
+ # scopes: true, # create scopes of soft_delete.
+ # soft_delete: true, # create action of soft_delete.
+ # restore: true, # create action of restore.
+ # hard_delete: true # create action of hard_delete.
+ # }
+ #
+ def soft_delete_actions(opts = {})
@timoschilling

timoschilling Nov 7, 2014

Owner

call it options not opts, rename it to soft_delete

@timoschilling timoschilling commented on an outdated diff Nov 7, 2014

lib/active_admin/soft_delete.rb
+ # default options
+ # {
+ # scopes: true, # create scopes of soft_delete.
+ # soft_delete: true, # create action of soft_delete.
+ # restore: true, # create action of restore.
+ # hard_delete: true # create action of hard_delete.
+ # }
+ #
+ def soft_delete_actions(opts = {})
+ return unless @resource.respond_to?(:paranoid?) and @resource.paranoid?
+
+ controller do
+ def resource; active_admin_config.resource_class.unscoped{ super } end
+ end
+
+ if opts[:scopes].nil? || opts[:scopes]
@timoschilling

timoschilling Nov 7, 2014

Owner

if options[:scopes]
While nil is a false value. And the same on the next ifs.

@timoschilling timoschilling commented on an outdated diff Nov 7, 2014

lib/active_admin/soft_delete.rb
+ module SoftDelete
+ module DSL
+ # Add soft delete actions to resource.
+ # (paranoia and acts_as_paranoid)
+ #
+ # @param opts [Hash] options.
+ # default options
+ # {
+ # scopes: true, # create scopes of soft_delete.
+ # soft_delete: true, # create action of soft_delete.
+ # restore: true, # create action of restore.
+ # hard_delete: true # create action of hard_delete.
+ # }
+ #
+ def soft_delete_actions(opts = {})
+ return unless @resource.respond_to?(:paranoid?) and @resource.paranoid?
@timoschilling

timoschilling Nov 7, 2014

Owner

Make a Raise with a description about whats necessary

@timoschilling timoschilling commented on an outdated diff Nov 7, 2014

lib/active_admin/soft_delete.rb
+ resource.destroy!
+ end
+ end
+ end
+
+ private
+ # Add action and batch_action.
+ #
+ # @param title [Symbol] title of action and batch_action.
+ # @param options [Hash] options.
+ # default
+ # {
+ # method: :get # method of action.
+ # }
+ # @param &block [Proc{|resource|}] process of resource.
+ def add_action_and_batch(title, options = {}, &block)
@timoschilling

timoschilling Nov 7, 2014

Owner

name it add_member_and_batch_action

@timoschilling timoschilling commented on an outdated diff Nov 7, 2014

config/locales/en.yml
delete_confirmation: "Are you sure you want to delete this?"
+ soft_delete_confirmation: "Are you sure you want to soft-delete this?"
+ restore_confirmation: "Are you sure you want to restore this?"
+ hard_delete_confirmation: "Are you sure you want to hard-delete this?"
@timoschilling

timoschilling Nov 7, 2014

Owner

please sort this lines like:
delete
delete_confirmation
soft_delete
soft_delete_confirmation
restore
restore_confirmation
hard_delete
hard_delete_confirmation

@timoschilling timoschilling commented on an outdated diff Nov 7, 2014

lib/active_admin/soft_delete.rb
+ # (paranoia and acts_as_paranoid)
+ #
+ # @param opts [Hash] options.
+ # default options
+ # {
+ # scopes: true, # create scopes of soft_delete.
+ # soft_delete: true, # create action of soft_delete.
+ # restore: true, # create action of restore.
+ # hard_delete: true # create action of hard_delete.
+ # }
+ #
+ def soft_delete_actions(opts = {})
+ return unless @resource.respond_to?(:paranoid?) and @resource.paranoid?
+
+ controller do
+ def resource; active_admin_config.resource_class.unscoped{ super } end
@timoschilling

timoschilling Nov 7, 2014

Owner

please don't write this in one line

@timoschilling timoschilling and 1 other commented on an outdated diff Nov 7, 2014

activeadmin.gemspec
@@ -30,4 +30,6 @@ Gem::Specification.new do |s|
s.add_dependency "rails", ">= 3.2", "< 4.1"
s.add_dependency "ransack", "~> 1.0"
s.add_dependency "sass-rails"
+
+ s.add_development_dependency "rb-readline" # ruby2.x need this gem, when run a test.
@timoschilling

timoschilling Nov 7, 2014

Owner

Please don't at this to the gemspec.
Why is this needed? On my machine ruby 2.0/2.1 runs the test without this line.

@alfa-jpn

alfa-jpn Nov 7, 2014

This was my misunderstanding.

alfa-jpn commented Nov 7, 2014

I improved the code.

  • Improved flexibility in the DSL soft_delete_actions.
  • Sorted yml keys.
  • Merged current master.
  • Unit Test support the formats of RSpec3.

@timoschilling timoschilling commented on an outdated diff Nov 7, 2014

lib/active_admin/soft_delete.rb
+ # @yield [action, resource] If you don't use a compatible acts_as_paranoid.
+ # @yieldparam [Symbol] action action name (:soft_delete or :restore or :hard_delete).
+ # @yieldparam [ActiveRecord::Base] resource target resource.
+ #
+ # @example If you don't use a compatible acts_as_paranoid. (current paranoia)
+ # soft_delete_actions do |action, resource|
+ # case action
+ # when :soft_delete
+ # resource.destroy
+ # when :restore
+ # resource.restore
+ # when :hard_delete
+ # resource.really_destroy!
+ # end
+ # end
+ def soft_delete_actions(options = {})
@timoschilling

timoschilling Nov 7, 2014

Owner

rename it to soft_delete

@timoschilling timoschilling commented on an outdated diff Nov 7, 2014

lib/active_admin/soft_delete.rb
+ # resource.destroy
+ # when :restore
+ # resource.restore
+ # when :hard_delete
+ # resource.really_destroy!
+ # end
+ # end
+ def soft_delete_actions(options = {})
+ check_acts_as_paranoid! unless block_given?
+
+ options = {
+ scopes: true,
+ soft_delete: true,
+ restore: true,
+ hard_delete: true
+
@timoschilling

timoschilling Nov 7, 2014

Owner

remove this empty line

@timoschilling timoschilling commented on an outdated diff Nov 7, 2014

lib/active_admin/soft_delete.rb
+ end
+ end
+
+ if options[:hard_delete]
+ add_member_and_batch_action :hard_delete, :method => :delete do |resource|
+ (block_given?) ? yield(:hard_delete, resource) : resource.destroy!
+ end
+ end
+ end
+
+ private
+ # Add action and batch_action.
+ #
+ # @param title [Symbol] title of action and batch_action.
+ # @param options [Hash] options.
+ # default
@timoschilling

timoschilling Nov 7, 2014

Owner

yard style like in soft_delete method

@timoschilling timoschilling commented on an outdated diff Nov 7, 2014

lib/active_admin/soft_delete.rb
+ scope :only_deleted # only soft_deleted records..
+ end
+
+ if options[:soft_delete]
+ add_member_and_batch_action :soft_delete, :method => :delete do |resource|
+ (block_given?) ? yield(:soft_delete, resource) : resource.destroy
+ end
+ end
+
+ if options[:restore]
+ add_member_and_batch_action :restore, :method => :put do |resource|
+ (block_given?) ? yield(:restore, resource) : resource.recover
+ end
+ end
+
+ if options[:hard_delete]
@timoschilling

timoschilling Nov 7, 2014

Owner

we don't need a hard delete, that should handled by destroy, i think we should overwrite them

@timoschilling timoschilling commented on an outdated diff Nov 7, 2014

lib/active_admin/soft_delete.rb
+ # when :hard_delete
+ # resource.really_destroy!
+ # end
+ # end
+ def soft_delete_actions(options = {})
+ check_acts_as_paranoid! unless block_given?
+
+ options = {
+ scopes: true,
+ soft_delete: true,
+ restore: true,
+ hard_delete: true
+
+ }.merge(options)
+
+ remove_scope_of_controller_resource
@timoschilling

timoschilling Nov 7, 2014

Owner

Thats not a good option, we can't make a "global" unscope

Owner

timoschilling commented Nov 7, 2014

Please apply my notes.
Merge master in a PR is a bad way, please use a rebase onto master. And squash your commits into one.

alfa-jpn commented Nov 8, 2014

Fixed issues.

@timoschilling timoschilling commented on an outdated diff Nov 8, 2014

spec/unit/soft_delete_spec.rb
+ describe 'MemberActions' do
+ it 'Defined #soft_delete' do
+ expect(admin_resource.controller.public_instance_methods).to include(:soft_delete)
+ end
+
+ it 'Defined #restore' do
+ expect(admin_resource.controller.public_instance_methods).to include(:restore)
+ end
+
+ context 'When call action' do
+ subject do
+ controller.send(action)
+ end
+
+ context '#soft_delete' do
+ let :action do
@timoschilling

timoschilling Nov 8, 2014

Owner

where does we use this action variable? Same for destroy and restore.

@timoschilling timoschilling commented on the diff Nov 8, 2014

spec/unit/soft_delete_spec.rb
+ end
+
+ it 'Receive #recover and redirect_to' do
+ expect(resource).to receive(:recover).twice
+ expect(controller).to receive(:redirect_to).with('This is dummy path.',
+ :notice => I18n.t("active_admin.batch_actions.succesfully_restored",
+ :count => 2,
+ :model => config.resource_label.downcase,
+ :plural_model => config.plural_resource_label(:count => 2).downcase)).once
+ subject
+ end
+ end
+
+
+ context '#destroy' do
+ let :action do
@timoschilling

timoschilling Nov 8, 2014

Owner

is this needed?

@alfa-jpn

alfa-jpn Nov 8, 2014

Forgot this...

I will remove this.

@alfa-jpn

alfa-jpn Nov 8, 2014

I misunderstood.
The action variable of this side is necessary.
The batch_action(L120-123) variable uses this.

let :batch_action do
  admin_resource.batch_actions.select{|act| act.sym == action}.first
end

@timoschilling timoschilling commented on an outdated diff Nov 8, 2014

lib/active_admin/views/index_as_table.rb
links
end
+
+ # Check if record was soft deleted.
+ # @param resource [Activerecord::Base] record.
+ # @return [Boolean] true if record was soft deleted.
+ def soft_destroyed?(resource)
+ case
+ when resource.respond_to?(:deleted?)
@timoschilling

timoschilling Nov 8, 2014

Owner

please write case and when with the same indent.

@timoschilling timoschilling commented on an outdated diff Nov 8, 2014

lib/active_admin/soft_delete.rb
+ # Add member_action and batch_action.
+ # @raise When yield block is nothing.
+ #
+ # @param name [Symbol] name of member and batch_action.
+ # @param options [Hash] options.
+ # @option options [Symbol] :method (:get) HTTP Method.
+ # @option options [String] :preterit ("#{name}d") preterit name.
+ #
+ # @yield [resource] action.
+ # @yieldparam [ActiveRecord::Base] resource target resource.
+ def add_member_and_batch_action(name, options = {})
+ raise 'yield block is nothing.' unless block_given?
+
+ options = {
+ method: :get,
+ preterit: "#{name}d"
@timoschilling

timoschilling Nov 8, 2014

Owner

use only 2 space more indent from options to method

@timoschilling timoschilling commented on an outdated diff Nov 8, 2014

lib/active_admin/soft_delete.rb
+ # case action
+ # when :soft_delete
+ # resource.destroy
+ # when :restore
+ # resource.restore
+ # when :hard_delete
+ # resource.really_destroy!
+ # end
+ # end
+ def soft_delete(options = {})
+ check_acts_as_paranoid! unless block_given?
+
+ options = {
+ scopes: true,
+ soft_delete: true,
+ restore: true,
@timoschilling

timoschilling Nov 8, 2014

Owner

use only 2 space more indent from options to method

@timoschilling timoschilling commented on an outdated diff Nov 8, 2014

lib/active_admin/soft_delete.rb
+ check_acts_as_paranoid! unless block_given?
+
+ options = {
+ scopes: true,
+ soft_delete: true,
+ restore: true,
+ }.merge(options)
+
+ if options[:scopes]
+ scope :only_deleted # only soft_deleted records.
+ include_soft_deleted_records_in_default_scope
+ end
+
+ if options[:soft_delete]
+ add_member_and_batch_action :soft_delete, :method => :delete do |resource|
+ (block_given?) ? yield(:soft_delete, resource) : resource.destroy
@timoschilling

timoschilling Nov 8, 2014

Owner

(block_given?) > block_given?

Owner

timoschilling commented Nov 8, 2014

sorry some last things. You can add a documentation section and a changelog entry if you want.

kofronpi commented Nov 8, 2014

This is great work, but shouldn't this be a plugin as this adds a dependency to paranoia which has quite a few bugs ? I remember using it in production lastly and for instance, it was running uniqueness validations on soft-deleted records too.

Owner

timoschilling commented Nov 8, 2014

@kofronpi this implementation is complexly optional, we have only this development dependency for dev/testing

alfa-jpn commented Nov 8, 2014

@kofronpi
It is compatible with acts_as_paranoid, when option is nothing. But, It can be customized by yield.

soft_delete do |action, resource|
  case action
  when :soft_delete
    resource.destroy
  when :restore
    resource.restore
  when :hard_delete
    resource.really_destroy!
  end
end

@timoschilling
I forgot action_item. So, I added it.
Could you review it?
CHANGELOG and Documentation are writing now.

Owner

Fivell commented Jan 14, 2017

@timoschilling , maybe this is candidate for 3d party gem ?

Owner

varyonic commented Jan 15, 2017

I see active_admin_paranoia was written and released during 2015.

Owner

timoschilling commented Jan 17, 2017

closed while released as plugin gem

gregbell removed the 4 - Review label Jan 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment