Skip to content

remediate infrastructure technical debt for #867#1354

Closed
nexpectArpit wants to merge 2 commits into
DalgoT4D:mainfrom
nexpectArpit:cleanup/issue-867-pylint-infrastructure
Closed

remediate infrastructure technical debt for #867#1354
nexpectArpit wants to merge 2 commits into
DalgoT4D:mainfrom
nexpectArpit:cleanup/issue-867-pylint-infrastructure

Conversation

@nexpectArpit

@nexpectArpit nexpectArpit commented May 14, 2026

Copy link
Copy Markdown

Key Changes

1. Configuration & Accuracy (pyproject.toml)

  • Ignored non-application code: Excluded migrations and tests directories from Pylint tracking to filter out machine-generated noise and intentional test boilerplate.
  • Enabled pylint-django: Integrated the Django plugin to resolve hundreds of false-positive no-member errors on dynamic Django model properties (e.g., Model.objects).
  • Enforcement: Raised the fail-under threshold from 6.5 to 8.5.

2. Infrastructure Cleanup

  • Dead Code Removal: Removed unused imports (e.g., shutil, slugify, GitManager) and redundant local variables across tasks.py, unified_logger.py, and moretasks.py.
  • API Simplification: Removed unused author_email and org arguments from internal methods in email_templates.py and mention_service.py, updating all call sites and tests to match.
  • Celery Task Binding: Replaced unused self arguments with _ in @app.task(bind=True) functions to resolve unused-argument warnings while maintaining valid signatures.

3. Object-Oriented Refinement

  • Protected Access Fix: Renamed internal URL builders (_build_private_url, _build_public_url) in ReportService to public methods, resolving W0212 (protected-access) violations triggered by external Celery workers.

addresses #867

Summary by CodeRabbit

  • Refactor

    • Simplified notification service API by reducing required parameters
    • Promoted internal URL-building methods to public API for report sharing functionality
    • Streamlined email template handling for mention notifications
    • Cleaned up unused imports and dependencies throughout codebase
  • Chores

    • Updated code quality standards and linting configuration

Review Change Stack

@Ishankoradia @siddhant3030 @fatchat please review it

@coderabbitai

coderabbitai Bot commented May 14, 2026

Copy link
Copy Markdown

Walkthrough

This PR refactors ReportService URL-builder methods to be public API instead of private, updates dependent Celery tasks and test mocks to use the new public methods, simplifies email template signatures, and performs general code cleanup across imports and Pylint configuration.

Changes

Report URL Methods Public Refactoring and Email Service Updates

Layer / File(s) Summary
ReportService URL methods visibility refactoring
ddpui/core/reports/report_service.py
ReportService static methods _build_public_url() and _build_private_url() are renamed to remove the leading underscore, making them public API methods while maintaining the same URL construction logic.
Update service call sites for public URL methods
ddpui/core/reports/report_service.py
Two methods in ReportService that generate share responses now call build_public_url() instead of the former _build_public_url() for constructing public share links.
Celery email task uses public URL methods
ddpui/celeryworkers/report_tasks.py
The send_report_email_task Celery task is updated to call ReportService.build_private_url() and ReportService.build_public_url() (public methods) instead of the private underscore-prefixed variants, and the bound task parameter is renamed from self to _.
Test updates for public URL method patches
ddpui/tests/core/reports/test_share_email.py
Unit tests in test_share_email.py are updated to patch ReportService.build_private_url and ReportService.build_public_url (public methods) instead of the former private underscore-prefixed methods across five test cases.
Email template signature simplification
ddpui/utils/email_templates.py
The render_mention_email() function signature is simplified by removing the author_email parameter, and docstring is updated accordingly. The render_share_report_email() plain-text concatenation is adjusted to use non-f-string literals while preserving content.

Code Cleanup and Configuration

