Filters #890

MoshiKoi · 2022-09-13T11:53:47Z

Proof of concept, looking for feedback

cellio · 2022-09-13T15:09:01Z

Addresses #755 .

app/controllers/users_controller.rb

app/models/filter.rb

Co-authored-by: ArtOfCode <ArtOfCode-@users.noreply.github.com>

cellio · 2022-11-18T14:36:59Z

@ArtOfCode- , Moshi said in chat that this is ready for an initial code review. I see there are merge conflicts which obviously have to be resolved, but can someone can take a look at the code for any other issues in the meantime?

Taeir

I think this is some quite great work @MoshiKoi ! There are quite a few annoying/tricky cases in the search (and other places) that you dealt with in a pretty nice way :)

I do have quite a few review comments, some of those questions about the workings, others small improvements or suggestions. For a final version, I also think we should write some tests, to verify that basic filters work as intended.

I have not yet checked your JavaScript code, I kept to the Ruby and HTML for now. I will take a look at that at a later time if I get to it.

Taeir · 2022-11-24T13:26:08Z

db/migrate/20220916075849_add_tags_to_filters.rb

@@ -0,0 +1,6 @@
+class AddTagsToFilters < ActiveRecord::Migration[7.0]
+  def change
+    add_column :filters, :include_tags, :string


As a string column these would have a max length of 255 characters, which may not be sufficient. We could change it to text or switch how include_tags and exclude_tags are stored from array to something else.

This is the same way that tags are stored on posts and it seems to be doing okay - I don't imagine someone is going to be filtering by more tags than can fit on a post, but I could be wrong.

qpixel/db/schema.rb

Line 411 in ad595c3

t.string "tags_cache"

Taeir · 2022-11-24T13:28:21Z

db/migrate/20220914193044_diallow_filter_user_or_name_null.rb

@@ -0,0 +1,6 @@
+class DiallowFilterUserOrNameNull < ActiveRecord::Migration[7.0]
+  def change
+    change_column_null :filters, :user_id, false


The first three migrations can be merged into one to directly create the right table setup/structure. Depending on the underlying database, this could be slightly beneficial.

I don't actually know how to do this, so I'll leave it to you (or someone else).

config/config/preferences.yml

db/seeds/filters.yml

test/fixtures/category_filter_defaults.yml

Taeir · 2022-11-24T15:01:27Z

app/controllers/users_controller.rb

+      render json: { status: 'success', success: true, name: default_filter&.name },
+             status: 200
+    else
+      render json: { status: 'failed', success: false },


Should there be an error message here?

app/views/search/_filters.html.erb

Taeir · 2022-11-24T15:03:43Z

app/views/search/_filters.html.erb

+      <%= submit_tag 'Apply', class: 'button is-medium is-outlined', name: nil %>
+    <% end %>
+    <% if user_signed_in? %>
+      <button type="button" class="filter-save button is-medium is-filled">Save</button>


Should we display the button but grayed out (disabled) and with a title set to indicate "this is not possible because you are not signed in"?

Not sure - I personally prefer it the way it is now, but I'm open to changing it

What is the current behavior? (She says, hoping to avoid having to pull and set it up locally to find out. :-) )

It's just Not There™️

app/models/filter.rb

test/fixtures/filters.yml

app/assets/javascripts/filters.js

app/assets/javascripts/qpixel_api.js

ArtOfCode- · 2022-11-24T16:08:55Z

app/controllers/users_controller.rb

+    if user_signed_in? && params[:name]
+      filter = Filter.find_or_create_by(user: current_user, name: params[:name])
+
+      filter.update(min_score: params[:min_score], max_score: params[:max_score],


No obvious security risks that I can see here, but the validation concerns are... valid (ha!). Worth considering strong params here, i.e. filter.update(permitted_params), where permitted_params is a method that calls params.require(...).permit(...) etc.

ArtOfCode- · 2022-11-24T16:10:38Z

app/controllers/users_controller.rb

+                    include_tags: params[:include_tags], exclude_tags: params[:exclude_tags],
+                    status: params[:status])
+
+      unless params[:category].nil? || params[:is_default].nil?


This one might be more down to personal preference? I like an unless statement in Ruby - I find they make sense if you read them in natural language to yourself.

"Unless params[:category] is nil or params[:is_default] is nil"

vs

