foodcoops / foodsoft

Web-based software to manage a non-profit food coop (product catalog, ordering, accounting, job scheduling).
https://foodcoops.net/
Other
322 stars 146 forks source link

Can we use the article filter logic for the shared supplier for 'normal' supplier articles as well? #897

Open JohannHansing opened 2 years ago

JohannHansing commented 2 years ago

For shared suppliers a nice filtering logic is implemented to filter article names

  def shared
    # build array of keywords, required for ransack _all suffix
    q = params.fetch(:q, {})
    q[:name_cont_all] = params.fetch(:name_cont_all_joined, '').split(' ')
    search = @supplier.shared_supplier.shared_articles.ransack(q)
    @articles = search.result.page(params[:page]).per(10)
    render :layout => false
  end

By contrast, the following very simple filter functionality is implemented to filter articles for a non-shared supplier

@articles = Article.undeleted.where(supplier_id: @supplier, :type => nil).includes(:article_category).order(sort)

Both lines of code can be found in articles_controller.rb

Would anything speak against putting the shared_supplier filter logic into a function and reusing that for Article.undeleted as well?

JuliusR commented 2 years ago

I like that idea.

Just tried it, and it seems to work. It definitely needs some more testing.

Any disadvantages/regressions we are missing?

JohannHansing commented 2 years ago

Any disadvantages/regressions we are missing?

is there a performance overhead when using ransack?

While we are at it, we could also reconsider the placement of the name filter input fields for imported articles vs external supplier database.

In the attached screen shot you can see that one can use both filtering fields simultaneously but from the placement it is not 100% clear which field is for which purpose. Could one potentially move the "Artikel importieren" box (with light gray background) above the row of buttons (the row with the blue "Suchen/ Importieren" Button?

Screen Shot 2021-11-09 at 16 20 12
JuliusR commented 2 years ago

is there a performance overhead when using ransack?

I do not know. I am not planning to put any more effort into pushing this issue. I just wondered if the change is simple, and then I wanted to share my experiment.

While we are at it, we could also reconsider the placement of the name filter input fields for imported articles vs external supplier database.

Yes, that is a good point. However, your suggestion of putting the import UI above the button row has the following disadvantage: After clicking on "Suchen/Importieren", the button row you just clicked will jump down.

What about moving the upper filter input into the table header (see finance/balancing/new)?

Screenshot 2021-11-09 at 22-04-15 Foodsoft - Accounting Beautiful bakery

If we decide to apply any of these two improvements (name_cont_all filtering and better placement of filter inputs) then I suggest to check all filters across the app for consistent behavior. It might be better to discuss these two improvements in separate issues, though.

JohannHansing commented 2 years ago

is there a performance overhead when using ransack?

I do not know. I am not planning to put any more effort into pushing this issue. I just wondered if the change is simple, and then I wanted to share my experiment.

To answer my own question: https://github.com/activerecord-hackery/ransack#search-matchers seems like ransack is simply running an sql query matching the search term(s) via like. So I don't imagine we would run into performance issues here.

If we decide to apply any of these two improvements (name_cont_all filtering and better placement of filter inputs) then I suggest to check all filters across the app for consistent behavior. It might be better to discuss these two improvements in separate issues, though.

I agree