Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions app/assets/javascripts/source_filters.js
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);
Comment on lines +3 to +4
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last_id function uses Math.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. The Math.max(..., 0) handles this, but the logic is fragile. Consider using Math.max(0, ...Array.from(existing_list_item_ids)) or returning 0 explicitly when the array is empty for better clarity.

Suggested change
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);
var existing_list_item_ids = $(".source-filter-form").map(function (i, c) { return $(c).data("id-in-filter-list") }).get();
if (existing_list_item_ids.length === 0) {
return 0;
}
var maxId = existing_list_item_ids[0];
for (var i = 1; i < existing_list_item_ids.length; i++) {
if (existing_list_item_ids[i] > maxId) {
maxId = existing_list_item_ids[i];
}
}
return maxId + 1;

Copilot uses AI. Check for mistakes.
},

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");
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The delete function sets _destroy to the string "true", but this should be a boolean value or the string "1" for Rails to recognize it properly in nested attributes. Rails typically expects "1" for true and "0" for false in form submissions. Consider changing to .val(1) or .val("1").

Suggested change
$(this).parents('.source-filter-form').fadeOut().find("input[name$='[_destroy]']").val("true");
$(this).parents('.source-filter-form').fadeOut().find("input[name$='[_destroy]']").val("1");

Copilot uses AI. Check for mistakes.
}
};

document.addEventListener("turbolinks:load", function () {
$('#source-filters')
.on('click', '#add-source-filter-btn', SourceFilters.add)
.on('click', '#add-source-filter-btn-label', SourceFilters.add)
.on('click', '.delete-source-filter-btn', SourceFilters.delete);
$('#source-block-filters')
.on('click', '#add-source-block-filter-btn', SourceFilters.add_block_filter)
.on('click', '#add-source-block-filter-btn-label', SourceFilters.add_block_filter)
.on('click', '.delete-source-filter-btn', SourceFilters.delete);
});
9 changes: 9 additions & 0 deletions app/assets/stylesheets/sources.scss
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;
}
}
24 changes: 13 additions & 11 deletions app/controllers/sources_controller.rb
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
Expand Down Expand Up @@ -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
Expand All @@ -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?
Expand All @@ -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
Expand All @@ -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

Expand Down Expand Up @@ -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]
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parameter list includes keyword_filter which appears to be from an old implementation that was removed in migration 20251218100418. This should be removed from the permitted parameters list.

Suggested change
permitted = %i[url method token default_language enabled keyword_filter source_filters]
permitted = %i[url method token default_language enabled source_filters]

Copilot uses AI. Check for mistakes.
permitted << :approval_status if policy(Source).approve?
permitted << :content_provider_id if policy(Source).index?

params.require(:source).permit(permitted)
params.require(:source).permit(permitted, source_filters_attributes: %i[id filter_mode filter_by filter_value _destroy])
end

def set_breadcrumbs
Expand All @@ -164,7 +167,7 @@ def set_breadcrumbs
add_breadcrumb 'Sources', content_provider_path(@content_provider, anchor: 'sources')

if params[:id]
add_breadcrumb @source.title, content_provider_source_path(@content_provider, @source) if (@source && !@source.new_record?)
add_breadcrumb @source.title, content_provider_source_path(@content_provider, @source) if @source && !@source.new_record?
add_breadcrumb action_name.capitalize.humanize, request.path unless action_name == 'show'
elsif action_name != 'index'
add_breadcrumb action_name.capitalize.humanize, request.path
Expand All @@ -173,5 +176,4 @@ def set_breadcrumbs
super
end
end

end
62 changes: 39 additions & 23 deletions app/models/source.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,13 @@ class Source < ApplicationRecord

belongs_to :user
belongs_to :content_provider
has_many :source_filters, dependent: :destroy

validates :url, :method, presence: true
validates :url, url: true
validates :approval_status, inclusion: { in: APPROVAL_STATUS.values }
validates :method, inclusion: { in: -> (_) { TeSS::Config.user_ingestion_methods } },
unless: -> { User.current_user&.is_admin? || User.current_user&.has_role?(:scraper_user) }
validates :method, inclusion: { in: ->(_) { TeSS::Config.user_ingestion_methods } },
unless: -> { User.current_user&.is_admin? || User.current_user&.has_role?(:scraper_user) }
validates :default_language, controlled_vocabulary: { dictionary: 'LanguageDictionary',
allow_blank: true }
validate :check_method
Expand All @@ -30,6 +31,8 @@ class Source < ApplicationRecord
before_update :log_approval_status_change
before_update :reset_approval_status

accepts_nested_attributes_for :source_filters, allow_destroy: true

if TeSS::Config.solr_enabled
# :nocov:
searchable do
Expand All @@ -44,7 +47,7 @@ class Source < ApplicationRecord
ingestor_title
end
string :content_provider do
self.content_provider.try(:title)
content_provider.try(:title)
end
string :node, multiple: true do
associated_nodes.pluck(:name)
Expand Down Expand Up @@ -72,18 +75,16 @@ def ingestor_class
end

def self.facet_fields
field_list = %w( content_provider node method enabled approval_status )
field_list = %w[content_provider node method enabled approval_status]
field_list.delete('node') unless TeSS::Config.feature['nodes']
field_list
end