"If params[:category] is not nil and params[:is_default] is not nil"

ArtOfCode- · 2022-11-24T16:12:55Z

app/helpers/search_helper.rb

+    qualifiers = []
+    qualifiers.append({ param: :score, operator: '>=', value: filter.min_score }) unless filter.min_score.nil?
+    qualifiers.append({ param: :score, operator: '<=', value: filter.max_score }) unless filter.max_score.nil?
+    qualifiers.append({ param: :answers, operator: '>=', value: filter.min_answers }) unless filter.min_answers.nil?
+    qualifiers.append({ param: :answers, operator: '<=', value: filter.max_answers }) unless filter.max_answers.nil?
+    qualifiers.append({ param: :include_tags, tag_ids: filter.include_tags }) unless filter.include_tags.nil?
+    qualifiers.append({ param: :exclude_tags, tag_ids: filter.exclude_tags }) unless filter.exclude_tags.nil?
+    qualifiers.append({ param: :status, value: filter.status }) unless filter.status.nil?
+    qualifiers


consider (quality): is it better to iterate through the properties of filter and add to qualifiers for each that exists, rather than repeated append/unless statements?

I'm not really sure how that would look to be honest; I'm not familiar enough with idiomatic Ruby patterns.

Since the treatment of each property is different, I see no good way to do what you propose.

Co-authored-by: Taico Aerts <devtaeir@taico.nl>

test/fixtures/filters.yml

app/models/user.rb

Taeir · 2022-12-09T10:13:09Z

app/models/user.rb

@@ -24,6 +24,7 @@ class User < ApplicationRecord
  has_many :comments, dependent: :nullify
  has_many :comment_threads_deleted, class_name: 'CommentThread', foreign_key: :deleted_by_id, dependent: :nullify
  has_many :comment_threads_locked, class_name: 'CommentThread', foreign_key: :locked_by_id, dependent: :nullify
+  has_many :filters, dependent: :destroy
  belongs_to :deleted_by, required: false, class_name: 'User'


On another note, the question is why category_filter_defaults is linked to user, since it is also linked to filter and filter is linked to a user.

The problem is the System filters. We could have, for instance

Filter(Positive, belongs to System)
CategoryFilterDefault(Moshi, Meta, Positive)

To be honest, attaching "universal" filters to System might not have been the brightest idea, since it leads to weird edge cases like this.

Co-authored-by: Taico Aerts <devtaeir@taico.nl>

MoshiKoi · 2022-12-20T23:00:16Z

This test no longer succeeds due to a change in how search works:

qpixel/test/controllers/search_controller_test.rb

Lines 6 to 10 in 8f36e90

    
           test 'get without a search term should result in nil' do 
        
             get :search 
        
             assert_response 200 
        
             assert_nil assigns(:posts) 
        
           end

Now, an empty search will return all posts. Should this method be changed to instead assert not nil?

Co-authored-by: Taico Aerts <devtaeir@taico.nl>

MoshiKoi · 2022-12-21T23:55:35Z

Since it was easy, I just added in #905 as well.

MoshiKoi · 2023-01-25T00:32:35Z

Work is continuing on #976

MoshiKoi added 17 commits September 13, 2022 13:28

Create views for filters

f6dd613

Refactor search and make filter functional

ceade96

Autofill filter fields

03a8de2

Add filter widget to sidebar

9364b7e

Add form to filter widget

d4e0e37

Make filters work with categories

7d2bcc4

Move apply button to bottom

cbc5bf7

Make predefined filters work

084ed3e

Fix duplicate id issue

e319446

Unify filter and search qualifiers

928d880

Add answer count filter

0b8bfc5

Formatting

5aa9ef7

Add preset filter for unanswered questions

0bcfe82

Add reset button for filters

34928a5

Enforce category permissions

8da46bd

Add search and filter by post status

f1d6947

Update presets

3d3e23a

MoshiKoi added 9 commits September 13, 2022 08:54

Only show filter widget on category feed page

624454c

Add filters to database

02b821b

Add routes to get/set filters

959f442

Convert response to a nicer client format

b26d191

Fetch user filters from server

e648872

Seed filters

6ea26f5

Add system filters to response

4bf0872

Refactor json conversion

91be2e2

Give unsigned users the system filters

9f88a0f

ArtOfCode- reviewed Sep 14, 2022

