search

avo-hq / avo

Build Ruby on Rails apps 10x faster
https://avohq.io
Other
1.47k stars 230 forks source link

dynamic_filters should pass NEW instead of INDEX view #3009

Closed pjmuller closed 1 month ago

pjmuller commented 1 month ago

Describe the bug

I'm using the view inside of the fields to put them on steroids 💪 , for example

# resource class
def fields
  if view.index?
    field :abc_select, as: :text, format_using: -> {}, .. # FYI did this because the format_using behaves better within type select
  else
    field :abc_select, as: :select, options: [...]
  end
end

However I notice that when you add a dynamic filter, you invoke Avo::DynamicFilters::FieldsController#create with params 

{
  "filter_param_id"=>"xxx",
  "resource_class"=>"Avo::Resources::YYYY",
  "turbo_frame"=>"avo_filters_holder_zzz"
}

you are passing the view as INDEX, could you do this as NEW as this makes more sense to me (we are showing the form element, not rendering the content of that field)

The current workaround I use is xxx but that doesn't feel stable in the long run

def fields
  view_2 = params[:filter_param_id].present? ? Avo::ViewInquirer.new("new") : view
  if view_2.index? # ....
  end
end

PS: regarding the abs_select resource example, this is just one example, I do DOZENS of other tweaks like these, so don't pinpoint on it -> as could also be fixed by improving format_using for select field type. Trust me that I need to know the right view.

System configuration

Avo version: 3.10.6

Rails version: 7.2.0.beta3

Ruby version: 3.2.2

Are you using Avo monkey patches, overriding views or view components?

Additional context

Impact

Urgency

Paul-Bob commented 1 month ago

Hei @pjmuller

I'm using the view inside of the fields to put them on steroids 💪 , for example

That was the idea when we first implemented def fields, unfortunately, it proved to be problematic to use if statements inside it. More context and information here https://docs.avohq.io/3.0/best-practices.html#avoid-using-if-else-statements-in-def-fields

you are passing the view as INDEX, could you do this as NEW as this makes more sense to me (we are showing the form element, not rendering the content of that field)

The filters are always applied on index. Can you please explain better the use case? How are you applying a filter on the new view?

Trust me that I need to know the right view.

Makes sense, we also want the view to be coherent!

pjmuller commented 1 month ago

Hi @Paul-Bob ,

thanks for getting back to me so quickly. I'll answer bullet point style

NameError (undefined local variable or method `options' for #<Avo::Fields::TextField:0x0000fffeedb2d710 @id=:gender,
1: <%= render Avo::DynamicFilters::FilterComponent.new filter: filter, render_open: render_open %>
avo (3.10.6) lib/avo/fields/base_field.rb:267:in `options_for_filter'
avo-dynamic_filters (3.10.6) app/components/avo/dynamic_filters/filter_component.html.erb:55:in `block (2 levels)
avo-dynamic_filters (3.10.6) app/components/avo/dynamic_filters/filter_component.html.erb:31:in `block in call'
avo-dynamic_filters (3.10.6) app/components/avo/dynamic_filters/filter_component.html.erb:1:in `call'
avo-dynamic_filters (3.10.6) app/views/avo/dynamic_filters/_select_filter.html.erb:1
avo-dynamic_filters (3.10.6) app/components/avo/dynamic_filters/filters_component.html.erb:52:in `block (3 levels) in call'

Some solution ideas

def filters dynamic_filter :abc_select, ... end

Paul-Bob commented 1 month ago

thanks for getting back to me so quickly. I'll answer bullet point style

Anytime! My preferred style, is easy to respond and focus on the several aborded topics

  • Adrian already trained me well 👨‍🎓 that I should use the least amount of if statements as possible in the fields, however here (to him) it made sense as well as we keep the same field IDs, I'm mainly switching field_types

You're right since the IDs are the same should be fine.

Thanks for the video, it makes more sense now! But forcing the view to new when fetching fields from inside filters is not ideal since the view should be coherent with the current index state. It might make sense for this particular case but we should (and will) find a better alternative.

Thank you for all your suggestions and possible solutions!

Some solution ideas

  • let me overwrite how you render the "we are filtering on this value" so that you don't dig into the "options" field to humanize the enum yourself

I sustain a more stable fix which should be available for everyone, let's apply this only on extreme edge-cases

  • create a filter that is NOT directly tied to a resource field. Though I don't think you are passing the full DSL like options 
    def filters
dynamic_filter :whatever_field, type: "select", options: [], label: , conditions: {}, query: -> {}
end

I think this is the most fittable approach, we already have 2 issues tracking it https://github.com/avo-hq/avo/issues/3026 and https://github.com/avo-hq/avo/issues/2939.

  • define my select field twice, though so far I haven't needed it, so feels like plan C 
    
def fields
field :abc_select_render, as: :text, format_using: -> {}, hide_on: [:form]
field :abc_select, as: :select, options: [...], hide_on: [:index, :show]
end

def filters dynamic_filter :abc_select, ... end



Agree, this is plan C, we should implement the above solution with the `options` filter option and try it out.
adrianthedev commented 1 month ago

I agree with @Paul-Bob to not force the view to New instead of Index. We are opening up a different can of worms.

The C scenario seems to be the actual soution. the dynamic_filter should know which field type you're refering to and use that one

Paul-Bob commented 1 month ago

I think this is the most fittable approach, we already have 2 issues tracking it https://github.com/avo-hq/avo/issues/3026 and https://github.com/avo-hq/avo/issues/2939.

@Paul-Bob Jul 19, 2024

@pjmuller options are available since 3.10.10 on select filters (docs here, scroll max down)

Now this DSL is available:

def filters
  dynamic_filter :whatever_field, type: :select, options: ["Label 1", "Label 2"]
end

Or

def filters
  dynamic_filter :whatever_field, type: :select, options: { "Label 1" => :value_1, "Label 2" => :value_2 }
end

Or

def filters
  dynamic_filter :whatever_field, type: :select, options: { value_1: "Label 1", value_2: "Label 2" }.invert
end

I'm closing this one as completed, let me know if it's working as expected.