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 support for before build callback #1639

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mohammednasser-32
Copy link

@mohammednasser-32 mohammednasser-32 commented Apr 28, 2024

Fixes #1633

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

Tested the changes by adding the local gem to a project
image
image

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

@mike-burns
Copy link
Contributor

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
Copy link
Author

mohammednasser-32 commented Apr 29, 2024

@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
Copy link
Contributor

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
Copy link
Author

@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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

Thank you, much appreciated 🙏

@mohammednasser-32
Copy link
Author

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 sarahraqueld requested review from smaboshe and sarahraqueld and removed request for mike-burns August 30, 2024 09:06
@sarahraqueld
Copy link
Contributor

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
Copy link
Author

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

@sarahraqueld
Copy link
Contributor

@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?

change the approach for calling the before_build_callback, now it is called before the object is built, and thus called without any attributes
@mohammednasser-32 mohammednasser-32 force-pushed the add_before_build_callback branch from 13a5f67 to 6a63eea Compare August 30, 2024 19:45
@mohammednasser-32 mohammednasser-32 force-pushed the add_before_build_callback branch from 86c4370 to 35db09f Compare August 30, 2024 20:28
@mohammednasser-32
Copy link
Author

@sarahraqueld Done

@mohammednasser-32
Copy link
Author

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

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.

Support for callback before(:build)
4 participants