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

Add manual triage view #167

Merged
merged 4 commits into from
Aug 15, 2024

Conversation

alejandrosame
Copy link
Contributor

@alejandrosame alejandrosame commented Jul 28, 2024

This view allows users to filter CVEs and packages, link them and create NixpkgsIssue from their selections.

Model changes:

  • Added SearchVectorField to improve performance of full-text search.
  • Indices added for performance improvements:
    • GinIndex for SearchVectorFields.
    • BTreeIndex for fields to be searched for filtering and aggregation.

Part of #5

@alejandrosame alejandrosame marked this pull request as ready for review August 10, 2024 20:11
@proofconstruction
Copy link
Contributor

Some notes:

  • Can you share a screenshot of the rendered view?
  • Code-wise this looks OK, but it would be nice to have more comments explaining what things do and why they exist.
  • Is there any reasonable way to split the big commit into smaller ones?
  • Is there a reasonable way to roll back these commits, fast-forward the branch to the latest main, and then apply the commits, instead of having that merge commit in the middle?

Comment on lines +275 to +299
def _get_cve_page(
self, search_cves: str | None, cve_page_number: int
) -> tuple[Page, CustomCountPaginator]:
cve_qs, cve_count_function = self._get_cve_qs(search_cves)
cve_paginator = CustomCountPaginator(
cve_qs, self.paginate_by, custom_count=cve_count_function
)
cve_page = cve_paginator.get_page(cve_page_number)
return cve_page, cve_paginator

def _get_pkg_page(
self, search_pkgs: str | None, pkg_page_number: int
) -> tuple[Page, CustomCountPaginator]:
pkg_qs, pkg_count_function = self._get_pkg_qs(search_pkgs)
pkg_paginator = CustomCountPaginator(
pkg_qs, self.paginate_by, custom_count=pkg_count_function
)
pkg_page = pkg_paginator.get_page(pkg_page_number)
return pkg_page, pkg_paginator
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not critical: One could pass what's currently _get_cve_qs and _get_pkgs_qs as a parameter _get_queryset, and rename the search string and page number variables to be more generic. But it's okay for now, the duplication is not super confusing.

Copy link
Collaborator

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

There are some tiny cosmetic things left, such as check boxes being invalidated on navigating between pages. The JS to handle that is ugly, but okay for now since we're not taking too much care for the front-end side of things.

src/website/webview/views.py Outdated Show resolved Hide resolved
src/website/webview/paginators.py Show resolved Hide resolved
src/website/webview/views.py Outdated Show resolved Hide resolved
Comment on lines +229 to +234
<span class="cve-header-actions">
<input type="checkbox" class="cve-checkbox"
data-id="{{object.cve_id}}" value="{{object.cve_id}}"
name="cve" form="issue-form"
>
</span>
Copy link
Collaborator

@fricklerhandwerk fricklerhandwerk Aug 14, 2024

Choose a reason for hiding this comment

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

Suggested change
<span class="cve-header-actions">
<input type="checkbox" class="cve-checkbox"
data-id="{{object.cve_id}}" value="{{object.cve_id}}"
name="cve" form="issue-form"
>
</span>
<input type="checkbox" class="cve-checkbox"
data-id="{{object.cve_id}}" value="{{object.cve_id}}"
name="cve" form="issue-form"
>

Not a blocker: I don't think the additional wrapper is necessary, you can already select .cve-checkbox.

Comment on lines +235 to +241
<span class="cve-header-description">
<span class="cve-db-id">{{ object.id }}</span>
<span class="cve-id">{{ object.cve_id_code }}</span>
<div class="cve-header-description-long">
{{ object.title | default_to_na }}
</div>
</span>
Copy link
Collaborator

@fricklerhandwerk fricklerhandwerk Aug 14, 2024

Choose a reason for hiding this comment

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

Suggested change
<span class="cve-header-description">
<span class="cve-db-id">{{ object.id }}</span>
<span class="cve-id">{{ object.cve_id_code }}</span>
<div class="cve-header-description-long">
{{ object.title | default_to_na }}
</div>
</span>
<span class="cve-db-id">{{ object.id }}</span>
<span class="cve-id">{{ object.cve_id_code }}</span>
<div class="cve-header-description-long">
{{ object.title | default_to_na }}
</div>

Not a blocker: Possibly not necessary either, because you should be able to control rendering through the container, but not sure from just reading the text.

</span>
</summary>
<div class="cve-extra-info">
<hr>
Copy link
Collaborator

@fricklerhandwerk fricklerhandwerk Aug 14, 2024

Choose a reason for hiding this comment

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

Suggested change
<hr>

Not a blocker: You can invoke that in CSS

src/website/webview/views.py Show resolved Hide resolved
src/website/webview/templates/triage_view.html Outdated Show resolved Hide resolved
@fricklerhandwerk
Copy link
Collaborator

Please rewrite history and I'll merge.

@alejandrosame
Copy link
Contributor Author

Please rewrite history and I'll merge.

Doing it after this last commit. Now there's some basic checkbox state management, but I'm skipping the CSS/HTML change suggestions

RaitoBezarius and others added 4 commits August 15, 2024 15:00
This enables to search through available CVE containers in the database
using a very simple PostgreSQL TS vector search.
According to the specs of the VM.
This view allows users to filter CVEs and packages, link them and create
NixpkgsIssue from their selections.

Model changes:
    - Added **SearchVectorField** to improve performance of full-text search.
    - Indices added for performance improvements:
        - **GinIndex** for SearchVectorFields.
        - **BTreeIndex** for fields to be searched for filtering and aggregation.
@fricklerhandwerk fricklerhandwerk merged commit 18146e0 into Nix-Security-WG:main Aug 15, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants