Skip to content
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

Fixes #37946 - Add 'other' option for package type in errata filters #11188

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
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
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ def resource_class
param :types, Array, :desc => N_("erratum: types (enhancement, bugfix, security)")
param :date_type, String, :desc => N_("erratum: search using the 'Issued On' or 'Updated On' column of the errata. Values are 'issued'/'updated'")
param :module_stream_ids, Array, :desc => N_("module stream ids")
param :allow_other_types, :bool, :desc => N_("erratum: allow types not matching a valid errata type")
def create
rule_clazz = ContentViewFilter.rule_class_for(@filter)

Expand Down Expand Up @@ -89,6 +90,7 @@ def show
param :start_date, String, :desc => N_("erratum: start date (YYYY-MM-DD)")
param :end_date, String, :desc => N_("erratum: end date (YYYY-MM-DD)")
param :types, Array, :desc => N_("erratum: types (enhancement, bugfix, security)")
param :allow_other_types, :bool, :desc => N_("erratum: allow types not matching a valid errata type")
def update
update_params = rule_params
update_params[:name] = update_params[:name].first if update_params[:name]
Expand Down Expand Up @@ -136,7 +138,7 @@ def rule_params

@rule_params ||= params.fetch(:content_view_filter_rule, {}).
permit(:uuid, :version, :min_version, :max_version, :architecture,
:errata_id, :start_date, :end_date, :date_type,
:errata_id, :start_date, :end_date, :date_type, :allow_other_types,
:types => [], :module_stream_ids => [], :errata_ids => [], name: [])
end

Expand Down
13 changes: 12 additions & 1 deletion app/controllers/katello/api/v2/errata_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,20 @@ def custom_index_relation(collection)
fail HttpErrors::UnprocessableEntity, msg
end

custom_index_relation_handle_type_and_time(collection)
end

def custom_index_relation_handle_type_and_time(collection)
collection = collection.where("#{date_type} >= ?", params[:start_date]) if params[:start_date]
collection = collection.where("#{date_type} <= ?", params[:end_date]) if params[:end_date]
collection = collection.of_type(params[:types]) if params[:types]
if params[:types]
include_other = params[:types]&.include?('other')
params[:types]&.delete('other')
collection = collection.of_type(
params[:types],
include_other
)
end
collection
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ module Katello
module Validators
class ContentViewErratumFilterRuleValidator < ActiveModel::Validator
def validate(record)
if record.errata_id.blank? && record.start_date.blank? && record.end_date.blank? && record.types.blank?
if record.errata_id.blank? && record.start_date.blank? && record.end_date.blank? && record.types.blank? && record.allow_other_types == false
invalid_parameters = _("Invalid erratum filter rule specified, Must specify at least one of the following:" \
" 'errata_id', 'start_date', 'end_date' or 'types'")
" 'errata_id', 'start_date', 'end_date', 'types', or 'allow_other_types'")
record.errors.add(:base, invalid_parameters)
return
Copy link
Member

@sjha4 sjha4 Nov 25, 2024

Choose a reason for hiding this comment

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

I am seeing an error updating a date range filter by removing the regular types and keeping only the "Other" type selected :
May not add an id rule to a filter that has an existing type or date range rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look into this shortly. Thank you Samir!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Samir, I haven't been able to replicate this on my end. How can I reproduce this?

Copy link
Member

Choose a reason for hiding this comment

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

I hit a similar weird issue:

  1. Create errata date filter
  2. Set the types to only be other
  3. Add a date range
  4. Save the filter rule
  5. Go back into the filter rule
  6. See UI error and blank screen
  7. Refresh the page
  8. See that the filter rule was somehow turned from an errata date rule to an errata by ID rule

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This issue should now be fixed. It was a combination of two issues: the page rendering as 'id' when no types existed and the initial selected types erroring out when types was blank/undefined.

end
Expand Down
20 changes: 18 additions & 2 deletions app/models/katello/content_view_erratum_filter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,21 @@ def erratum_arel
end

def types_clause
# Create an array to store output clauses for quick type filtering later
conditions = []

# Add clauses for types in the filter
types = erratum_rules.first.types
return if types.blank?
errata_types_in(types)
conditions << errata_types_in(types) unless types.blank?

# Add clauses for 'other' types
conditions << errata_types_not_in(Erratum::TYPES) if erratum_rules.first.allow_other_types?

# Reduce the array of clauses to a single clause and return
return if conditions.empty?
conditions.reduce(nil) do |combined_clause, condition|
combined_clause ? combined_clause.or(condition) : condition
end
end

def filter_by_id?
Expand All @@ -105,6 +117,10 @@ def errata_types_in(types)
erratum_arel[:errata_type].in(types)
end