View reviewed changes

app/controllers/users_controller.rb Outdated Show resolved Hide resolved

app/models/filter.rb Outdated Show resolved Hide resolved

MoshiKoi and others added 2 commits September 14, 2022 10:00

Update app/controllers/users_controller.rb

6a5d75a

Co-authored-by: ArtOfCode <ArtOfCode-@users.noreply.github.com>

Remove unnecessary code

a4ff51d

Taeir reviewed Nov 24, 2022

View reviewed changes

ArtOfCode- reviewed Nov 24, 2022

View reviewed changes

MoshiKoi and others added 6 commits December 8, 2022 17:39

Update preference config schema comment

bf4fbcc

Change var to let

8e9b343

Consistantly use CamelCased variables

50cd4c8

Update test/fixtures/category_filter_defaults.yml

d143acb

Co-authored-by: Taico Aerts <devtaeir@taico.nl>

Update config/config/preferences.yml

6691585

Co-authored-by: Taico Aerts <devtaeir@taico.nl>

Set priority of Users and Filters

8c5e7e9

Taeir reviewed Dec 9, 2022

View reviewed changes

test/fixtures/filters.yml Show resolved Hide resolved

Taeir reviewed Dec 9, 2022

View reviewed changes

test/fixtures/filters.yml Outdated Show resolved Hide resolved

Taeir reviewed Dec 9, 2022

View reviewed changes

app/models/user.rb Show resolved Hide resolved

Taeir reviewed Dec 9, 2022

View reviewed changes

MoshiKoi and others added 2 commits December 20, 2022 14:50

Update test/fixtures/filters.yml

b283e4a

Co-authored-by: Taico Aerts <devtaeir@taico.nl>

Update test/fixtures/filters.yml

db73643

Co-authored-by: Taico Aerts <devtaeir@taico.nl>

MoshiKoi and others added 3 commits December 20, 2022 15:02

Update app/models/user.rb

cf06f5b

Co-authored-by: Taico Aerts <devtaeir@taico.nl>

Fix order of dependents' destruction

2e1c7e5

Implement user:me search

e5504c2

MoshiKoi added 2 commits December 21, 2022 16:42

Rename param parsing method

f95d41b

Fix error on invalid qualifiers

b205c3e

cellio mentioned this pull request Dec 23, 2022

Filters phase 1: predefined filters for post list and UI for choosing one (set as default for that community + category) #755

Closed

MoshiKoi added 3 commits December 27, 2022 20:27

Fix missed rename

c42fbac

Add uniqueness constraint to filter name

af661b9

Fill in the active filter in search

ade7b35

Taeir mentioned this pull request Jan 21, 2023

Filters updated #976

Merged

MoshiKoi closed this Jan 25, 2023

MoshiKoi deleted the filters branch June 27, 2023 03:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filters #890

Filters #890

MoshiKoi commented Sep 13, 2022

cellio commented Sep 13, 2022

cellio commented Nov 18, 2022

Taeir left a comment •

edited

Loading

Taeir Nov 24, 2022 •

edited

Loading

MoshiKoi Dec 9, 2022

Taeir Nov 24, 2022

MoshiKoi Dec 9, 2022

Taeir Nov 24, 2022

Taeir Nov 24, 2022

MoshiKoi Dec 9, 2022

cellio Dec 9, 2022

MoshiKoi Dec 9, 2022

ArtOfCode- Nov 24, 2022

ArtOfCode- Nov 24, 2022

ArtOfCode- Nov 24, 2022

MoshiKoi Dec 9, 2022

Taeir Jan 21, 2023

Taeir Dec 9, 2022

MoshiKoi Dec 10, 2022

MoshiKoi commented Dec 20, 2022

MoshiKoi commented Dec 21, 2022

MoshiKoi commented Jan 25, 2023

Filters #890

Filters #890

Conversation

MoshiKoi commented Sep 13, 2022

cellio commented Sep 13, 2022

cellio commented Nov 18, 2022

Taeir left a comment • edited Loading

Choose a reason for hiding this comment

Taeir Nov 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoshiKoi commented Dec 20, 2022

MoshiKoi commented Dec 21, 2022

MoshiKoi commented Jan 25, 2023

Taeir left a comment •

edited

Loading

Taeir Nov 24, 2022 •

edited

Loading