Layer / File(s) Summary
Celery task parameter and import cleanup
ddpui/celeryworkers/moretasks.py, ddpui/celeryworkers/tasks.py
The schedule_notification_task in moretasks.py has its bound parameter renamed from self to _, unused import os is removed, and tasks.py imports are reorganized to remove unused internal imports (OrgDbt, TransformType, create_or_update_org_cli_block, GitManager, GitManagerError, DBTCLIPROFILE) while adding necessary ones like subprocess.
Logger imports and Pylint configuration
ddpui/utils/unified_logger.py, pyproject.toml
Unused imports (uuid, Optional, Any) are removed from unified_logger.py and typing imports are narrowed to only Dict. Pylint configuration in pyproject.toml is extended with [tool.pylint.master] section, fail-under threshold is raised from 6.5 to 8.5, and additional disabled message types are configured.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • DalgoT4D/DDP_backend#1292: Both PRs touch the same share-via-email implementation points—ddpui/celeryworkers/report_tasks.py's send_report_email_task and ddpui/core/reports/report_service.py URL-builder methods—so the main PR's changes are directly related to the retrieved PR's feature work.

Suggested reviewers

  • himanshudube97
  • siddhant3030
  • pradnk
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 64.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main infrastructure and technical debt remediation work across the codebase addressing issue #867, including linting configuration, unused import cleanup, and method accessibility changes.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

- Configure pylint-django and ignore migrations
- Remove unused imports and local variables
- Resolve protected access warnings in report services
- Simplify email template signatures
@nexpectArpit nexpectArpit force-pushed the cleanup/issue-867-pylint-infrastructure branch from 9f3e6a0 to b967047 Compare May 14, 2026 23:07
@nexpectArpit nexpectArpit marked this pull request as ready for review May 14, 2026 23:11

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
ddpui/utils/email_templates.py (1)

70-70: ⚡ Quick win

Update docstring to remove unused author_email field.

The thread item structure documents author_email, but the rendering functions (_render_thread_html at line 24 and _render_thread_plain at line 50) only use author_name and content. Since this PR removes unused author_email references, update the docstring to reflect the actual fields used.

📝 Proposed docstring fix
         thread: Optional list of prior comments for context.
-                Each item: {"author_name": str, "author_email": str, "content": str}
+                Each item: {"author_name": str, "content": str}
         chart_name: Optional chart title when comment is on a specific chart
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ddpui/utils/email_templates.py` at line 70, The docstring for the thread item
list still lists an unused "author_email" field; update the docstring in
ddpui/utils/email_templates.py to remove "author_email" so it matches the actual
structure used by the rendering functions _render_thread_html and
_render_thread_plain (which only reference "author_name" and "content"); ensure
the example/description now reads Each item: {"author_name": str, "content":
str} and adjust any surrounding wording to remain accurate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pyproject.toml`:
- Around line 288-304: Move the "fail-under" option out of the
[tool.pylint.messages_control] section and place it into the
[tool.pylint.master] section alongside "ignore" and "load-plugins" so pylint
sees it as a main option; specifically, remove the line fail-under = 8.5 from
the messages_control block and add fail-under = 8.5 under the master block that
contains ignore and load-plugins.

---

Nitpick comments:
In `@ddpui/utils/email_templates.py`:
- Line 70: The docstring for the thread item list still lists an unused
"author_email" field; update the docstring in ddpui/utils/email_templates.py to
remove "author_email" so it matches the actual structure used by the rendering
functions _render_thread_html and _render_thread_plain (which only reference
"author_name" and "content"); ensure the example/description now reads Each
item: {"author_name": str, "content": str} and adjust any surrounding wording to
remain accurate.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3eccbd15-3c25-4b96-8063-4510df7e3e89

📥 Commits

Reviewing files that changed from the base of the PR and between 75cb6fd and b967047.

📒 Files selected for processing (10)
  • ddpui/celeryworkers/moretasks.py
  • ddpui/celeryworkers/report_tasks.py
  • ddpui/celeryworkers/tasks.py
  • ddpui/core/reports/mention_service.py
  • ddpui/core/reports/report_service.py
  • ddpui/tests/core/reports/test_mention_notifications.py
  • ddpui/tests/core/reports/test_share_email.py
  • ddpui/utils/email_templates.py
  • ddpui/utils/unified_logger.py
  • pyproject.toml
💤 Files with no reviewable changes (2)
  • ddpui/tests/core/reports/test_mention_notifications.py
  • ddpui/core/reports/mention_service.py

Comment thread pyproject.toml
@nexpectArpit

Copy link
Copy Markdown
Author

@Ishankoradia May you please tell the reason for closure of this PR! . thanks

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.

2 participants