Documentation for using a block when creating multiple records misleading

[jackrg]

Description

The documentation that covers passing a block to create_list and build_list has an example that only works for build_list.

Reproduction Steps

Look at the section on Creating Multiple Records. The example where there is a block uses attribute assignment. That is fine if you're using build_list, but if you're using create_list that does not work. With create_list, you'll have to use rec.update(...) or something similar, as it appears that the record has already been persisted.

Expected behavior

I expected the example to work for both build_list and create_list

Actual behavior

It only works for build_list.

System configuration

factory_bot version: 5.2.0
rails version: 5.1.7
ruby version: 2.4.10

[entcheva]

Hi @jackrg - do you mind using our reproduction script to share an executable example where using create_list is failing? I've tried a few examples and have been unable to reproduce the issue you're describing. Thanks!

[jackrg]

Hi Christina, Attached is the script modified to reproduce the issue. I think you missed the point, if that was your attempt to reproduce it. The key point is the use of a block with the call. To make it work, I added another table (“replies”) that has a one-to-many relationship with posts. That way, it makes sense to create a list of “replies” to a “post”. The key is that I allow the factory to use the default value for body but then attempt to override it within the block. While that would work with #build_list, it does not work with #create_list, because the block is called after the record is persisted, and therefore the changes are not saved. I could not actually get this to run on my system, as it was giving me dependency issues with the gem file you provided, but I’m sure it will build and run on your system just fine, if the original file builds and runs. Your documentation gave this example: twenty_somethings = build_list(:user, 10) do |user, i| user.date_of_birth = (20 + i).years.ago end And that example works fine, as long as you eventually save the records. But it is implied that you could just change #build_list to #create_list and it would work, but it doesn’t. Please let me know if you have any further questions. Jack

…

On Mar 22, 2021, at 3:15 PM, Christina Entcheva ***@***.***> wrote: Hi @jackrg <https://github.com/jackrg> - do you mind using our reproduction script <https://github.com/thoughtbot/factory_bot/blob/master/.github/REPRODUCTION_SCRIPT.rb> to share an executable example where using create_list is failing? I've tried a few examples and have been unable to reproduce the issue you're describing. Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1479 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHK5AJEOVF7BDYKJZGUQFTTE66RRANCNFSM4YWAOE7A>.

[composerinteralia]

I think this is a duplicate of #1444 (comment). There's definitely still room to improve the documentation.

I think you missed the point, if that was your attempt to reproduce it.

I hope you will consider that this sentence comes across to me as fairly rude. I don't think Christina missed the point—the reproduction script shared in that comment is the standard reproduction script shared in our bug report template. It's generally easier for us to figure out what's going on if we have something we can run.

Thank you!

[jackrg]

I apologize if that statement came across as rude; that was certainly not my intent. I just wasn’t sure whether the template provided was a standardized template, an attempt to reproduce the problem, or something somewhere in between. Based on your statement, Daniel, I obviously misinterpreted the significance of the script. For a standardized template, it was quite close to what I was referring to. I understand your point about having a reproducible example; I didn’t include one in the first place because I thought that the point was clear; obviously I was mistaken about that. Given that the documentation at that point is talking about both build_list and create_list (see the section title), if the one example does not work for both calls, it would probably be advisable to give a second example for #create_list. Where I went wrong is that in seeing that example, I assumed that the block would be called prior to persisting the record (similar to traits and such within q Factory definition). Anyway, I would characterize this as more of a desirable tweak to the documentation than a bull-blown documentation error. Thanks for your time. Jack

…

On Mar 22, 2021, at 5:19 PM, Daniel Colson ***@***.***> wrote: I think this is a duplicate of #1444 (comment) <#1444 (comment)>. There's definitely still room to improve the documentation. I think you missed the point, if that was your attempt to reproduce it. I hope you will consider that this sentence comes across to me as fairly rude. I don't think Christina missed the point—the reproduction script shared in that comment is the standard reproduction script shared in our bug report template. It's generally easier for us to figure out what's going on if we have something we can run. Thank you! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1479 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHK5AKMDMU72VGPT2GNYDTTE7M7TANCNFSM4YWAOE7A>.

[dan-drew]

IMHO this isn't a documentation issue, it's a bug. The expectation for create_list is that it returns a list of persisted objects, not a list of persisted objects with unpersisted changes. I'd also suggest the most useful reason to pass a block function is to make changes you either want or need to make before each item is persisted. It's easy enough to make changes after the list is returned without needing a hook.

FWIW I've tripped over this multiple times, far enough apart that I forget about this quirk. It's wasted a day or two each time trying to figure out why all the test objects look correct but DB queries are returning the wrong things. If you don't agree with it being a bug then adding a one-line note to the docs would be appreciated.

Cheers
Dan

[composerinteralia]

Closed by fc098ff

I agree there's a case to be made for changing the behavior here, but it may be difficult to do in a backwards compatible way. At least now we have documentation for the current behavior.

[jackrg]

Totally agree with your comment about the difficulty of changing the behavior in a backward-compatible way, and I appreciate the documentation tweak. Cheers!

…

On Jan 14, 2022, at 12:16 PM, Daniel Colson ***@***.***> wrote: Closed by fc098ff <fc098ff> I agree there's a case to be made for changing the behavior here, but it may be difficult to do in a backwards compatible way. At least now we have documentation for the current behavior. — Reply to this email directly, view it on GitHub <#1479 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHK5AORKD6SZFHYGQDRBJLUWCACNANCNFSM4YWAOE7A>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.