def self.check_exists(source_params)
given_source = self.new(source_params)
given_source = new(source_params)
source = nil

if given_source.url.present?
source = self.find_by_url(given_source.url)
end
source = find_by_url(given_source.url) if given_source.url.present?

source
end
Expand Down Expand Up @@ -134,30 +135,45 @@ def self.approval_required?
TeSS::Config.feature['user_source_creation'] && !User.current_user&.is_admin?
end

def passes_filter?(item)
passes = false
allow_all = true

source_filters.each do |filter|
if filter.allow?
allow_all = false
passes ||= filter.match(item)
elsif filter.block? && filter.match(item)
return false
end
end

passes || allow_all
end

private

def set_approval_status
if self.class.approval_required?
self.approval_status = :not_approved
else
self.approval_status = :approved
end
self.approval_status = if self.class.approval_required?
:not_approved
else
:approved
end
end

def reset_approval_status
if self.class.approval_required?
if method_changed? || url_changed?
self.approval_status = :not_approved
end
end
return unless self.class.approval_required?
return unless method_changed? || url_changed?

self.approval_status = :not_approved
end

def log_approval_status_change
if approval_status_changed?
old = (APPROVAL_STATUS[approval_status_before_last_save.to_i] || APPROVAL_STATUS[0]).to_s
new = approval_status.to_s
create_activity(:approval_status_changed, owner: User.current_user, parameters: { old: old, new: new })
end
return unless approval_status_changed?

old = (APPROVAL_STATUS[approval_status_before_last_save.to_i] || APPROVAL_STATUS[0]).to_s
new = approval_status.to_s
create_activity(:approval_status_changed, owner: User.current_user, parameters: { old:, new: })
end

def loggable_changes
Expand Down
67 changes: 67 additions & 0 deletions app/models/source_filter.rb
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
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SourceFilter model lacks validation for required fields. Consider adding validations for filter_mode, filter_by, and filter_value to ensure data integrity. For example: validates :filter_mode, :filter_by, :filter_value, presence: true.

Suggested change
auto_strip_attributes :filter_value
auto_strip_attributes :filter_value
validates :filter_mode, :filter_by, :filter_value, presence: true

Copilot uses AI. Check for mistakes.

enum :filter_by, {
target_audience: 'target_audience',
keyword: 'keyword',
title: 'title',
description: 'description',
description_contains: 'description_contains',
url: 'url',
url_prefix: 'url_prefix',
doi: 'doi',
license: 'license',
difficulty_level: 'difficulty_level',
resource_type: 'resource_type',
prerequisites_contains: 'prerequisites_contains',
learning_objectives_contains: 'learning_objectives_contains',
subtitle: 'subtitle',
subtitle_contains: 'subtitle_contains',
city: 'city',
country: 'country',
event_type: 'event_type',
timezone: 'timezone'
}

enum :filter_mode, {
allow: 'allow',
block: 'block'
}

def match(item)
return false unless item.respond_to?(filter_property)

val = item.send(filter_property)

# string match
if %w[title url doi description license difficulty_level subtitle city country timezone].include?(filter_by)
val.to_s.casecmp(filter_value).zero?
Copy link

Copilot AI Dec 22, 2025

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.

Suggested change
val.to_s.casecmp(filter_value).zero?
val.to_s.casecmp?(filter_value)

Copilot uses AI. Check for mistakes.
# prefix string match
elsif %w[url_prefix].include?(filter_by)
val.to_s.downcase.start_with?(filter_value.downcase)
# contains string match
elsif %w[description_contains prerequisites_contains learning_objectives_contains subtitle_contains].include?(filter_by)
val.to_s.downcase.include?(filter_value.downcase)
# array string match
elsif %w[target_audience keyword resource_type event_type].include?(filter_by)
val.any? { |i| i.to_s.casecmp?(filter_value) }
Copy link

Copilot AI Dec 22, 2025

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) }.

Suggested change
val.any? { |i| i.to_s.casecmp?(filter_value) }
Array.wrap(val).any? { |i| i.to_s.casecmp?(filter_value) }

Copilot uses AI. Check for mistakes.
else
false
end
end

def filter_property
{
'event_type' => 'event_types',
'keyword' => 'keywords',
'url_prefix' => 'url',
'description_contains' => 'description',
'prerequisites_contains' => 'prerequisites',
'learning_objectives_contains' => 'learning_objectives',
'subtitle_contains' => 'subtitle',
'license' => 'licence'
}.fetch(filter_by, filter_by)
end
end
43 changes: 43 additions & 0 deletions app/views/sources/_form.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The template element is using inline style style="display: none" which could be moved to CSS for better separation of concerns. Consider adding a CSS class instead, such as .hidden { display: none; } in the stylesheet.

Suggested change
<template id="source-filter-template" style="display: none">
<template id="source-filter-template">

Copilot uses AI. Check for mistakes.
<%= f.simple_fields_for :source_filters, SourceFilter.new(filter_mode: 'allow'), child_index: "REPLACE_ME" do |ff| %>
<%= render partial: 'source_filter_form', locals: { f: ff } %>
<% end %>
</template>

<% end %>
11 changes: 11 additions & 0 deletions app/views/sources/_source_filter_form.html.erb
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>
Loading
Loading