Fixes #37729 - Add version option for deb filter rules #11454
+874
−44
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideEnable version-based filtering in Debian content view rules by introducing comparator UI in the frontend modal, reusing deb_version_cmp SQL scopes in models, refining the debs API for autocomplete, and expanding test coverage across both frontend and backend. Sequence diagram for DEB filter rule creation with version comparatorsequenceDiagram
actor User
participant Modal as AddEditDebPackageRuleModal
participant API as DebsController
participant Model as ContentViewDebFilter
User->>Modal: Open Add DEB rule modal
Modal->>API: Autocomplete package name/arch (as user types)
API-->>Modal: Return autocomplete suggestions
User->>Modal: Selects version comparator and enters version(s)
User->>Modal: Submit rule
Modal->>Model: addCVFilterRule (with name, arch, version/min/max)
Model->>Model: Apply search_version_equal or search_version_range
Model-->>Modal: Rule saved
Modal->>User: Show updated rules and trigger publish-needed event
Entity relationship diagram for ContentViewDebFilter and Deb filter ruleserDiagram
CONTENT_VIEW_DEB_FILTER ||--o{ CONTENT_VIEW_DEB_FILTER_RULE : has
CONTENT_VIEW_DEB_FILTER_RULE {
integer id
string name
string architecture
string version
string min_version
string max_version
}
CONTENT_VIEW_DEB_FILTER {
integer id
string name
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
hstct
force-pushed
the
content_view_filter_deb
branch
2 times, most recently
from
ee56c5f to
2d91bb6
Compare
|
Forgot to add the webpack tests |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @hstct - I've reviewed your changes - here's some feedback:
- In DebVersionSearchTest, a couple of methods are missing the "test_" prefix (e.g. search_version_less_than), so they never run under Minitest—rename them to start with "test_".
- The VersionModifiers/FormSelectOption mapping currently iterates over values and then uses the value as a key, which may misalign label/value pairs; switch to iterating over Object.entries so you can explicitly set option value and label.
- The SearchText fields use defaultValue which makes them uncontrolled after mount—consider passing value and onChange props to keep them as controlled components and keep the UI in sync with state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In DebVersionSearchTest, a couple of methods are missing the "test_" prefix (e.g. search_version_less_than), so they never run under Minitest—rename them to start with "test_".
- The VersionModifiers/FormSelectOption mapping currently iterates over values and then uses the value as a key, which may misalign label/value pairs; switch to iterating over Object.entries so you can explicitly set option value and label.
- The SearchText fields use defaultValue which makes them uncontrolled after mount—consider passing value and onChange props to keep them as controlled components and keep the UI in sync with state.
## Individual Comments
### Comment 1
<location> `webpack/scenes/ContentViews/Details/Filters/Rules/DebPackage/AddEditDebPackageRuleModal.js:60` </location>
<code_context>
+ const [version, setVersion] = useState(editingVersion || '');
+ const [minVersion, setMinVersion] = useState(editingMinVersion || '');
+ const [maxVersion, setMaxVersion] = useState(editingMaxVersion || '');
+ const [versionComparator, setVersionComparator] = useState(versionText(selectedFilterRuleData));
const [saving, setSaving] = useState(false);
const dispatch = useDispatch();
</code_context>
<issue_to_address>
The initial value for versionComparator may not match the expected type.
Since versionComparator stores a localized string, any changes in translations or usage of these keys elsewhere could cause subtle bugs. It's safer to store a non-localized key in state and map to the localized string only for display purposes.
Suggested implementation:
```javascript
const [version, setVersion] = useState(editingVersion || '');
const [minVersion, setMinVersion] = useState(editingMinVersion || '');
const [maxVersion, setMaxVersion] = useState(editingMaxVersion || '');
// Store the key (e.g., 'Equal to', 'Greater than', etc.) instead of the localized string
const [versionComparator, setVersionComparator] = useState(getVersionComparatorKey(selectedFilterRuleData));
const [saving, setSaving] = useState(false);
```
```javascript
const submitDisabled = !name || name.length === 0;
// Compare against the key, not the localized string
const showVersion = versionComparator === VersionModifiers.Equal;
const showMinVersion =
versionComparator === VersionModifiers.Greater ||
versionComparator === VersionModifiers.Range;
const showMaxVersion =
versionComparator === VersionModifiers.Less ||
```
1. You will need to implement the `getVersionComparatorKey(selectedFilterRuleData)` function, which should extract the correct key (e.g., 'Equal', 'Greater', 'Less', 'Range', etc.) from `selectedFilterRuleData`.
2. Ensure that wherever you display the comparator in the UI, you map the key to the localized string using your `versionText` function or similar.
3. If `VersionModifiers` is an object with keys like `Equal`, `Greater`, etc., make sure to use those keys consistently throughout the file.
4. Update any code that sets or reads `versionComparator` to use the key, not the localized string.
</issue_to_address>
### Comment 2
<location> `webpack/scenes/ContentViews/Details/Filters/Rules/DebPackage/AddEditDebPackageRuleModal.js:65` </location>
<code_context>
const dispatch = useDispatch();
const status = useSelector(state => selectCreateFilterRuleStatus(state));
const submitDisabled = !name || name.length === 0;
+ const showVersion = versionComparator === VersionModifiers['Equal to'];
</code_context>
<issue_to_address>
submitDisabled only checks for name, not for required version fields.
Extend submitDisabled to also check that required version fields are filled when the selected comparator requires them. This will prevent submission of incomplete rules.
</issue_to_address>
### Comment 3
<location> `webpack/scenes/ContentViews/Details/Filters/Rules/DebPackage/AddEditDebPackageRuleModal.js:140` </location>
<code_context>
isOpen
onClose={onClose}
appendTo={document.body}
+ help={
+ <Popover
+ headerContent={__('Help')}
+ bodyContent={__('Filter rules may include or exclude specific Debian packages' +
+ 'by name, architecture, and version. Version comparisons follow Debian policy' +
+ '(epoch:upstream-revision; "~" sorts before empty). Leave all version fields' +
+ 'blank to match all version.')}
+ >
+ <Button variant="plain" aria-label="Help" ouiaId="deb-filter-rule-modal-help">
+ <HelpIcon />
+ </Button>
+ </Popover>
+ }
>
</code_context>
<issue_to_address>
The help popover text contains a minor typo: 'blank to match all version.'
It should read 'all versions' instead of 'all version.'
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
'blank to match all version.')}
=======
'blank to match all versions.')}
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `webpack/scenes/ContentViews/Details/Filters/Rules/DebPackage/AddEditDebPackageRuleModal.js:192` </location>
<code_context>
+ name="version_comparator"
+ aria-label="version_comparator"
+ >
+ {Object.values(VersionModifiers).map(v => (
+ <FormSelectOption key={v} value={v} label={VersionModifiers[v]} />
+ ))}
+ </FormSelect>
</code_context>
<issue_to_address>
Using localized strings as keys in FormSelectOption may cause issues with translation changes.
Use a stable, non-localized key and value for FormSelectOption, reserving the localized string for the label to prevent issues with translation changes or duplicate labels.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
{Object.values(VersionModifiers).map(v => (
<FormSelectOption key={v} value={v} label={VersionModifiers[v]} />
))}
=======
{Object.entries(VersionModifiers).map(([modifierKey, localizedLabel]) => (
<FormSelectOption key={modifierKey} value={modifierKey} label={localizedLabel} />
))}
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `test/models/deb_test.rb:53` </location>
<code_context>
+ refute_includes result, @deb_one
+ end
+
+ def search_version_less_than
+ result = Deb.in_repositories(@repo).search_for('version < 1.1')
+ assert_includes result, @deb_one
+ refute_includes result, @deb_one_v2
+ end
+
</code_context>
<issue_to_address>
Test method 'search_version_less_than' is missing 'test_' prefix.
Please rename the method to 'test_search_version_less_than' so it is recognized and executed as a test.
</issue_to_address>
### Comment 6
<location> `test/models/deb_test.rb:59` </location>
<code_context>
+ refute_includes result, @deb_one_v2
+ end
+
+ def search_version_equal
+ result = Deb.in_repositories(@repo).search_for('version = 1.0')
+ assert_includes result, @deb_one
+ refute_includes result, @deb_one_v2
+ end
+
</code_context>
<issue_to_address>
Test method 'search_version_equal' is missing 'test_' prefix.
Please rename the method to 'test_search_version_equal' so it is recognized and executed as a test.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const [version, setVersion] = useState(editingVersion || ''); | ||
| const [minVersion, setMinVersion] = useState(editingMinVersion || ''); | ||
| const [maxVersion, setMaxVersion] = useState(editingMaxVersion || ''); | ||
| const [versionComparator, setVersionComparator] = useState(versionText(selectedFilterRuleData)); |
There was a problem hiding this comment.
suggestion (bug_risk): The initial value for versionComparator may not match the expected type.
Since versionComparator stores a localized string, any changes in translations or usage of these keys elsewhere could cause subtle bugs. It's safer to store a non-localized key in state and map to the localized string only for display purposes.
Suggested implementation:
const [version, setVersion] = useState(editingVersion || '');
const [minVersion, setMinVersion] = useState(editingMinVersion || '');
const [maxVersion, setMaxVersion] = useState(editingMaxVersion || '');
// Store the key (e.g., 'Equal to', 'Greater than', etc.) instead of the localized string
const [versionComparator, setVersionComparator] = useState(getVersionComparatorKey(selectedFilterRuleData));
const [saving, setSaving] = useState(false); const submitDisabled = !name || name.length === 0;
// Compare against the key, not the localized string
const showVersion = versionComparator === VersionModifiers.Equal;
const showMinVersion =
versionComparator === VersionModifiers.Greater ||
versionComparator === VersionModifiers.Range;
const showMaxVersion =
versionComparator === VersionModifiers.Less ||- You will need to implement the
getVersionComparatorKey(selectedFilterRuleData)function, which should extract the correct key (e.g., 'Equal', 'Greater', 'Less', 'Range', etc.) fromselectedFilterRuleData. - Ensure that wherever you display the comparator in the UI, you map the key to the localized string using your
versionTextfunction or similar. - If
VersionModifiersis an object with keys likeEqual,Greater, etc., make sure to use those keys consistently throughout the file. - Update any code that sets or reads
versionComparatorto use the key, not the localized string.
webpack/scenes/ContentViews/Details/Filters/Rules/DebPackage/AddEditDebPackageRuleModal.js
Show resolved
Hide resolved
webpack/scenes/ContentViews/Details/Filters/Rules/DebPackage/AddEditDebPackageRuleModal.js
Outdated
Show resolved
Hide resolved
Comment on lines
+192
to
+194
| {Object.values(VersionModifiers).map(v => ( | ||
| <FormSelectOption key={v} value={v} label={VersionModifiers[v]} /> | ||
| ))} |
There was a problem hiding this comment.
suggestion (bug_risk): Using localized strings as keys in FormSelectOption may cause issues with translation changes.
Use a stable, non-localized key and value for FormSelectOption, reserving the localized string for the label to prevent issues with translation changes or duplicate labels.
Suggested change
| {Object.values(VersionModifiers).map(v => ( | |
| <FormSelectOption key={v} value={v} label={VersionModifiers[v]} /> | |
| ))} | |
| {Object.entries(VersionModifiers).map(([modifierKey, localizedLabel]) => ( | |
| <FormSelectOption key={modifierKey} value={modifierKey} label={localizedLabel} /> | |
| ))} |
hstct
force-pushed
the
content_view_filter_deb
branch
from
2d91bb6 to
31767a7
Compare
m-bucher
force-pushed
the
content_view_filter_deb
branch
from
2e85c42 to
709d6e1
Compare
Co-Authored-by: Markus Bucher <[email protected]> Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
m-bucher
force-pushed
the
content_view_filter_deb
branch
from
709d6e1 to
caed97d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What are the changes introduced in this pull request?
deb_version_cmpfunctions to fix the version search and version search ranges for Debian content.Considerations taken when implementing this change?
What are the testing steps for this pull request?
Summary by Sourcery
Introduce version-based filtering in Debian package filter rules, align the DEB filter UI with RPM conventions, and ensure backend support for version comparisons.
New Features:
Bug Fixes:
Enhancements:
Tests: