Created a new page for showing Banned_apps with backend_api #3934
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨ |
igennova
changed the title
|
Looks good! How does one get to this page? |
|
Should i add it to the Sidenavbar ? |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
WalkthroughThis pull request introduces a banned apps feature. A new JSON file is added to store details about banned applications. URL routes are updated with new endpoints for accessing a banned apps view and an API search function. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as Banned Apps UI
participant API as Search API Endpoint
participant DB as BannedApp Model
User->>UI: Load banned apps page
UI->>User: Render page with search input & map view
User->>UI: Enter search query (country)
UI->>API: GET /api/banned-apps/search/?country=XYZ
API->>DB: Query active banned apps for country
DB-->>API: Return matching apps data
API-->>UI: JSON response with app details
UI->>User: Update list and map with results
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: 2
🧹 Nitpick comments (9)
website/models.py (1)
2299-2331: Consider enhancing the BannedApp model with validators and help_textThe BannedApp model looks well-structured with appropriate fields and Meta configuration. Consider these improvements:
- Add a validator for country_code to ensure it's a valid 2-letter ISO code
- Add help_text to fields to provide context in the admin interface
- Consider using an Enum for app_type choices, similar to other models in this file
class BannedApp(models.Model): APP_TYPES = ( ("social", "Social Media"), ("messaging", "Messaging"), ("gaming", "Gaming"), ("streaming", "Streaming"), ("other", "Other"), ) country_name = models.CharField(max_length=100) - country_code = models.CharField(max_length=2) # ISO 2-letter code + country_code = models.CharField( + max_length=2, + help_text="ISO 3166-1 alpha-2 country code" + ) # ISO 2-letter code app_name = models.CharField(max_length=100) app_type = models.CharField(max_length=20, choices=APP_TYPES) ban_reason = models.TextField() ban_date = models.DateField(default=timezone.now) - source_url = models.URLField(blank=True) + source_url = models.URLField(blank=True, help_text="URL to the source of information about this ban") is_active = models.BooleanField(default=True) created_at = models.DateTimeField(auto_now_add=True) updated_at = models.DateTimeField(auto_now=True)website/views/banned_apps.py (1)
11-20: Consider adding pagination and additional filtering options to the search APIThe search function works correctly, but could be enhanced with pagination and additional filtering capabilities to make it more scalable and flexible.
def search_banned_apps(request): country = request.GET.get("country", "").strip() + app_type = request.GET.get("app_type", "").strip() + page = int(request.GET.get("page", 1)) + page_size = int(request.GET.get("page_size", 10)) if not country: return JsonResponse({"apps": []}) - apps = BannedApp.objects.filter(country_name__icontains=country, is_active=True).values( - "app_name", "app_type", "country_name", "ban_reason", "ban_date", "source_url" - ) + query = BannedApp.objects.filter(country_name__icontains=country, is_active=True) + + if app_type: + query = query.filter(app_type=app_type) + + total = query.count() + start = (page - 1) * page_size + end = page * page_size + + apps = query[start:end].values( + "app_name", "app_type", "country_name", "ban_reason", "ban_date", "source_url" + ) - return JsonResponse({"apps": list(apps)}) + return JsonResponse({ + "apps": list(apps), + "pagination": { + "total": total, + "page": page, + "page_size": page_size, + "total_pages": (total + page_size - 1) // page_size + } + })website/templates/banned_apps.html (7)
9-17: Page Header and Tab Navigation
The header (“Search Banned Apps by Country”) and tab navigation buttons for toggling between List View and Map View are well structured. For enhanced accessibility, consider adding ARIA labels (or appropriate roles) to the buttons so screen readers can better interpret their purpose.
53-56: Inclusion of Leaflet CSS and JS
External Leaflet resources are properly included with thecrossoriginattribute. For extra security and to ensure integrity, consider adding Subresource Integrity (SRI) attributes so that browsers can verify that the files have not been tampered with.
57-73: Tab Switching Event Listener for List View
The event listener attached to the “listViewBtn” correctly reveals the List View and hides the Map View. The DOM manipulation (adding/removing classes) is straightforward. As a minor improvement, you might refactor the duplicated styling toggling into a helper function for better maintainability.
145-162: Marker Position Calculation (calculateMarkerPositions)
The logic to calculate distinct positions for multiple markers around a central point is clear and effective. A brief inline comment explaining the choice ofradius = 0.5(i.e., the degree offset) could further aid future maintainers.
183-202:searchAppsFunction and API Integration
ThesearchAppsfunction checks for an empty string and then fetches data from the backend API endpoint. While logging errors to the console is useful during development, consider providing in-UI feedback for scenarios when the API call fails. This will improve the user experience by informing users of connectivity issues.
246-342: Map Update Function (updateMap)
TheupdateMapfunction effectively groups apps by country, highlights countries on the map, and adds markers (either as a count marker or individual markers) based on the number of apps. The logic is clear, though the function is quite lengthy. Refactoring parts of it into smaller helper functions could improve readability and maintainability. Additionally, consider documenting the rationale behind the threshold (e.g., more than 3 apps) for displaying a count marker versus individual markers.
1-394: General JavaScript Scoping Consideration
Currently, most functions and variables (likemap,countriesLayer, andcurrentCountry) are declared in the global scope. For better encapsulation and to avoid potential conflicts, consider wrapping the entire script in an Immediately Invoked Function Expression (IIFE) or converting it into a module. This will enhance the maintainability and robustness of your code as the project grows.Proposed Diff:
- <script> - // All JS code ... - </script> + <script> + (function() { + // All JS code remains here. + // Variables like map, countriesLayer, etc. will be scoped within this function. + })(); + </script>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
banned_apps.json(1 hunks)blt/urls.py(2 hunks)website/admin.py(2 hunks)website/migrations/0231_bannedapp.py(1 hunks)website/models.py(1 hunks)website/templates/banned_apps.html(1 hunks)website/templates/includes/sidenav.html(1 hunks)website/tests.py(1 hunks)website/views/banned_apps.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run Tests
- GitHub Check: docker-test
- GitHub Check: Analyze (python)
🔇 Additional comments (17)
blt/urls.py (1)
353-354: LGTM! The URL patterns are properly configuredThe new URL patterns for the banned apps feature are well-placed and follow the existing URL structure in the application.
website/migrations/0231_bannedapp.py (1)
1-51: Well-structured migration file for the new BannedApp modelThe migration file correctly defines all necessary fields for the BannedApp model with appropriate types and constraints. The model includes proper indexing on country_name and country_code fields, which is good for query performance when filtering by country. The app_type choices cover the main categories of applications that might be banned.
The Meta options for verbose naming, ordering, and indexing are well-defined. I especially like the use of verbose_name and verbose_name_plural which will improve the admin interface display.
website/templates/includes/sidenav.html (1)
161-169: Navigation item added correctlyThe "Banned Apps" menu item follows the same pattern as other sidebar items, with consistent styling, conditional highlighting, and proper URL template tag. The Font Awesome ban icon (fa-ban) is appropriate for this feature.
The placement in the sidebar looks appropriate, keeping it with other similar information features.
website/tests.py (1)
151-153: Good improvement for test reliabilityAdding an explicit wait for the URL input field using WebDriverWait is an excellent improvement over the previous direct approach. This makes the test more robust against timing issues and will reduce flakiness in your test suite.
This is a best practice for Selenium testing - always wait for elements to be present before interacting with them.
website/admin.py (1)
764-776: Well-configured admin interface for BannedApp modelThe admin configuration is comprehensive with appropriate:
- list_display showing key fields for quick overview
- list_filter for the most filterable fields
- search_fields covering text fields admins might search
- date_hierarchy for ban_date to allow easy date-based navigation
- logical ordering by country and app name
- well-organized fieldsets that group related information
This follows Django admin best practices and will provide a good user experience for administrators.
website/templates/banned_apps.html (12)
1-8: Template Inheritance and Static Files Loading
The template correctly extendsbase.html, loads static assets, and sets a custom title. Including the sidebar navigation with{% include "includes/sidenav.html" %}promotes modularity.
20-25: Search Bar Implementation
The search bar is straightforward and styled with utility classes. Its clear placeholder text improves usability. No issues found here.
26-40: List View Section Structure
The List View section uses hidden containers for displaying either search results or a “no results” message. The structure is clear and the use of utility classes assists in responsive design.
41-50: Map View Section Layout
The Map View is well defined with a dedicated container for the map and an adjacent panel for country details. The separation of concerns between the map and country info is clear.
75-87: Map View Activation and Lazy Map Initialization
The event listener on “mapViewBtn” toggles the view and conditionally initializes the map. Lazy initialization helps conserve resources, which is a positive design choice.
164-175: Debounce Utility Function
The debounce implementation is standard and serves well to throttle frequent input events. This helps improve performance on dynamic searches.
177-182: Search Input Event with Debounce
Attaching a debounced input listener on the search bar is a good performance optimization. The implementation correctly trims and passes the input value to the search handler.
204-215: Results Display withshowResults
The function clears the current grid and appends new result cards generated bycreateAppCard. This logic is concise and effective for dynamically updating the UI.
216-232: Dynamic Card Creation increateAppCard
The card creation using template literals is clear and visually appealing. However, note that inserting variables (likeapp.app_name) directly intoinnerHTMLcan pose XSS risks if the data is not sanitized. Ensure that backend API responses are properly escaped or sanitized.
343-367: Country Info Panel (showCountryInfo)
This function updates the side panel with the country name, count, and a list of banned apps. Similar tocreateAppCard, ensure that the dynamic HTML content is sanitized to prevent potential XSS if any of the app data is user-generated or untrusted.
369-385: Map Reset Functionality (resetMap)
TheresetMapfunction cleanly resets country styles, clears marker layers, and hides the country info panel. This contributes to a smooth user experience when starting a new search.
387-392: DOM Content Loaded and Default View Initialization
Initializing the default view as List View onDOMContentLoadedis a good design choice. It ensures that the page content is structured as expected even before any user interaction, promoting a stable initial state.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
website/models.py (1)
2304-2334: BannedApp model implementation looks good.The model is well-structured with appropriate fields for tracking banned applications by country. The implementation includes:
- Clear choices for app_type categories
- Proper Meta class configuration for ordering and indexing
- Helpful string representation method
Consider adding validation for the country_code field to ensure it follows the ISO 2-letter format. You could use a validator like:
def validate_country_code(value): if not re.match(r'^[A-Z]{2}$', value): raise ValidationError('Country code must be a valid ISO 2-letter code') # Then add to the field: country_code = models.CharField( max_length=2, validators=[validate_country_code], help_text="ISO 2-letter country code" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
blt/urls.py(2 hunks)website/admin.py(2 hunks)website/migrations/0232_bannedapp.py(1 hunks)website/models.py(2 hunks)website/templates/banned_apps.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- blt/urls.py
- website/templates/banned_apps.html
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run Tests
- GitHub Check: docker-test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
website/admin.py (1)
766-778: Well-organized admin configuration for the BannedApp model.The admin configuration for the BannedApp model follows Django best practices with:
- Comprehensive list_display showing the most relevant fields
- Appropriate filters for app_type, is_active, and ban_date
- Useful search fields covering both country and app information
- Logical fieldset organization for better admin form layout
website/migrations/0232_bannedapp.py (1)
7-49: Migration correctly creates the BannedApp model with appropriate fields and configuration.The migration properly defines:
- All required fields with appropriate types
- Configures app_type choices with human-readable labels
- Sets up indexes on country_name and country_code fields that will likely be queried often
- Includes proper Meta options for ordering and verbose names
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
website/templates/banned_apps.html (5)
13-16: Enhance tab navigation accessibility.The tab navigation buttons primarily rely on text color for active/inactive states. Consider adding ARIA attributes (e.g.,
aria-selected) or distinct outlines to improve accessibility for screen readers and color-blind users.
19-25: Consider providing user feedback during searches.While the debounce approach avoids excessive requests, you might also display a “Searching…” indicator or disable the input briefly to convey that a background process is underway, improving user experience on slow networks.
90-143: Confirm caching or retry logic for GeoJSON fetching.Fetching country boundaries from GitHub is handled properly, but there’s only a console log on error. Consider implementing a retry or local fallback if GitHub is unreachable. This ensures the map remains functional for users in case of external network issues.
190-202: Display user-friendly error for search failures.While the
.catch()block logs errors to the console, providing visible error feedback (e.g., a snackbar or alert) will help users understand that a backend or network error occurred instead of simply seeing no results.
246-341: Consider color-blind-friendly styling for highlight colors.Highlighting countries in red (#ef4444) may pose usability challenges for color-blind individuals. Providing alternative styling or contrast improvements could make the map more inclusive.
website/models.py (1)
2304-2335: Validatecountry_codeand date defaults for clarity.While the two-letter limit for
country_codeis straightforward, you might enforce valid ISO 3166 codes via a custom validator or choices list. Also, usingtimezone.nowas a default ban date could mistakenly ban items in the “past.” Ensure that’s intended or prompt manual date input.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
website/models.py(1 hunks)website/templates/banned_apps.html(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run Tests
- GitHub Check: docker-test
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
website/templates/banned_apps.html (3)
8-9: Ensure sidebar discoverability.By including the “sidenav.html” at line 7 and providing a title at line 9, the feature is visible to users, but you might consider adding an explicit menu entry or highlight for “Banned Apps” in the sidebar to improve discoverability.
37-39: Confirm empty-state message for broader search queries.Currently, the “No banned apps found” message only displays if no results are returned for a single country. If users enter partial or incorrect country names, ensure graceful handling of those edge cases, possibly with suggestions or hints.
369-385: Resetting map states is handled cleanly.Your
resetMapfunction effectively clears markers, resets styles, and hides the country info panel. This is a neat approach to ensure a consistent default state.
User description
You have to run this command to populate the database
python manage.py loaddata banned_apps.json
Fixes: #3360
PR Type
Enhancement, Tests
Description
Added a new page to display banned apps by country.
Implemented backend models, views, and APIs for banned apps.
Integrated frontend search functionality with dynamic results display.
Added admin interface and database migration for managing banned apps.
Changes walkthrough 📝
4 files
Added frontend template for banned apps search and displayAdded admin interface for managing banned appsAdded BannedApp model for banned apps dataAdded views for banned apps page and search API1 files
Added routes for banned apps page and API1 files
Added migration for BannedApp model1 files
Added sample data for banned appsSummary by CodeRabbit