Make ActiveStorage attach method be consistent between one and many #45397
Conversation
There was a problem hiding this comment.
I had not dug into it when I wrote #45345 (comment), but I see now that the behavior we were discussing was added by #44439, and tests were added by #44558. However, the tests only covered 1 of 3 possible branches. I'm not sure if #44439 was motivated by a real-world use case (hence the test coverage), but it's plausible that there is one, so let's roll with it. Thus we should also fix the tests added by #44558 to cover all 3 branches. And, in keeping with the theme of #44439, I think we should return nil instead of false when record.save fails. That way you can write something like @user.avatar.attach(params[:avatar])&.url.
Also, regarding the tests themselves, perhaps I am missing something, but I think the assertions can be simplified as just assert_equal @user.attached_thing, return_value_of_attach, no?
santib
force-pushed
the
activestorage-attach-consistency
branch
from
5952c3c
to
4c1523d
Compare
santib
force-pushed
the
activestorage-attach-consistency
branch
4 times, most recently
from
10fd209
to
98f504b
Compare
santib
force-pushed
the
activestorage-attach-consistency
branch
from
98f504b
to
a4956b4
Compare
| @user.highlights.attach create_blob(filename: "funky.jpg"), create_blob(filename: "town.jpg") | ||
| assert @user.highlights.attached? | ||
| assert 2, @user.highlights.count | ||
| assert_equal "racecar.jpg", @user.highlights.first.filename.to_s | ||
| assert_equal "funky.jpg", @user.highlights.second.filename.to_s |
There was a problem hiding this comment.
@jonathanhefner Look at this test 😅 I pushed it so you can see that it's keeping the amount of files but the wrong ones. I suggest removing (or skipping) this test and tackling this bug separately. WDYT?
There was a problem hiding this comment.
Yes, I would strongly prefer to avoid scope creep. In retrospect, I should have suggested a separate PR in #45397 (comment). Would you mind limiting the scope of this PR to the return value of the attach methods?
There was a problem hiding this comment.
There I removed those tests and I thought it'd be valuable to add a CHANGELOG entry, but let me know if you want to adjust the message or even remove it.
Apart from that, I'll work in a separate PR on adding some tests for the "attachments kept in memory" situation and find a fix for the strange scenario I found.
Let me know if I missed something.
santib
force-pushed
the
activestorage-attach-consistency
branch
from
a4956b4
to
7acdfcb
Compare
|
Thank you, @santib! 😄 |
|
Thanks to you @jonathanhefner for your time and collaboration |
|
@santib Nice changes! @jonathanhefner In which release might this be included? |
|
@ron-shinall This was committed to |
Summary
After the code change done #45345 I realized that
ActiveStorage::Attached::Onehad a very similarattachmethod toActiveStorage::Attached::Many#attach. Given that it's not just a refactor and that the return value was changed for some branches, it'd be good to have them be consistent.cc @jonathanhefner
question: do we want to add tests for the return values for all the branches? I guess it'd help "documenting" how the method behaves