Add support for before build callback

[mohammednasser-32]

Fixes #1633

add support for before(:build) callback as requested in the issue

Tested the changes by adding the local gem to a project

I am honestly not sure how to add unit tests for the change, any help would be appreciated

[mike-burns]

Probably the easiest thing to do is to add a test to spec/acceptance/callbacks_spec.rb. You'll want to make sure that before(:build) is called before anything is built.

Spitballing here, something like:

class TitleSetter
  def self.title=(new_title)
    @@title = new_title
  end

  def self.title
    @@title
  end
end

define_model("Article", title: :string)

FactoryBot.define do
  factory :article_with_before_callbacks, class: :article do
    before(:build) { TitleSetter.title = "title from before build" }
    after(:build) { TitleSetter.title = "title from after build" }

    title { TitleSetter.title }
  end
end

it "runs the before callback" do
  article = FactoryBot.build(:article_with_before_callbacks)
  expect(article.title).to eq("title from before build")
end

[mohammednasser-32]

@mike-burns Thank you for your suggestion 🙏
but won't title from after build in the after(:build) callback always overwrite the title from before build? why is this scenario expected to pass?

also would something like this do it?

define_model("User", full_name: :string)

FactoryBot.define do
  factory :user_with_before_callbacks, class: :user do
    before(:build) { user.full_name = "first }
    after(:build) { user.full_name = "#{user.full_name} last" }
  end
end

it "runs the before callback" do
  article = FactoryBot.build(:user_with_before_callbacks)
  expect(user.full_name).to eq("first last")
end

[mike-burns]

We need to make sure before(:build) is called before any building happens.

My theory on why my suggestion works is this timeline:

1. FactoryBot.build(:article)
2. before(:build) { TitleSetter.title = "title from before build" }
3. TitleSetter.title = "title from before build"
4. now build happens
5. internal: resource = Article.new
6. internal: resource.title = (-> { TitleSetter.title }).call
7. TitleSetter.title # => "title from before build"
8. internal: done building, returns resource
9. after(:build) { TitleSetter.title = "title from after build" }
10. TitleSetter.title = "title from after build"
11. resource.title has not changed since step (6), because we already called #call.

(For what it's worth, I'm not sure your current code calls the callback before the object is built.)

[mohammednasser-32]

@mike-burns you are right the approach was not correct and it was actually called after the build, thanks for raising the hand!
I changed the approach here, now it is being called before the object is built and your test passes.

[mohammednasser-32]

Hey @mike-burns , sorry to ping you again 😅 I am just wondering if the new approach is okay or do I need to investigate other options? Thank you

[mike-burns]

Oh hey, this approach is good, thanks for taking it on.

I need to find time to do a final review and merge it. I can do small tweaks from here and give you credit.

[mohammednasser-32]

Thank you, much appreciated 🙏

[mohammednasser-32]

Hey @sarahraqueld , sorry for pinging..I am just keen to have some contributions in the gem (if possible) 😅 so I would appreciate it if someone can take a look on this PR and tell me if there is any change I need to do. (I also had another PR here 🙈)
Thank you so much.

[sarahraqueld]

Hey @sarahraqueld , sorry for pinging..I am just keen to have some contributions in the gem (if possible) 😅 so I would appreciate it if someone can take a look on this PR and tell me if there is any change I need to do. (I also had another PR here 🙈) Thank you so much.

Hi @mohammednasser-32, me and @smaboshe are trying our best to deal with all the issues and PRs we inherited when we became maintainers. We thank you for your patience, and will review this today.

[mohammednasser-32]

@sarahraqueld @smaboshe Thank you so much, much appreciated and sorry for the noise 🙏

[sarahraqueld]

@mohammednasser-32 We see that the CI seems to be stuck. Can you rebase your branch and push to see if that triggers the tests to run correctly this time?

[mohammednasser-32]

@sarahraqueld Done

[mohammednasser-32]

Hey @smaboshe, since this already got approved I was just wondering if there is plan for it to get merged soon? Thank you 🙏