-
Notifications
You must be signed in to change notification settings - Fork 20
Ingestion filters. Fixes #1192 #1201
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
base: master
Are you sure you want to change the base?
Changes from all commits
078a74a
154629d
ff960b1
0e74413
d0dfe3e
b30a068
006ea47
4142959
a19883a
f2b0854
09350a5
de60848
767872e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,35 @@ | ||||||
| var SourceFilters = { | ||||||
| last_id: function () { | ||||||
| var existing_list_item_ids = $(".source-filter-form").map(function (i, c) { return $(c).data("id-in-filter-list") }); | ||||||
| return Math.max(Math.max.apply(null, existing_list_item_ids) + 1, 0); | ||||||
| }, | ||||||
|
|
||||||
| add: function () { | ||||||
| var new_form = $($('#source-filter-template').clone().html().replace(/REPLACE_ME/g, SourceFilters.last_id())); | ||||||
| new_form.appendTo('#source-filter-list'); | ||||||
|
|
||||||
| return false; // Stop form being submitted | ||||||
| }, | ||||||
|
|
||||||
| add_block_filter: function () { | ||||||
| var new_form = $($('#source-filter-template').clone().html().replace(/REPLACE_ME/g, SourceFilters.last_id()).replace(/allow/, 'block')); | ||||||
| new_form.appendTo('#source-block-list'); | ||||||
|
|
||||||
| return false; // Stop form being submitted | ||||||
| }, | ||||||
|
|
||||||
| delete: function () { | ||||||
| $(this).parents('.source-filter-form').fadeOut().find("input[name$='[_destroy]']").val("true"); | ||||||
|
||||||
| $(this).parents('.source-filter-form').fadeOut().find("input[name$='[_destroy]']").val("true"); | |
| $(this).parents('.source-filter-form').fadeOut().find("input[name$='[_destroy]']").val("1"); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| .source-filter-form { | ||
| display: flex; | ||
| gap: 1em; | ||
| margin-bottom: 4px; | ||
|
|
||
| label { | ||
| margin-right: 4px; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,6 +1,6 @@ | ||||||
| class SourcesController < ApplicationController | ||||||
| before_action :set_source, except: [:index, :new, :create, :check_exists] | ||||||
| before_action :set_content_provider, except: [:index, :check_exists] | ||||||
| before_action :set_source, except: %i[index new create check_exists] | ||||||
| before_action :set_content_provider, except: %i[index check_exists] | ||||||
| before_action :set_breadcrumbs | ||||||
|
|
||||||
| include SearchableIndex | ||||||
|
|
@@ -64,8 +64,8 @@ def check_exists | |||||
| end | ||||||
| else | ||||||
| respond_to do |format| | ||||||
| format.html { render :nothing => true, :status => 200, :content_type => 'text/html' } | ||||||
| format.json { render json: {}, :status => 200, :content_type => 'application/json' } | ||||||
| format.html { render nothing: true, status: 200, content_type: 'text/html' } | ||||||
| format.json { render json: {}, status: 200, content_type: 'application/json' } | ||||||
| end | ||||||
| end | ||||||
| end | ||||||
|
|
@@ -74,6 +74,7 @@ def check_exists | |||||
| # PATCH/PUT /sources/1.json | ||||||
| def update | ||||||
| authorize @source | ||||||
|
|
||||||
| respond_to do |format| | ||||||
| if @source.update(source_params) | ||||||
| @source.create_activity(:update, owner: current_user) if @source.log_update_activity? | ||||||
|
|
@@ -93,8 +94,10 @@ def destroy | |||||
| @source.create_activity :destroy, owner: current_user | ||||||
| @source.destroy | ||||||
| respond_to do |format| | ||||||
| format.html { redirect_to policy(Source).index? ? sources_path : content_provider_path(@content_provider), | ||||||
| notice: 'Source was successfully deleted.' } | ||||||
| format.html do | ||||||
| redirect_to policy(Source).index? ? sources_path : content_provider_path(@content_provider), | ||||||
| notice: 'Source was successfully deleted.' | ||||||
| end | ||||||
| format.json { head :no_content } | ||||||
| end | ||||||
| end | ||||||
|
|
@@ -105,7 +108,7 @@ def test | |||||
| @source.test_job_id = job_id | ||||||
|
|
||||||
| respond_to do |format| | ||||||
| format.json { render json: { id: job_id }} | ||||||
| format.json { render json: { id: job_id } } | ||||||
| end | ||||||
| end | ||||||
|
|
||||||
|
|
@@ -150,11 +153,11 @@ def set_content_provider | |||||
|
|
||||||
| # Never trust parameters from the scary internet, only allow the white list through. | ||||||
| def source_params | ||||||
| permitted = [:url, :method, :token, :default_language, :enabled] | ||||||
| permitted = %i[url method token default_language enabled keyword_filter source_filters] | ||||||
|
||||||
| permitted = %i[url method token default_language enabled keyword_filter source_filters] | |
| permitted = %i[url method token default_language enabled source_filters] |
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,67 @@ | ||||||||
| class SourceFilter < ApplicationRecord | ||||||||
| belongs_to :source | ||||||||
|
|
||||||||
| auto_strip_attributes :filter_value | ||||||||
|
||||||||
| auto_strip_attributes :filter_value | |
| auto_strip_attributes :filter_value | |
| validates :filter_mode, :filter_by, :filter_value, presence: true |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string comparison logic uses casecmp which returns 0 for equal strings, but this only provides case-insensitive equality checking. However, casecmp returns nil when comparing with a non-string value. This could cause a NoMethodError if val is nil. Consider using val.to_s.casecmp?(filter_value) instead, which returns a boolean and handles nil values gracefully.
| val.to_s.casecmp(filter_value).zero? | |
| val.to_s.casecmp?(filter_value) |
Copilot
AI
Dec 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The array comparison using casecmp? could fail if val is nil or not an array. Consider adding a nil check or ensuring val is an array before calling any?, for example: Array.wrap(val).any? { |i| i.to_s.casecmp?(filter_value) }.
| val.any? { |i| i.to_s.casecmp?(filter_value) } | |
| Array.wrap(val).any? { |i| i.to_s.casecmp?(filter_value) } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -36,10 +36,53 @@ | |||||
| include_blank: false %> | ||||||
| <% end %> | ||||||
|
|
||||||
| <h3 style="margin-top: 1em; margin-bottom: .5em;"><%= t('sources.headings.filters') %></h3> | ||||||
|
|
||||||
| <h4><%= t('sources.headings.allow_list') %></h4> | ||||||
| <span class="help-block"><%= t('sources.hints.allow_list') %></span> | ||||||
| <div class="form-group" id="source-filters"> | ||||||
| <div id="source-filter-list"> | ||||||
| <% f.object.source_filters.allow.each do |filter| %> | ||||||
| <%= f.simple_fields_for :source_filters, filter, child_index: filter.id do |ff| %> | ||||||
| <%= render partial: 'source_filter_form', locals: { f: ff } %> | ||||||
| <% end %> | ||||||
| <% end %> | ||||||
| </div> | ||||||
|
|
||||||
| <a href="#" id="add-source-filter-btn" class="btn btn-icon"> | ||||||
| <i class="icon icon-h4 plus-icon"></i> | ||||||
| </a> | ||||||
| <span id="add-source-filter-btn-label" class="help-inline-block help-block"><%= t('sources.hints.add_filter') %></span> | ||||||
| </div> | ||||||
|
|
||||||
| <h4><%= t('sources.headings.block_list') %></h4> | ||||||
| <span class="help-block"><%= t('sources.hints.block_list') %></span> | ||||||
| <div class="form-group" id="source-block-filters"> | ||||||
| <div id="source-block-list"> | ||||||
| <% f.object.source_filters.block.each do |filter| %> | ||||||
| <%= f.simple_fields_for :source_filters, filter, child_index: filter.id do |ff| %> | ||||||
| <%= render partial: 'source_filter_form', locals: { f: ff } %> | ||||||
| <% end %> | ||||||
| <% end %> | ||||||
| </div> | ||||||
|
|
||||||
| <a href="#" id="add-source-block-filter-btn" class="btn btn-icon"> | ||||||
| <i class="icon icon-h4 plus-icon"></i> | ||||||
| </a> | ||||||
| <span id="add-source-block-filter-btn-label" class="help-inline-block help-block"><%= t('sources.hints.add_filter') %></span> | ||||||
| </div> | ||||||
|
|
||||||
|
|
||||||
| <div class="form-group actions"> | ||||||
| <%= f.submit(class: 'btn btn-primary') %> | ||||||
| <%= link_to t('.cancel', default: t("helpers.links.cancel")), | ||||||
| sources_path, class: 'btn btn-default' %> | ||||||
| </div> | ||||||
|
|
||||||
| <template id="source-filter-template" style="display: none"> | ||||||
|
||||||
| <template id="source-filter-template" style="display: none"> | |
| <template id="source-filter-template"> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| <div class="form-inline source-filter-form" data-id-in-filter-list="<%= f.options[:child_index] %>"> | ||
| <%= f.input :filter_by, collection: SourceFilter.filter_bies.keys.map { |t| [t.humanize, t] }, include_blank: false %> | ||
|
|
||
| <%= f.input :filter_value %> | ||
|
|
||
| <%= f.input :filter_mode, collection: SourceFilter.filter_modes.keys.map { |m| [m.humanize, m] }, include_blank: false, as: :hidden %> | ||
|
|
||
| <%= f.input :_destroy, as: :hidden %> | ||
|
|
||
| <button type="button" class="btn btn-danger delete-source-filter-btn">Remove</button> | ||
| </div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
last_idfunction usesMath.max.apply(null, existing_list_item_ids)which could fail if the array is empty or very large. When the array is empty, this returns -Infinity, and then adding 1 results in -Infinity. TheMath.max(..., 0)handles this, but the logic is fragile. Consider usingMath.max(0, ...Array.from(existing_list_item_ids))or returning 0 explicitly when the array is empty for better clarity.