Add pagination to joblistings #3792
Conversation
itsisak
added
review-needed
|
Have you added pagination to the frontend? |
📝 WalkthroughWalkthroughThe changes add a new Boolean filter to the job listings filter set. A method named Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FilterSet
participant DB
Client->>FilterSet: Request job listings with timeFilter parameter
alt timeFilter is True
FilterSet->>DB: Query listings where visible_from ≤ now ≤ visible_to
DB-->>FilterSet: Return filtered listings
else timeFilter is False
FilterSet-->>Client: Return unmodified queryset
end
sequenceDiagram
participant Client
participant ViewSet
participant FilterSet
participant DB
Client->>ViewSet: API request with filter parameters
ViewSet->>FilterSet: Apply filtering logic
ViewSet->>ViewSet: Check if filter parameters are present
alt Filters applied
ViewSet->>ViewSet: Set pagination_class to None
else
ViewSet->>ViewSet: Use default pagination settings
end
ViewSet->>DB: Retrieve job listings (filtered or unfiltered)
DB-->>ViewSet: Return queryset
ViewSet-->>Client: Send response
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lego/apps/joblistings/filters.py (1)
9-9: Consider using snake_case naming convention for consistency.The filter name
timeFilteruses camelCase, which is inconsistent with Python/Django naming conventions and the other filtercreated_afterthat uses snake_case. Consider renaming totime_filterfor consistency.- timeFilter = BooleanFilter(method="filter_time") + time_filter = BooleanFilter(method="filter_time")lego/apps/joblistings/views.py (1)
53-57: Consider a more explicit check for filter values.The current implementation disables pagination if any filter parameter exists and has a non-empty value. For more precise control, consider explicitly checking if the filter values are meaningful rather than just present.
- if params and any( - params.get(key) for key in self.filterset_class.get_filters() - ): + if params and any( + filter_value for key in self.filterset_class.get_filters() + if (filter_value := params.get(key)) and filter_value not in ('', 'false', '0') + ):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lego/apps/joblistings/filters.py(1 hunks)lego/apps/joblistings/views.py(1 hunks)
🔇 Additional comments (3)
lego/apps/joblistings/filters.py (2)
1-2: Updated imports to support new time filtering functionality.The addition of
timezoneandBooleanFilterimports supports the newly added time filtering capability.
15-20: Effective implementation of time-based filtering.The
filter_timemethod correctly filters job listings based on their visibility window when the filter is active. This implementation aligns with the PR objective of enhancing filtering capabilities while maintaining the ability to view all job listings when needed.lego/apps/joblistings/views.py (1)
51-57:❓ Verification inconclusive
Pagination is now disabled when any filter is applied.
Good implementation that disables pagination when any filter parameter is applied, aligning with the PR objective of maintaining consistency with existing functionality. The approach checks for the presence and non-emptiness of any filter parameter.
However, there might be a potential issue: if a parameter exists but has an empty or "false" value, pagination will still be disabled. Verify that this is the intended behavior.
🏁 Script executed:
#!/bin/bash # Check if there are any cases where parameters with empty values are handled in the codebase rg -A 2 -B 2 "params\.(get|getlist)\(.*\)" --glob "*.py" | grep -v "if not" | grep -v "if params\."Length of output: 4593
Attention: Verify Filter Parameter Truthiness in Pagination Disabling
The current implementation in
lego/apps/joblistings/views.py(lines 51–57) correctly disables pagination when any filter parameter is provided. The check usingany(params.get(key) for key in self.filterset_class.get_filters())evaluates the truthiness of each parameter. This means that if a parameter is present with a non-empty value (for example,"false"as a string), pagination will be disabled—even if the value might logically represent a false state. In contrast, an empty string would evaluate as false and not disable pagination.Notably, similar patterns are found in other parts of the codebase, though there is some variation (e.g., an explicit check like
if descendants == "true"in memberships). Please review the business logic to ensure that treating any non-empty value (including"false") as a trigger for disabling pagination is the intended behavior.
- Confirm if parameters with values like
"false"should indeed disable pagination.- Consider whether stricter type/value validations are necessary for specific filters.
No, but I believe there are no places in frontend that use this endpoint without filtering. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3792 +/- ##
==========================================
- Coverage 89.97% 89.96% -0.01%
==========================================
Files 740 740
Lines 23962 23967 +5
==========================================
+ Hits 21560 21563 +3
- Misses 2402 2404 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| from lego.apps.joblistings.models import Joblisting | ||
|
|
||
|
|
||
| class JoblistingFilterSet(FilterSet): | ||
| created_after = DateFilter("created_at", lookup_expr="gte") | ||
| timeFilter = BooleanFilter(method="filter_time") |
There was a problem hiding this comment.
Wait is this correct? A Boolean?
There was a problem hiding this comment.
Also, does it have to be camelCase?
There was a problem hiding this comment.
Wait is this correct? A Boolean?
What else? It is used like this api/v1/joblistings?timeFilter=true.
Also, does it have to be camelCase?
Since that is how it is written in the request (see above) it is how Django maps params. So in order to change it to it seems I would have to add custom logic for it and I dont think that is worth it. Also there are other instances where camelCase is used in similar scenarios.
|
Hopefully resolves webkom/uptime#99 |
It really should not in my opinion, however my goal here was mainly to improve the healthcheck taking forever and did not want to update all the usages in frontend to handle pagination. Since the frontend never uses the endpoint without filtering (only the healthcheck do) this should keep the current functionality for joblistings. |
Description
Add pagination to joblistings, since now whenever anyone access
api/v1/joblistingsit returns all joblistings ever.Disable pagination whenever filters are applied, to stay consistent with previous functionality.
This aims to improve speed of health check.
Testing
Tests work, so i guess... isss good?
Resolves ABA-1366