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

feature: Allow custom destroy methods. #1818

Merged
merged 4 commits into from
Jul 4, 2023

Conversation

davekruse
Copy link
Contributor

Description

This is a proposed feature to add custom destroy methods to resources. This makes Avo compatible with soft delete gems like discard. Please reject if not appropriate.

# widget_resource.rb
...
  self.destroy_record_method = -> (record:) {
    record.discard
  }
...

If not defined, the default destroy! is used.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Manual reviewer: please leave a comment with output from the test if that's the case.

@codeclimate
Copy link

codeclimate bot commented Jun 30, 2023

Code Climate has analyzed commit fc7b7d0 and detected 0 issues on this pull request.

View more on Code Climate.

@davekruse davekruse changed the title Allow custom destroy methods. feature: Allow custom destroy methods. Jun 30, 2023
@adrianthedev
Copy link
Collaborator

adrianthedev commented Jul 2, 2023

Could this be achieved by overriding the destroy_model method in a specific controller?

class UsersController
  ...
  def destroy_model
    perform_action_and_record_errors do
      @model.destroy!
    end
  end
end

In that case, I would merge a PR where we would extract that method so we don't have to wrap it in the perform_action_and_record_errors block.

class UsersController
  def destroy_model
    perform_action_and_record_errors do
	  destroy_record_action
    end
  end

  def destroy_record_action
    @model.destroy!
  end
end

Same with save_model.

@davekruse
Copy link
Contributor Author

@adrianthedev yes, perfect.

Now, customizing the delete method is as simple as:

class UsersController
  ...
  def destroy_record_action
    @model.discard
  end
end

Is that what you had in mind?

@davekruse
Copy link
Contributor Author

What's the preferred naming convention in this case where we are straddling naming conventions for the model and the resource? Within destroy_model and save_model, should it be:

  • destroy_record_action and save_record_action
  • destroy_model_action and save_model_action

@adrianthedev
Copy link
Collaborator

So, in the begining we used model everywhere, but that's not the best name for that. A better name would be record (pundit uses that and other gems too).
In Avo 3 we migrated to record, but in Avo 2 we still have a few places where we use model.

Let's use model here too as others might have overridden that method in their controller.

@davekruse
Copy link
Contributor Author

Since the existing destroy_model and save_model are remaining the same, they are backwards compatible - current overrides won't be broken by this change.

For the new methods, we shouldn't have conflict with existing implementations. But for forward compatibility looking ahead to v3, would it make sense to keep 'resource' in the names so that overrides of the method could carry forward without change?

Or do you prefer a consistent use of 'model' throughout v2?

Lemme know and I'll make it so.

@adrianthedev
Copy link
Collaborator

Yes. I would prefer to use model in v2.

save_model -> save_model_action

@adrianthedev adrianthedev merged commit 84c5f11 into avo-hq:main Jul 4, 2023
9 of 10 checks passed
@adrianthedev
Copy link
Collaborator

Thank you for the contribution. I'll cut a release in a few hours as I have another PR almost ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants