Updates for Attachments

[andrewerlanger]

Enabling updates on associations is one of the many great powers that updates_for confers. This PR takes a stab at extending that power to Active Storage attachments, along the lines of the following:

# models/person.rb
class Person
  has_many_attached :photos, enable_updates: true
end

# views/people/show.html.erb
<h1><%= person.name %></h1>
<%= updates_for person, :photos do %>
  <!-- render images -->
<% end %>

Specifically: it adds an enrich_attachments_with_updates method, which borrows heavily from enrich_association_with_updates to register the attachments within the CollectionsRegistry.

I've done some manual testing and the updates seem to work as you would expect (i.e. in the same way they do for the existing collection logic – has_many :posts, enable_updates: true).

That said, I'm not sure if there are any edge-cases or customizations related to Active Storage that could make this a little brittle. I couldn't find any obvious red flags in the Active Storage docs, but that doesn't mean they don't exist 🤔

[julianrubisch]

Thanks @andrewerlanger for taking this on, this is a great addition that I've been thinking about myself for a long time already 🙏

Two questions right off the bat:

1. wouldn't we actually want to add a proxy for has_one_attached too? though I think that can wait because we don't have one for has_one either at the moment.
2. I'd really like to have some tests for that. 😬
3. EDIT: While we're at it, what would it take to make such an adjustment for ActionText, too? I'm asking that because I was just going to propose to split the code up a bit WRT the individual rails modules... if we make a clever interface for that, doing ActionText probably shouldn't take more than adding just another klass...

[julianrubisch]

This is awesome! The only caveat is that this class has grown a long beard up till now. I think there's an opportunity to factor out some of the Association / ActiveStorage related stuff here... let me ponder that a bit...

[leastbad]

I agree that it would be nice to see support for has_one, has_one_attached and has_rich_text if possible.

Nice work, @andrewerlanger!

[andrewerlanger]

@julianrubisch @leastbad thanks for the quick turnaround on the review 🙏

I'll add a test for has_many_attached for starters, and then look into how we might best add support for has_one, has_one_attached and has_rich_text as well.

Should have something to share by mid-next week :)

[julianrubisch]

@andrewerlanger adding a test should suffice for this one. I'll think about what architecture the different options should have later...

[leastbad]

Just wanted to poke because this PR is cool and I'd love to see it hit the ground running for 5.0.

[julianrubisch]

Just a quick update: @andrewerlanger I checked out your branch to check if I could add a test real quick, but for me it turned out that has_many_attached enable_updates: true didn't broadcast anything.

I dug a bit into ActiveStorage code and it turns out, has_many_attached evals in a has_many "...attachments" here:

https://github.com/rails/rails/blob/de53ba56cab69fb9707785a397a59ac4aaee9d6f/activestorage/lib/active_storage/attached/model.rb#L164

that, I believe, we'd have to set the enable_updates: true flag upon. The only way to do that, actually, would be to monkey patch that whole class_eval, which I'm super uneasy about...

However, that attachment model has touch: true set on the polymorphic parent (https://github.com/rails/rails/blob/de53ba56cab69fb9707785a397a59ac4aaee9d6f/activestorage/app/models/active_storage/attachment.rb#L21), which would mean essentially we could just document to enable updates on the model holding the has_many_attached, which would get updated as soon as something is attached?

Would love to hear everyone's thoughts on this...

[andrewerlanger]

@julianrubisch @leastbad apologies for the delay on this one – I have a nasty habit of overestimating how much free time I'll have up my sleeve whenever I go on vacation 🙈

In any case, I've added a couple of tests for has_many_attached now, which should cover both adding and removing files.

@julianrubisch as for the lack of broadcasting you mention here...

I checked out your branch to check if I could add a test real quick, but for me it turned out that has_many_attached enable_updates: true didn't broadcast anything.

...are you sure this is actually the case? I unfortunately don't have time to dig into it in detail right now, but I tested the extended has_many_attached :images, enable_updates: true logic on a real app when I first wrote it and everything worked as expected as far as I recall.

As always, it's very possible I'm wrong about this though. Please feel free to update the PR accordingly if that's the case here!

[julianrubisch]

It decidedly didn't work for me, but I could have also missed something very basic and would be the happiest person if it did!

I'll take another look!

[julianrubisch]

Alright, I don't know what I was missing there. Merging!