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

Pass the factory module and factory name to the options in custom strategies #111

Closed
paulcsmith opened this issue Mar 26, 2016 · 11 comments
Closed
Milestone

Comments

@paulcsmith
Copy link
Contributor

That way people can do things with the module. For example, you could create an Ecto strategy that let's you use a changeset like in #78

Also convert keyword lists to map so they part of the map can be matched. Keyword lists require a full match

defmodule ExMachina.EctoWithChangesetStrategy do
  use ExMachina.Strategy, function_name: :insert

  def handle_insert(record, %{repo: repo, factory_module: module, factory_name: factory_name}) do
    changeset_function_name = to_string(factory_name) <> "_changeset" |> String.to_atom
    if Code.ensure_loaded?(module) && function_exported(module, changeset_function_name, 1) do
      apply(module, changset_function_name, record) |> repo.insert!
    else
      repo.insert! record
    end
  end
end
@danhper
Copy link

danhper commented Mar 27, 2016

@paulcsmith This seems nice, thank you for working on it! 😄
Is there any easy way to replace ExMachina.EctoStrategy by the EctoWithChangesetStrategy here?

@paulcsmith
Copy link
Contributor Author

@tuvistavie If I understood correctly, you are asking how to use the EctoWithChangesetStrategy in your factory?

If so, here's how to do that. Instead of use ExMachina.Ecto you would do something like

defmodule Factory do
  use ExMachina
  use MyApp.EctoWithChangesetStrategy, repo: MyRepo
end

Though the strategy in this issue won't work until I also pass the factory module and name. Right now it just passes the options given to use MyStrategy, and they are a keyword list

@paulcsmith paulcsmith changed the title Pass the factory module to the options in custom strategies Pass the factory module and factory name to the options in custom strategies Mar 28, 2016
@danhper
Copy link

danhper commented Mar 31, 2016

@paulcsmith
Sorry if I missed something here but what is the behavior when a function is defined in two different strategies?
As far as I can tell, insert is already defined by EctoStrategy, will it be overriden if it is defined in a strategy used after?

@paulcsmith
Copy link
Contributor Author

@tuvistavie Good catch! I accidentally typoed. I corrected the snippet above from use Exmachina.Ecto to use ExMachina. That way the insert functions do not conflict :)

@paulcsmith
Copy link
Contributor Author

So there is a small problem with this that I came across as I was implementing it. The factory module can be passed without issue, but the factory name can't be passed when using the pipe operator. Here's an example

 # Can pass the factory name to the strategy because we have it as the first 
insert(:user)
# Can't pass factory name because it was given to build/2
build(:user) |> do_something_to_it |> insert

Here are some potential solutions, none of which I'm super excited about:

# Make it so that you can pass options to the pipeable version of the function. 
# In this case, you would pass the factory name
build(:user) |> do_something |> insert(:user)

^ would work, but requires retyping the factory name :S

# build_pipeable puts a key __ex_machina_factory_name__
# If __ex_machina_factory_name__ is present it will be used as the factory name when piping to the custom function
build_pipeable(:user) |> do_something |> insert 

^ would also work, but then we introduce a new concept and other potential ways that things could break by adding a key to maps/structs. It could be removed after piping to the custom function, but that seems strange to me.

Any thoughts on this?

cc @jsteiner @BlakeWilliams

@paulcsmith
Copy link
Contributor Author

After chatting at lunch a bit I think a better solution is none of the above and instead to rely on the struct name. We can add ExMachina.factory_name_from_struct/1 that would return a factory name for the struct

ExMachina.factory_name_from_struct(%MyApp.UserSubscription}) # Returns :user_subscription

That way you can use this in strategies for things like callbacks. The downside to this is that it's a bit less explicit, but it might actually be better in a lot of cases. Here's an example:

  def user_factory do
    %User{name: "me", admin: false}
  end

  def admin_factory do
    build(:user, admin: true)
  end

  def before_insert_user(user) do
    do_something_to(user)
  end

Since the factory name is extracted from the struct, the before_insert_user would be called for both the admin and user factory. So it is a bit less explicit, but I think the advantages of not adding new concepts or complicating the pipe functions makes it the best solution

@paulcsmith paulcsmith added this to the Release 1.0 milestone Apr 12, 2016
paulcsmith added a commit that referenced this issue Apr 12, 2016
Options passed to handling functions are now a map which makes it easier
to pattern match. It also includes the calling factory module in the
options.

name_from_struct/1 is added so that people can create strategies that
define callbacks or need to call other functions with the factory name.

Closes #111. See discussion for more details on why this strategy was
chosen.
paulcsmith added a commit that referenced this issue Apr 15, 2016
Options passed to handling functions are now a map which makes it easier
to pattern match. It also includes the calling factory module in the
options.

name_from_struct/1 is added so that people can create strategies that
define callbacks or need to call other functions with the factory name.

Closes #111. See discussion for more details on why this strategy was
chosen.
@paulcsmith
Copy link
Contributor Author

@tuvistavie Do you mind giving master a try with a custom Ecto strategy like in the description? I believe it would look something like:

defmodule ExMachina.EctoWithChangesetStrategy do
  use ExMachina.Strategy, function_name: :insert

  def handle_insert(record, %{repo: repo, factory_module: module}) do
    factory_name = ExMachina.Strategy.name_from_struct(record)
    changeset_function_name = to_string(factory_name) <> "_changeset" |> String.to_atom
    if Code.ensure_loaded?(module) && function_exported(module, changeset_function_name, 1) do
      apply(module, changset_function_name, record) |> repo.insert!
    else
      repo.insert! record
    end
  end
end

If you don't have time, LMK and I can try this out in a few weeks

@danhper
Copy link

danhper commented Apr 15, 2016

@paulcsmith Sure, I am going to give this a try, thank you very much for the great work!

@danhper
Copy link

danhper commented Apr 17, 2016

I just tried it out.
I was using a forked version, so to reuse my old implementation, I went with the following code:

defmodule ExMachina.EctoWithChangesetStrategy do
  use ExMachina.Strategy, function_name: :insert

  def handle_insert(record, %{repo: repo, factory_module: module}) do
    record
    |> module.make_changeset
    |> repo.insert!
  end
end

In my factory, the make_changeset looks like this:

  def make_changeset(%AdminUser{} = user) do
    {password, user} = Map.pop(user, :password)
    AdminUser.changeset(user, %{password: password})
  end
  def make_changeset(record) do
    Ecto.Changeset.change(record)
  end

It is working perfectly for my use case, thank you very much 😃

@danhper
Copy link

danhper commented Apr 17, 2016

Oh, I forgot, the only drawback is that the params_for is part of ExMachina.Ecto, which is coupled with ExMachina.EctoStrategy, so I had to copy paste the function into my factory. It's simply a matter of calling ExMachina.Ecto, so it's not that much of a big deal, but maybe it could be improved.

@paulcsmith
Copy link
Contributor Author

Very cool. I'm glad it worked. Yeah that is a drawback of using your own strategy for using ExMachina. I think for ExMachina 1.0 I will include a before_insert_ option for ExMachina.EctoStrategy. I've run in to a couple instances where it would be nice :) I believe that would solve those issues

matiasgarciaisaia added a commit to instedd/surveda that referenced this issue Dec 9, 2019
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

No branches or pull requests

2 participants