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

The use of classify is not documented which produces unexpected singularization of class names in organizers #167

Closed
majksner opened this issue Feb 7, 2020 · 10 comments
Labels
bug Something isn't working verified This issue has been verified. wontfix This will not be worked on
Milestone

Comments

@majksner
Copy link

majksner commented Feb 7, 2020

Question

If I want to have multiple organizers but I want them in separate modules, let's say I have an organizer in module Order (app/interactors/order)

app/interactors/order
├── generate_invoice.rb
├── place.rb
├── prepare_data.rb
├── redeem_coupon.rb
└── send_order_email.rb
module Order
  class Place < ApplicationOrganizer
    organize do
      add Order::PrepareData
      add Order::GenerateInvoice
      add Order::RedeemCoupon
      add Order::SendOrderEmail
    end
  end
end

This kinda works, but for an unknown reason, some interactors are skipped and I'm not sure why (in my case Order::PrepareData)

[1] pry(main)> Order::Place.organize
=> #<ActiveInteractor::Organizer::InteractorInterfaceCollection:0x00007fbc280220d8
 @collection=
  [#<ActiveInteractor::Organizer::InteractorInterface:0x00007fbc28030c50 @filters={}, @interactor_class=Order::GenerateInvoice, @perform_options={}>,
   #<ActiveInteractor::Organizer::InteractorInterface:0x00007fbc28030020 @filters={}, @interactor_class=Order::RedeemCoupon, @perform_options={}>,
   #<ActiveInteractor::Organizer::InteractorInterface:0x00007fbc26cc3398 @filters={}, @interactor_class=Order::SendOrderEmail, @perform_options={}>]>

If I rename class to PrepareStuff it works fine.

P.S. I'm new to Ruby, so I might be doing something wrong.

@majksner majksner added the question A question label Feb 7, 2020
@aaronmallen
Copy link
Owner

This is possibly a bug, let me investigate this evening.

@aaronmallen aaronmallen added bug Something isn't working unverified This issue has not been verified labels Feb 7, 2020
@aaronmallen
Copy link
Owner

@majksner can you add the contents of the interactors themselves?

@aaronmallen aaronmallen added P4 verified This issue has been verified. and removed question A question unverified This issue has not been verified labels Feb 7, 2020
@aaronmallen aaronmallen added this to the v1.0.3 milestone Feb 7, 2020
@aaronmallen aaronmallen changed the title [Question] Organizing interactors in modules Organizing interactors in modules skips first interactor Feb 7, 2020
@aaronmallen
Copy link
Owner

🤔 I've done a bit of testing and it appears to be an issue with the class name Order::PrepareData specifically.

@majksner
Copy link
Author

majksner commented Feb 7, 2020

I've also noticed if the class is called CleanupFiles same issue. It could be because I'm not using symbols for class names, but I can't since I'm relying on modules.

While I'm here. I've noticed another possible issue. If some of the interactors of the organizer fail context.fail!('Error Message') error message is not persisted.

[10] pry(main)> stripe = StripeCustomer::UpdateCustomer.perform(user: user, params: params)
stripe.success?
=> false

[10] pry(main)> stripe.errors
=> #<ActiveModel::Errors:0x00007fa730422528
 @base=
  #<StripeCustomer::UpdateCustomer::Context user=#<User id: 104..>, params={...}>,
 @details={},
 @messages={}>

Thank you for looking into this.

@aaronmallen
Copy link
Owner

This appears to be an issue with constantize in ActiveSupport:

irb(main):005:0> "PrepareData".classify
=> "PrepareDatum"

@aaronmallen aaronmallen added docs and removed P4 labels Feb 7, 2020
@majksner
Copy link
Author

majksner commented Feb 7, 2020

I see it singularizes string.

[2] pry(main)> "PrepareFiles".classify
=> "PrepareFile"
[3] pry(main)> "PrepareThings".classify
=> "PrepareThing"
[4] pry(main)> "Generates".classify
=> "Generate"

Maybe I can send you a pull request for documentation to mention this? Aside from this, putting organizers into modules that I mentioned at the beginning of this issue is a way to go or you have a better suggestion?

@aaronmallen
Copy link
Owner

The issue isn't the nested modules, it's the class names themselves. classifywill return PlaceDataum or Order::PlaceDatum.

@aaronmallen
Copy link
Owner

I've also noticed if the class is called CleanupFiles same issue. It could be because I'm not using symbols for class names, but I can't since I'm relying on modules.

While I'm here. I've noticed another possible issue. If some of the interactors of the organizer fail context.fail!('Error Message') error message is not persisted.

[10] pry(main)> stripe = StripeCustomer::UpdateCustomer.perform(user: user, params: params)
stripe.success?
=> false

[10] pry(main)> stripe.errors
=> #<ActiveModel::Errors:0x00007fa730422528
 @base=
  #<StripeCustomer::UpdateCustomer::Context user=#<User id: 104..>, params={...}>,
 @details={},
 @messages={}>

Thank you for looking into this.

@majksner could you file a separate bug for this?

@aaronmallen aaronmallen changed the title Organizing interactors in modules skips first interactor The use of classify is not documented which produces unexpected singularization of class names in organizers Feb 7, 2020
@aaronmallen
Copy link
Owner

I've also added a new bug, #168, for this... for Symbol or String the fix should be to document the use of classify, however If I pass a Class or Module directly I would expect that to be found correctly.

@aaronmallen
Copy link
Owner

Instead of a documentation change we will use camelize instead of classify to safe_constantize class names. If I have a class defined like PrepareData and I pass an argument to the organize method like :prepare_data, "PrepareData", or PrepareData I should reasonably expect this argument would not be singularized down the chain. Closing this bug as won't fix.

see #168 and #171

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working verified This issue has been verified. wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants