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

Add option to pass block to controller's #save! #73

Merged
merged 3 commits into from
Oct 30, 2023

Conversation

s-andringa
Copy link

@s-andringa s-andringa commented Oct 18, 2023

Hi @pond, it's me again.

This time I extended ActiveRecordBackedResourcesController#save! to accept a block, so that subclasses may override the default record.save! call with their own saving logic, without having to reimplement the rescue clause for handling uniqueness errors. Also, it aligns nicely with the destroy action, which already accepts a block for a very similar reasons.

In our case we would like a service object to save the record and do some other stuff, because we do not want to employ AR callbacks for that other stuff. With these changes it can look like this:

def save!(record)
  super do
    Services::ManagedUser::Save.new(record).call
  end
end

This will be helpful in many situations. But sometimes it's not enough.

We are trying to make things work with an ActiveModel model that wraps an ActiveRecord record. It mostly delegates stuff to the wrapped record, but it has its own additional validations. That means that we'll need to rescue ActiveModel::ValidationError next to ActiveRecord::InvalidRecord. Therefore we need to override the rescue clause in #save!, but we'd still like to avoid copy-pasting all of the invalid record handling logic. Therefore I have extracted that logic to a separate method. This way we only need to copy-paste one method call, and we don't have to override the actual logic:

def save!(record)
  Services::ManagedUser::Save.new(record).call
rescue ActiveModel::ValidationError, ActiveRecord::InvalidRecord
  handle_invalid_record(record)
end

In short, both changes just add a bit more extensibility to the controller.

I would love to see this merged in one form or another. Thanks!

Copy link
Member

@pond pond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great & only requesting changes on RDoc on the new method as I have to be a stickler for details like that in a public gem where docs are particularly important.

No other changes tho, on the money overall! Thanks.

@s-andringa
Copy link
Author

Thanks @pond. I just pushed the RDoc comment for #handle_invalid_record.

Did you have a chance to look at my other PR #71 ?

@pond
Copy link
Member

pond commented Oct 30, 2023

@s-andringa Yes, sorry it took a while. Only major request you'll see over there is a README.md addition.

Merging here, it's good to go. Thanks! I'll compile a few of the current PRs together and hope to do a new gem version release pretty soon.

@pond pond merged commit 773df2d into RIPAGlobal:main Oct 30, 2023
4 checks passed
@s-andringa
Copy link
Author

I'll compile a few of the current PRs together and hope to do a new gem version release pretty soon.

That'd be great. Thank you.

@pond
Copy link
Member

pond commented Nov 15, 2023

This is now available via RubyGems in v2.6.0 / v1.7.0.

@s-andringa
Copy link
Author

Great, thanks @pond!

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

Successfully merging this pull request may close these issues.

None yet

2 participants