Add configurable methods for Rentals#index - #81
Conversation
| private | ||
|
|
||
| def not_connected_rentals | ||
| BookingsyncPortal.not_connected_rentals || Proc.new { |
There was a problem hiding this comment.
why proc? I think lambda accepting current_account as argument will be cleaner
| context "methods from default config" do | ||
| render_views | ||
|
|
||
| it 'synchronizes rentals' do |
| end | ||
| end | ||
|
|
||
| it "should call custom 'not_connected_rentals' method" do |
There was a problem hiding this comment.
show some confidence 😉 it "calls ..."
| context "using custom methods" do | ||
| before do | ||
| BookingsyncPortal.setup do |config| | ||
| config.not_connected_rentals = Proc.new { "not_connected_rentals" } |
There was a problem hiding this comment.
I'd rather do some real setup with custom action, and render views
| @remote_accounts = current_account.remote_accounts | ||
| @remote_rentals_by_account = current_account.remote_rentals.ordered | ||
| .includes(:remote_account, :rental).group_by(&:remote_account) | ||
| @not_connected_rentals = not_connected_rentals.call |
There was a problem hiding this comment.
Why not making this entire action an operation itself? This is hardly extendable, what if you want to modify the views and have some extra ivar? you would need to override the method which really defeats the purpose of these changes.
Not sure about the API though, maybe it should be something like:
# use registry so that the operation is overridable easily
BookingsyncPortal::OperationsRegistry.fetch("Admin::RentalsController#index").call(controller)
and that method would probably need to handle the rendering, like:
controller.render :index, locals: { visible_rentals: visible_rentals, **other_depedencies }
So that might require removing usage of ivars at all and just "normal" injected variables.
This is just an idea, but if we already make some changes here, it should be robust.
| private | ||
|
|
||
| def not_connected_rentals | ||
| BookingsyncPortal.not_connected_rentals || lambda { |
There was a problem hiding this comment.
Hmm we decided to override full action, therefore customizations for each ivar are not needed. We want to overridde whole action
| end | ||
|
|
||
| def index_arguments | ||
| BookingsyncPortal.custom_arguments.dig("Admin", "RentalsController", "index") || { |
There was a problem hiding this comment.
this looks quite static, whole operation should be customizable
There was a problem hiding this comment.
if you want to customize all operation, wouldn't be better to override the action?
There was a problem hiding this comment.
It would be but it's not easy to override just one action from controller, and even more painful to test it, so the point of this operation is to give a mean to do that without all the overhead
3 participants
Closes #80