Skip to content

Conversation

@joseph-sentry
Copy link
Contributor

we want to be able to call task service methods from the django admin UI so it's easier to debug customer issues

@joseph-sentry joseph-sentry requested a review from a team July 29, 2025 17:26
@codecov
Copy link

codecov bot commented Jul 29, 2025

Codecov Report

❌ Patch coverage is 27.64228% with 89 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.12%. Comparing base (7e8dec1) to head (838fd62).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
apps/codecov-api/core/forms.py 15.85% 69 Missing ⚠️
apps/codecov-api/core/admin.py 51.21% 20 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #362      +/-   ##
==========================================
- Coverage   94.30%   94.12%   -0.18%     
==========================================
  Files        1247     1248       +1     
  Lines       45847    45969     +122     
  Branches     1455     1455              
==========================================
+ Hits        43237    43270      +33     
- Misses       2305     2394      +89     
  Partials      305      305              
Flag Coverage Δ
apiunit 95.94% <27.64%> (-0.49%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@codecov-notifications
Copy link

codecov-notifications bot commented Jul 29, 2025

Codecov Report

❌ Patch coverage is 27.64228% with 89 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
apps/codecov-api/core/forms.py 15.85% 69 Missing ⚠️
apps/codecov-api/core/admin.py 51.21% 20 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Contributor

@ElioDiNino ElioDiNino left a comment

Choose a reason for hiding this comment

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

See compose comments

we want to be able to call task service methods from the django admin UI so it's
easier to debug customer issues
@joseph-sentry joseph-sentry force-pushed the joseph/django-admin-ui branch from a1bc56f to 838fd62 Compare July 29, 2025 18:22
Comment on lines +184 to +187
extra={
"task_method": task_method,
"method_kwargs": method_kwargs,
"error": str(e),
Copy link
Contributor

Choose a reason for hiding this comment

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

Potential bug: Undefined variables `task_method`, `method_kwargs` in `except` block cause `NameError` and admin crash.
  • Description: The submit_task_view function attempts to log task_method and method_kwargs within its except block. If form.call_task_method() on line 170 raises an exception (e.g., a Celery broker connection error or serialization error not caught by call_task_method's internal except block), then task_method and method_kwargs (assigned on lines 171-172) will not have been defined. This leads to a NameError during logging, masking the original issue and causing the admin interface to crash, hindering debugging.
  • Suggested fix: Ensure task_method and method_kwargs are defined before form.call_task_method() or access them directly from form.cleaned_data within the except block.
    severity: 0.95, confidence: 0.98

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Contributor

@ElioDiNino ElioDiNino left a comment

Choose a reason for hiding this comment

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

I notice that the Execute TaskService Method only shows on the admin homepage, but disappears when clicking on any of the available actions. I think it should be added to the sidebar for better navigation if possible

Image Image

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.

3 participants