-
-
Notifications
You must be signed in to change notification settings - Fork 282
Move banned apps into its own repository #4899
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
base: main
Are you sure you want to change the base?
Conversation
|
👋 Hi @Nachiket-Roy! This pull request needs a peer review before it can be merged. Please request a review from a team member who is not:
Once a valid peer review is submitted, this check will pass automatically. Thank you! |
WalkthroughMoved banned apps out of the main repository into an external package: removed local BannedApp model, admin and migration; added external git dependency and app to INSTALLED_APPS; updated imports and URLs to use the external package; added a JSON fixture with banned app records. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant MainApp as "Main Django app"
participant BannedPkg as "banned_apps (external)"
Browser->>MainApp: GET /banned_apps/ or /api/banned_apps/
MainApp->>MainApp: resolve included URLconf
MainApp->>BannedPkg: forward request to included routes
BannedPkg->>BannedPkg: view handles request, queries `banned_apps.models.BannedApp`
BannedPkg-->>Browser: Response (HTML/JSON)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Knowledge base: Disabled due to 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
blt/urls.py (1)
376-377: Remove duplicate URL patterns or clarify the migration strategy for the external package.The codebase now has conflicting URL configurations for banned_apps:
- Lines 376-377: Local URL patterns (
banned_apps/andapi/banned_apps/search/) that import views fromwebsite.views.banned_apps- Line 1129: External package include statement (
path("banned_apps/", include("banned_apps.urls")))Both define the same base path
banned_apps/, creating routing ambiguity. Additionally, the local views already import the model from the external package (from banned_apps.models import BannedApp), indicating partial migration to the external package.Either:
- Remove the local URL patterns (lines 376-377) and the local views file to fully migrate to the external package, OR
- Remove the external package include (line 1129) if keeping local implementation
Clarify which approach is intended and ensure no duplicate route handlers remain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
banned_apps_fixture.json(1 hunks)blt/settings.py(1 hunks)blt/urls.py(1 hunks)pyproject.toml(1 hunks)website/admin.py(0 hunks)website/migrations/0232_bannedapp.py(0 hunks)website/migrations/0233_githubissue_assignee.py(1 hunks)website/models.py(0 hunks)website/views/banned_apps.py(1 hunks)
💤 Files with no reviewable changes (3)
- website/models.py
- website/admin.py
- website/migrations/0232_bannedapp.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (3)
blt/settings.py (1)
105-105: LGTM!The banned_apps app is correctly added to INSTALLED_APPS, enabling Django to load the external package's models, migrations, and URL configuration.
website/migrations/0233_githubissue_assignee.py (1)
9-9: Migration dependency correctly updated and verified.The dependency change in migration 0233 from
0232_bannedappto0231_organization_check_ins_enabled_notificationis correct. Migration 0231 exists and is directly referenced, the 0232_bannedapp migration has been removed from the codebase, and the migration chain (0230 → 0231 → 0233 → 0234) is intact with no broken dependencies.website/views/banned_apps.py (1)
1-1: The filewebsite/views/banned_apps.pyhas not been modified in this PR.The git diff shows no changes to this file. The import statement
from banned_apps.models import BannedAppwas already present in HEAD and remains unchanged. The review comment appears to be analyzing existing code rather than actual PR changes. No verification or approval is needed for unmodified code.Likely an incorrect or invalid review comment.
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
blt/urls.py(2 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (1)
blt/urls.py (1)
15-15: LGTM!Import consolidation improves readability by grouping related generic views together.
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
blt/urls.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (1)
blt/urls.py (1)
15-15: LGTM: Import consolidation is appropriate.The addition of
RedirectViewto this import statement is correct, as the class is already used elsewhere in the file (e.g., line 355 for the favicon redirect).
Closes #4874
Summary
This PR completes the migration of the banned apps functionality out of the main BLT repository into a dedicated standalone Django app:
banned-apps GitHub repo (installed via Poetry).
This improves modularity, maintenance, and clarity of ownership.
Key Changes
Removed old in-project implementation
Deleted website.models.BannedApp
Removed old imports such as from website.models import BannedApp
Cleaned up unused views, admin registrations, and any references pointing to the old model
Integrated new standalone package
Added Poetry dependency:
banned-apps = { git = "https://github.com/Nachiket-Roy/banned-apps.git" }Ensured the package loads correctly inside Docker
Applied new migrations:
Used pre-existing BLT seed data.
Added:
banned_apps_fixture.json
(contains 15 banned apps from India, China, Russia, Iran, USA)
Loaded via:
Updated imports:
python -c "import banned_apps; print(banned_apps.__file__)" python manage.py showmigrations banned_appsAPI Endpoint working:
/banned_apps/search/?country=india
Note : Only country-based search is supported for now.
Summary by CodeRabbit
Refactor
Data
Chores