-
Notifications
You must be signed in to change notification settings - Fork 201
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
Pagination Updated - Items per page option added #1625
base: main
Are you sure you want to change the base?
Conversation
Hi @pombredanne @TG1999 @keshav-space @Hritik14 , Can you please review this PR It has passed all workflow tests and was also working fine on my local server. Thank you |
d8068aa
to
c28765a
Compare
e4a1a18
to
8d177c8
Compare
Signed-off-by: Rishi Garg <[email protected]>
8d177c8
to
6ca25ee
Compare
@tdruez please see the changes now workflow tests are also successful . |
@tdruez I would request you to please review the PR I have made changes In the code after you requested some changes after first review. |
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.
See the various comments.
Generally, this implementation seems too complex and not "reusable" enough at it requires to many moving parts.
@@ -3,7 +3,7 @@ | |||
# VulnerableCode is a trademark of nexB Inc. | |||
# SPDX-License-Identifier: Apache-2.0 | |||
# See http://www.apache.org/licenses/LICENSE-2.0 for the license text. | |||
# See https://github.com/aboutcode-org/vulnerablecode for support or download. |
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.
Unwanted changes
@@ -12,26 +12,87 @@ | |||
|
|||
from vulnerabilities.models import ApiUser | |||
|
|||
from .models import * |
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.
Avoid * imports.
widget=forms.Select( | ||
attrs={ | ||
"class": "select is-small", | ||
"onchange": "handlePageSizeChange(this.value)", |
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.
Avoid inline JavaScript.
def clean_search(self): | ||
return self.cleaned_data.get("search", "") |
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.
What is the purpose of this?
def get_queryset(self, query=None): | ||
""" | ||
Get queryset with search/filter/ordering applied. | ||
Args: | ||
query (str, optional): Direct query for testing | ||
""" | ||
if query is not None: | ||
return self._search(query) | ||
|
||
if not self.is_valid(): | ||
return self.model.objects.none() | ||
|
||
return self._search(self.clean_search()) |
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.
What's the need for this logic?
def _search(self, query): | ||
"""Execute package-specific search logic.""" | ||
return ( | ||
self.model.objects.search(query) | ||
.with_vulnerability_counts() | ||
.prefetch_related() | ||
.order_by("package_url") | ||
) |
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.
This is not following the Django forms conventions.
def _search(self, query): | ||
"""Execute vulnerability-specific search logic.""" | ||
return self.model.objects.search(query=query).with_package_counts() |
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.
Same comment as above.
<a href="?page={{ page_obj.previous_page_number }}&search={{ search|urlencode }}&page_size={{ page_obj.paginator.per_page }}" | ||
class="pagination-previous">Previous</a> |
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.
What happens if other filters are active, those are lost when using the pagination?
|
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.
All the complexity of those templates should likely be handled in Python.
model = models.Package | ||
template_name = "packages.html" | ||
ordering = ["type", "namespace", "name", "version"] | ||
class BaseSearchView(ListView): |
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.
Could we use the django_filters.views.FilterView
instead of re-implementing it?
Add pagination to Package Search and Vulnerability Search #1616 #1617
This change aligns Package Search functionality and Vulnerability Search, improving consistency across the application and enhancing user control over result display.
For Package Search:
For Vulnerability Search:
Signed-off-by: Rishi Garg [email protected]