def errata_types_not_in(types)
erratum_arel[:errata_type].not_in(types)
end

def errata_in(ids)
erratum_arel[:errata_id].in(ids)
end
Expand Down
10 changes: 8 additions & 2 deletions app/models/katello/erratum.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,14 @@ class Erratum < Katello::Model
:validator => ->(value) { ['true', 'false'].include?(value.downcase) },
:operators => ["="]

def self.of_type(type)
where(:errata_type => type)
def self.of_type(type, include_other = false)
if include_other
where.not(
:errata_type => [Erratum::SECURITY, Erratum::BUGZILLA, Erratum::ENHANCEMENT].flatten
).or(where(:errata_type => type))
else
where(:errata_type => type)
end
end

scope :security, -> { of_type(Erratum::SECURITY) }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ attributes :start_date, :if => lambda { |rule| rule.respond_to?(:start_date) &&
attributes :end_date, :if => lambda { |rule| rule.respond_to?(:end_date) && !rule.end_date.blank? }
attributes :architecture, :if => lambda { |rule| rule.respond_to?(:architecture) && !rule.architecture.blank? }
attributes :types, :if => lambda { |rule| rule.respond_to?(:types) && !rule.types.blank? }
attributes :allow_other_types, :if => lambda { |rule| rule.respond_to?(:allow_other_types) }
attributes :date_type, :if => lambda { |rule| rule.respond_to?(:date_type) }
attributes :module_stream_id, :if => lambda { |rule| rule.respond_to?(:module_stream_id) && !rule.module_stream_id.blank? }
if @resource&.try(:module_stream)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
class AddAllowOtherTypesToContentViewErratumFilterRules < ActiveRecord::Migration[6.1]
def change
add_column :katello_content_view_erratum_filter_rules, :allow_other_types, :boolean,
:default => false, :null => false
end
end
51 changes: 45 additions & 6 deletions test/models/content_view_erratum_filter_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ def setup
@repo = katello_repositories(:fedora_17_x86_64)
end

TYPICAL_TYPES_RESPONSE =
" AND \"katello_errata\".\"errata_type\" IN ('bugfix', 'enhancement', 'security')".freeze

def test_erratum_by_id_returns_arel_for_specified_errata_id
erratum = katello_errata(:security)
@repo.errata = [erratum]
Expand All @@ -24,7 +27,7 @@ def test_errata_by_start_date_returns_arel_for_errata_by_updated_date_and_errata
filter = id_rule.filter
filter.reload

assert_equal "\"katello_errata\".\"updated\" >= '#{start_date}' AND \"katello_errata\".\"errata_type\" IN ('bugfix', 'enhancement', 'security')",
assert_equal "\"katello_errata\".\"updated\" >= '#{start_date}'" + TYPICAL_TYPES_RESPONSE,
filter.generate_clauses(@repo).to_sql
end

Expand All @@ -35,7 +38,7 @@ def test_errata_by_start_date_returns_arel_for_errata_by_issued_date_and_errata_
filter = id_rule.filter
filter.reload

assert_equal "\"katello_errata\".\"issued\" >= '#{start_date}' AND \"katello_errata\".\"errata_type\" IN ('bugfix', 'enhancement', 'security')",
assert_equal "\"katello_errata\".\"issued\" >= '#{start_date}'" + TYPICAL_TYPES_RESPONSE,
filter.generate_clauses(@repo).to_sql
end

Expand All @@ -45,7 +48,7 @@ def test_errata_by_end_date_returns_arel_for_errata_by_updated_date_and_errata_t
filter = id_rule.filter
filter.reload

assert_equal "\"katello_errata\".\"updated\" <= '#{end_date}' AND \"katello_errata\".\"errata_type\" IN ('bugfix', 'enhancement', 'security')",
assert_equal "\"katello_errata\".\"updated\" <= '#{end_date}'" + TYPICAL_TYPES_RESPONSE,
filter.generate_clauses(@repo).to_sql
end

Expand All @@ -56,7 +59,7 @@ def test_errata_by_end_date_returns_arel_for_errata_by_issued_date_and_errata_ty
filter = id_rule.filter
filter.reload

assert_equal "\"katello_errata\".\"issued\" <= '#{end_date}' AND \"katello_errata\".\"errata_type\" IN ('bugfix', 'enhancement', 'security')",
assert_equal "\"katello_errata\".\"issued\" <= '#{end_date}'" + TYPICAL_TYPES_RESPONSE,
filter.generate_clauses(@repo).to_sql
end

Expand All @@ -69,6 +72,16 @@ def test_errata_by_type_returns_arel_by_errata_type
filter.generate_clauses(@repo).to_sql
end

def test_errata_by_type_returns_arel_by_errata_type_other
id_rule = FactoryBot.create(:katello_content_view_erratum_filter_rule, :allow_other_types => true)
id_rule.update!(types: [])
filter = id_rule.filter
filter.reload

assert_equal "\"katello_errata\".\"errata_type\" NOT IN ('security', 'bugfix', 'recommended', 'enhancement', 'optional')",
filter.generate_clauses(@repo).to_sql
end

def test_content_unit_pulp_ids_with_empty_errata_list_returns_empty_result
rpm1 = @repo.rpms.first
rpm2 = @repo.rpms.last
Expand Down Expand Up @@ -209,13 +222,16 @@ def test_content_unit_pulp_ids_by_issued_end_date_returns_pulp_hrefs
end

def test_content_unit_pulp_ids_by_errata_type
rpm1 = @repo.rpms.first
rpm2 = @repo.rpms.last
rpm1 = @repo.rpms[0]
rpm2 = @repo.rpms[1]
rpm3 = @repo.rpms[2]

erratum1 = Katello::Erratum.new(:pulp_id => "one", :errata_id => "ERRATA1", :errata_type => 'bugfix')
erratum1.packages << Katello::ErratumPackage.new(:filename => rpm1.filename, :name => "e1", :nvrea => "e1")
erratum2 = Katello::Erratum.new(:pulp_id => "two", :errata_id => "ERRATA2", :errata_type => 'security')
erratum2.packages << Katello::ErratumPackage.new(:filename => rpm2.filename, :name => "e2", :nvrea => "e2")
erratum3 = Katello::Erratum.new(:pulp_id => "three", :errata_id => "ERRATA3", :errata_type => 'not_recognized')
erratum3.packages << Katello::ErratumPackage.new(:filename => rpm3.filename, :name => "e3", :nvrea => "e3")

@repo.errata = [erratum2]
@repo.save!
Expand All @@ -226,5 +242,28 @@ def test_content_unit_pulp_ids_by_errata_type

assert_equal [rpm2.pulp_id], filter.content_unit_pulp_ids(@repo)
end

def test_content_unit_pulp_ids_by_errata_type_other
rpm1 = @repo.rpms[0]
rpm2 = @repo.rpms[1]
rpm3 = @repo.rpms[2]

erratum1 = Katello::Erratum.new(:pulp_id => "one", :errata_id => "ERRATA1", :errata_type => 'bugfix')
erratum1.packages << Katello::ErratumPackage.new(:filename => rpm1.filename, :name => "e1", :nvrea => "e1")
erratum2 = Katello::Erratum.new(:pulp_id => "two", :errata_id => "ERRATA2", :errata_type => 'security')
erratum2.packages << Katello::ErratumPackage.new(:filename => rpm2.filename, :name => "e2", :nvrea => "e2")
erratum3 = Katello::Erratum.new(:pulp_id => "three", :errata_id => "ERRATA3", :errata_type => 'not_recognized')
erratum3.packages << Katello::ErratumPackage.new(:filename => rpm3.filename, :name => "e3", :nvrea => "e3")

@repo.errata = [erratum3]
@repo.save!

id_rule = FactoryBot.create(:katello_content_view_erratum_filter_rule, :allow_other_types => true)
id_rule.update!(types: [])
filter = id_rule.filter
filter.reload

assert_equal [rpm3.pulp_id], filter.content_unit_pulp_ids(@repo)
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -46,20 +46,36 @@ const CVErrataDateFilterContent = ({
selectCVFilterDetails(state, cvId, filterId), shallowEqual);
const { repositories = [], rules } = filterDetails;
const [{
id, types, start_date: ruleStartDate, end_date: ruleEndDate, date_type: ruleDateType,
id,
types,
allow_other_types: ruleAllowOtherTypes,
start_date: ruleStartDate,
end_date: ruleEndDate,
date_type: ruleDateType,
} = {}] = rules;
const { permissions } = details;
const [startDate, setStartDate] = useState(convertAPIDateToUIFormat(ruleStartDate));
const [endDate, setEndDate] = useState(convertAPIDateToUIFormat(ruleEndDate));
const [dateType, setDateType] = useState(ruleDateType);
const [dateTypeSelectOpen, setDateTypeSelectOpen] = useState(false);
const [typeSelectOpen, setTypeSelectOpen] = useState(false);
const [selectedTypes, setSelectedTypes] = useState(types);
const dispatch = useDispatch();
const [activeTabKey, setActiveTabKey] = useState(0);
const [startEntry, setStartEntry] = useState(false);
const [endEntry, setEndEntry] = useState(false);

const getInitialSelectedTypes = () => {
if (!types) {
return ['other'];
}
if (ruleAllowOtherTypes) {
return [...types, 'other'];
}
return types;
};

const [selectedTypes, setSelectedTypes] = useState(getInitialSelectedTypes());

const onSave = () => {
dispatch(editCVFilterRule(
filterId,
Expand All @@ -68,8 +84,9 @@ const CVErrataDateFilterContent = ({
content_view_filter_id: filterId,
start_date: startDate && startDate !== '' ? dateParse(startDate) : null,
end_date: endDate && endDate !== '' ? dateParse(endDate) : null,
types: selectedTypes,
types: selectedTypes.filter(e => e !== 'other'),
date_type: dateType,
allow_other_types: selectedTypes.includes('other'),
},
() => {
dispatch({ type: CONTENT_VIEW_NEEDS_PUBLISH });
Expand All @@ -81,15 +98,21 @@ const CVErrataDateFilterContent = ({
const resetFilters = () => {
setStartDate(convertAPIDateToUIFormat(ruleStartDate));
setEndDate(convertAPIDateToUIFormat(ruleEndDate));
setSelectedTypes(types);
setDateType(ruleDateType);
setSelectedTypes(getInitialSelectedTypes());
};

const onTypeSelect = (selection) => {
if (selectedTypes.includes(selection)) {
// If the selection is the only selection remaining, do not allow it to be removed
if (selectedTypes.length === 1) return;

// Filter out the current selection to deselect it
setSelectedTypes(selectedTypes.filter(e => e !== selection));
} else setSelectedTypes([...selectedTypes, selection]);
} else {
// Add the selection to the selected types
setSelectedTypes([...selectedTypes, selection]);
}
};

const singleSelection = selection => (selectedTypes.length === 1
Expand All @@ -99,7 +122,7 @@ const CVErrataDateFilterContent = ({
(
isEqual(convertAPIDateToUIFormat(ruleStartDate), startDate) &&
isEqual(convertAPIDateToUIFormat(ruleEndDate), endDate) &&
isEqual(sortBy(types), sortBy(selectedTypes)) &&
isEqual(sortBy(getInitialSelectedTypes()), sortBy(selectedTypes)) &&
isEqual(ruleDateType, dateType)
);

Expand Down Expand Up @@ -171,14 +194,23 @@ const CVErrataDateFilterContent = ({
{__('Bugfix')}
</p>
</SelectOption>
<SelectOption
isDisabled={singleSelection('other') || !hasPermission(permissions, 'edit_content_views')}
key="other"
value="other"
>
<p style={{ marginTop: '4px' }}>
{__('Other')}
</p>
</SelectOption>
</Select>
</FlexItem>
<FlexItem span={1} spacer={{ default: 'spacerNone' }}>
{(selectedTypes.length === 1) &&
<Tooltip
position="top"
content={
__('Atleast one errata type needs to be selected.')
__('At least one errata type option needs to be selected.')
}
>
<OutlinedQuestionCircleIcon />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ const CVErrataIDFilterContent = ({
const hasNotAddedSelected = rows.some(({ selected, added }) => selected && !added);
const [statusSelected, setStatusSelected] = useState(ALL_STATUSES);
const [typeSelectOpen, setTypeSelectOpen] = useState(false);
const [selectedTypes, setSelectedTypes] = useState(ERRATA_TYPES);
const [selectedTypes, setSelectedTypes] = useState([...ERRATA_TYPES, 'other']);
const [startDate, setStartDate] = useState('');
const [endDate, setEndDate] = useState('');
const activeFilters = [statusSelected, selectedTypes, startDate, endDate];
Expand Down Expand Up @@ -198,9 +198,15 @@ const CVErrataIDFilterContent = ({

const onTypeSelect = (selection) => {
if (selectedTypes.includes(selection)) {
// If the selection is the only selection remaining, do not allow it to be removed
if (selectedTypes.length === 1) return;

// Filter out the current selection to deselect it
setSelectedTypes(selectedTypes.filter(e => e !== selection));
} else setSelectedTypes([...selectedTypes, selection]);
} else {
// Add the current selection to the selected types
setSelectedTypes([...selectedTypes, selection]);
}
setTypeSelectOpen(false);
};

Expand Down Expand Up @@ -326,6 +332,11 @@ const CVErrataIDFilterContent = ({
{__('Bugfix')}
</p>
</SelectOption>
<SelectOption isDisabled={singleSelection('bugfix')} key="other" value="other">
<p style={{ marginTop: '4px' }}>
{__('Other')}
</p>
</SelectOption>
</Select>
</SplitItem>
{hasPermission(permissions, 'edit_content_views') &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ const CVFilterDetailType = ({
details={details}
/>);
case 'erratum':
if (head(rules)?.types) {
if (head(rules)?.types || head(rules)?.allow_other_types) {
return (<CVErrataDateFilterContent
cvId={cvId}
filterId={filterId}
Expand Down
Loading