Add EOI pages and solicitation functionality #741
Conversation
jjackson
force-pushed
the
jj/EOI-pages
branch
from
f4849ce to
4ef664a
Compare
WalkthroughThis pull request introduces three major new features to CommCare Connect: (1) a Solicitations app for managing Expressions of Interest (EOIs) and Request for Proposals (RFPs) with questions, responses, reviews, and attachments; (2) an Audit app for conducting audits with templates, data extraction from CommCare HQ via APIs and Superset, result tracking, and assessments; (3) a Labs OAuth authentication system providing session-based login without database persistence. Supporting changes include a new GitHub Actions workflow for AWS Fargate deployment, field constraints on existing models, database migrations, extensive fixtures and sample data, management commands for data loading and setup, comprehensive test suites, documentation, and configuration updates. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 31
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
commcare_connect/contrib/sites/migrations/0003_set_site_domain_and_name.py (1)
19-30: Remove redundant SQLite check.The outer condition on line 19 already gates entry to the sequence sync block with
connection.vendor != "sqlite", meaning the block is only entered when the vendor is NOT SQLite. The inner check on lines 28-30 testing for SQLite will never evaluate to true and is unreachable dead code.Apply this diff to remove the redundant inner check:
- if created and connection.vendor != "sqlite": + if created and connection.vendor != "sqlite": # We provided the ID explicitly when creating the Site entry, therefore the DB # sequence to auto-generate them wasn't used and is now out of sync. If we # don't do anything, we'll get a unique constraint violation the next time a # site is created. # To avoid this, we need to manually update DB sequence and make sure it's # greater than the maximum value. max_id = site_model.objects.order_by('-id').first().id with connection.cursor() as cursor: - # Skip sequence adjustment on SQLite where sequences are not used - if connection.vendor == "sqlite": - return cursor.execute("SELECT last_value from django_site_id_seq") (current_id,) = cursor.fetchone() if current_id <= max_id:
🧹 Nitpick comments (17)
commcare_connect/solicitations/instructions/rename-progress-summary.md (2)
1-1: Update the document date and consider archival or integration into main documentation.This progress summary is dated 8/20/25 but we're now in November 2025—approximately 2.5 months have passed. The document should either be updated to reflect current status (particularly Phase 4 outcomes if work has progressed) or archived with a clear note about its historical scope.
- Has Phase 4 (Testing & Documentation) been completed? If so, create a Phase 4 summary or update this document.
- Should this file be moved to a historical archive or integrated into main project documentation (e.g.,
docs/architecture/solicitations.md)?
124-140: Consider tracking Phase 4 work in a separate, linked document or GitHub issues.Phase 4 is currently outlined as unchecked tasks in this static document. For an active project, tracking these items in GitHub issues or a living roadmap document will make it easier for the team to iterate and stay aligned.
demo_decorator.py (2)
17-28: Addif __name__ == "__main__":guard for script execution.The print statements and function calls execute immediately when the module is imported, causing side effects. Wrap the execution code in a main guard:
+if __name__ == "__main__": -print("1. Defining the decorated function...") + print("1. Defining the decorated function...") -@my_decorator -def hello(name): - print(f"Hello, {name}!") - return f"Greeting for {name}" + @my_decorator + def hello(name): + print(f"Hello, {name}!") + return f"Greeting for {name}" -print("2. Calling the decorated function...") -result = hello("World") -print(f"3. Result: {result}") + print("2. Calling the decorated function...") + result = hello("World") + print(f"3. Result: {result}")
1-1: Make file executable or remove shebang.The shebang is present but the file lacks executable permissions. If this file is meant to be executed directly, add executable permissions; otherwise, remove the shebang.
#!/bin/bash # Add executable permissions if this is intended as a standalone script chmod +x demo_decorator.pycommcare_connect/solicitations/instructions/labs.md (1)
11-30: Add language identifier to fenced code block.The ASCII diagram in the fenced code block should specify a language identifier for proper markdown rendering. Using
textorplaintextwould be appropriate for ASCII art.Apply this diff:
-``` +```text ┌─────────────────────┐ ┌─────────────────────┐ │ connect.dimagi.com │ │ labs.connect.dimagi │ │ │ │ │commcare_connect/static/js/datetime-utils.js (2)
7-19: Add error handling for invalid dates.The code parses
isoDateTimewithout validating the resulting Date object. Invalid datetime strings will create an Invalid Date object, andtoLocaleString()will return "Invalid Date" as the text content.Apply this diff to add validation:
var isoDateTime = element.getAttribute('datetime'); if (isoDateTime) { var date = new Date(isoDateTime); + if (isNaN(date.getTime())) { + continue; // Skip invalid dates + } var localTime = date.toLocaleString('en-US', { year: 'numeric',
10-17: Consider using navigator.language for locale.The locale is hardcoded to
'en-US', which may not match the user's preferred locale. While the comment mentions "user's local timezone" (which is correctly applied via toLocaleString), the format is always US-style.If internationalization is a concern, consider:
- var localTime = date.toLocaleString('en-US', { + var localTime = date.toLocaleString(navigator.language || 'en-US', { year: 'numeric',commcare_connect/solicitations/instructions/user-organization-program-relationships.md (1)
9-11: Add language identifier to fenced code block.The static analysis tool suggests adding a language identifier to the fenced code block. Since this is ASCII art representing relationships, consider using
textas the language identifier.Apply this diff:
-``` +```text User ←→ UserOrganizationMembership ←→ Organization ←→ Program</blockquote></details> <details> <summary>commcare_connect/solicitations/instructions/implement-plan.md (1)</summary><blockquote> `122-130`: **Add language specifiers to code blocks.** Multiple code blocks throughout this file lack language specifiers (lines 122, 172, 188, 204, 219, 263, 273, 283), which degrades documentation readability and syntax highlighting. Apply language specifiers to improve rendering: ```diff -``` +```text Program Manager creates EOI → Organizations submit responses ↓ ...commcare_connect/solicitations/management/commands/test_dashboard_links.py (1)
44-46: Consider using spread operator for list concatenation.While the current concatenation works, using the spread operator is more idiomatic in modern Python.
Apply this diff:
- settings.ALLOWED_HOSTS = current_allowed_hosts + ["testserver"] + settings.ALLOWED_HOSTS = [*current_allowed_hosts, "testserver"]commcare_connect/solicitations/tests/test_helpers.py (1)
150-165: Consider using_for intentionally unused variable.The static analysis tool correctly identifies that the
errorvariable at line 155 is never used. Since the test intentionally ignores the error return value when verifying successful question creation, consider using_to make this intention explicit.Apply this diff to clarify the intent:
success, error = process_solicitation_questions(questions_json, self.solicitation) # Should still succeed but create question with empty text assert success is True assert self.solicitation.questions.count() == 1could become:
- success, error = process_solicitation_questions(questions_json, self.solicitation) + success, _ = process_solicitation_questions(questions_json, self.solicitation) # Should still succeed but create question with empty text assert success is True assert self.solicitation.questions.count() == 1commcare_connect/templates/solicitations/response_form.html (1)
382-390: Consider using CSS classes instead of inline styles for validation feedback.The current approach directly manipulates inline styles for validation feedback. Using CSS classes would be more maintainable and align better with the existing Tailwind-style approach.
Consider refactoring to use CSS classes:
requiredFields.forEach(field => { if (!field.value.trim()) { hasEmptyRequired = true; - field.style.borderColor = '#ef4444'; - field.style.boxShadow = '0 0 0 1px #ef4444'; + field.classList.add('border-red-500', 'ring-1', 'ring-red-500'); } else { - field.style.borderColor = ''; - field.style.boxShadow = ''; + field.classList.remove('border-red-500', 'ring-1', 'ring-red-500'); } });commcare_connect/solicitations/helpers.py (1)
171-174: Consider optimizing total_responses calculation.The current implementation annotates each solicitation and then sums the results in Python, which evaluates the entire queryset in memory. For large datasets, this could be inefficient.
Consider using database aggregation:
- # Calculate total responses across all solicitations - annotated_solicitations = solicitations_queryset.annotate( - total_responses=Count("responses", filter=~Q(responses__status=SolicitationResponse.Status.DRAFT)) - ) - stats["total_responses"] = sum(s.total_responses or 0 for s in annotated_solicitations) + # Calculate total responses across all solicitations using database aggregation + from django.db.models import Sum + total_responses = solicitations_queryset.aggregate( + total=Count("responses", filter=~Q(responses__status=SolicitationResponse.Status.DRAFT)) + ) + stats["total_responses"] = total_responses["total"] or 0commcare_connect/solicitations/views.py (4)
401-418: Consider wrapping solicitation save and question processing in a transaction.If question processing fails after the solicitation is saved, you'll have a solicitation without questions. While the user can edit to add questions later, it's better to ensure atomicity.
+ from django.db import transaction + # Save the solicitation first - response = super().form_valid(form) - - # Process questions using appropriate helper - questions_data = self.request.POST.get("questions_data") - if is_edit: - success, error_message = update_solicitation_questions(questions_data, self.object) - else: - success, error_message = process_solicitation_questions(questions_data, self.object) - - if not success: - if is_edit: - messages.warning(self.request, f"Questions could not be saved: {error_message}. Please try again.") - else: - messages.warning( - self.request, - f"Questions could not be saved: {error_message}. Please edit the solicitation to add questions.", - ) + with transaction.atomic(): + response = super().form_valid(form) + + # Process questions using appropriate helper + questions_data = self.request.POST.get("questions_data") + if is_edit: + success, error_message = update_solicitation_questions(questions_data, self.object) + else: + success, error_message = process_solicitation_questions(questions_data, self.object) + + if not success: + if is_edit: + messages.warning(self.request, f"Questions could not be saved: {error_message}. Please try again.") + else: + messages.warning( + self.request, + f"Questions could not be saved: {error_message}. Please edit the solicitation to add questions.", + ) + # Transaction will rollback, preventing inconsistent state + raise ValidationError(error_message)
983-988: Prefix unused variable with underscore.The
createdvariable fromget_or_createis never used. Following Python conventions, prefix it with an underscore to indicate it's intentionally unused.# Get or create draft - draft, created = SolicitationResponse.objects.get_or_create( + draft, _created = SolicitationResponse.objects.get_or_create( solicitation=solicitation, organization=user_org, status=SolicitationResponse.Status.DRAFT, defaults={"submitted_by": request.user, "responses": {}}, )
1012-1017: Prefix unused variable with underscore.The
createdvariable fromget_or_createis never used. Following Python conventions, prefix it with an underscore to indicate it's intentionally unused.# Get or create draft response - draft, created = SolicitationResponse.objects.get_or_create( + draft, _created = SolicitationResponse.objects.get_or_create( solicitation=solicitation, organization=user_org, status=SolicitationResponse.Status.DRAFT, defaults={"submitted_by": request.user, "responses": {}}, )
1115-1115: Useis not Noneinstead of!= Nonefor consistency.While the existing comparison works, using
is not Noneis more Pythonic and consistent with other null checks in the codebase.- return self.request.org_membership != None or self.request.user.is_superuser # noqa: E711 + return self.request.org_membership is not None or self.request.user.is_superuser
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (71)
.github/workflows/deploy-labs.yml(1 hunks)QUICK_START_GUIDE.md(1 hunks)commcare_connect/contrib/sites/migrations/0003_set_site_domain_and_name.py(2 hunks)commcare_connect/opportunity/migrations/0030_uservisit_location.py(1 hunks)commcare_connect/opportunity/migrations/0083_alter_uservisit_location.py(1 hunks)commcare_connect/opportunity/models.py(1 hunks)commcare_connect/program/migrations/0001_initial.py(1 hunks)commcare_connect/program/migrations/0002_alter_program_description.py(1 hunks)commcare_connect/program/models.py(1 hunks)commcare_connect/reports/views.py(1 hunks)commcare_connect/solicitations/admin.py(1 hunks)commcare_connect/solicitations/apps.py(1 hunks)commcare_connect/solicitations/fixtures/sample_solicitations.yaml(1 hunks)commcare_connect/solicitations/forms.py(1 hunks)commcare_connect/solicitations/helpers.py(1 hunks)commcare_connect/solicitations/instructions/Access-Security-Overview.md(1 hunks)commcare_connect/solicitations/instructions/JJ_questions.md(1 hunks)commcare_connect/solicitations/instructions/QA-prompt.md(1 hunks)commcare_connect/solicitations/instructions/implement-plan.md(1 hunks)commcare_connect/solicitations/instructions/labs.md(1 hunks)commcare_connect/solicitations/instructions/rename-progress-summary.md(1 hunks)commcare_connect/solicitations/instructions/review.MD(1 hunks)commcare_connect/solicitations/instructions/table_review.md(1 hunks)commcare_connect/solicitations/instructions/user-organization-program-relationships.md(1 hunks)commcare_connect/solicitations/management/commands/load_solicitation_fixtures.py(1 hunks)commcare_connect/solicitations/management/commands/load_uat_data.py(1 hunks)commcare_connect/solicitations/management/commands/test_dashboard_links.py(1 hunks)commcare_connect/solicitations/migrations/0001_initial.py(1 hunks)commcare_connect/solicitations/migrations/0002_alter_solicitationresponse_status.py(1 hunks)commcare_connect/solicitations/migrations/0003_alter_solicitationresponse_responses.py(1 hunks)commcare_connect/solicitations/migrations/0004_add_response_attachment_model.py(1 hunks)commcare_connect/solicitations/migrations/0005_remove_solicitationresponse_attachments_and_more.py(1 hunks)commcare_connect/solicitations/migrations/0006_remove_eoi_rfp_relationships.py(1 hunks)commcare_connect/solicitations/migrations/0007_remove_completed_status.py(1 hunks)commcare_connect/solicitations/migrations/0008_simplify_response_status.py(1 hunks)commcare_connect/solicitations/models.py(1 hunks)commcare_connect/solicitations/tables.py(1 hunks)commcare_connect/solicitations/tests/factories.py(1 hunks)commcare_connect/solicitations/tests/test_authenticated_views.py(1 hunks)commcare_connect/solicitations/tests/test_forms.py(1 hunks)commcare_connect/solicitations/tests/test_helpers.py(1 hunks)commcare_connect/solicitations/tests/test_models.py(1 hunks)commcare_connect/solicitations/tests/test_permissions.py(1 hunks)commcare_connect/solicitations/tests/test_views.py(1 hunks)commcare_connect/solicitations/urls.py(1 hunks)commcare_connect/solicitations/views.py(1 hunks)commcare_connect/static/js/datetime-utils.js(1 hunks)commcare_connect/templates/solicitations/components/file_attachments.html(1 hunks)commcare_connect/templates/solicitations/components/organization_overview.html(1 hunks)commcare_connect/templates/solicitations/components/response_answers.html(1 hunks)commcare_connect/templates/solicitations/components/stats_card.html(1 hunks)commcare_connect/templates/solicitations/components/table_section.html(1 hunks)commcare_connect/templates/solicitations/draft_list.html(1 hunks)commcare_connect/templates/solicitations/organization_required.html(1 hunks)commcare_connect/templates/solicitations/public_base.html(1 hunks)commcare_connect/templates/solicitations/response_detail.html(1 hunks)commcare_connect/templates/solicitations/response_form.html(1 hunks)commcare_connect/templates/solicitations/response_list.html(1 hunks)commcare_connect/templates/solicitations/response_review.html(1 hunks)commcare_connect/templates/solicitations/response_success.html(1 hunks)commcare_connect/templates/solicitations/review_form.html(1 hunks)commcare_connect/templates/solicitations/solicitation_detail.html(1 hunks)commcare_connect/templates/solicitations/solicitation_form.html(1 hunks)commcare_connect/templates/solicitations/solicitation_list.html(1 hunks)commcare_connect/templates/solicitations/unified_dashboard.html(1 hunks)config/settings/base.py(1 hunks)config/urls.py(1 hunks)demo_decorator.py(1 hunks)requirements/base.txt(3 hunks)requirements/production.txt(4 hunks)tatus(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (27)
commcare_connect/solicitations/tests/test_helpers.py (4)
commcare_connect/solicitations/helpers.py (3)
calculate_response_permissions(30-88)get_solicitation_dashboard_statistics(146-176)process_solicitation_questions(179-214)commcare_connect/solicitations/models.py (4)
Solicitation(10-74)SolicitationResponse(106-149)Status(19-22)Status(111-113)commcare_connect/solicitations/tests/factories.py (3)
SolicitationFactory(69-113)SolicitationResponseFactory(146-160)questions(167-188)commcare_connect/conftest.py (2)
user(50-51)organization(45-46)
commcare_connect/solicitations/tests/test_authenticated_views.py (6)
commcare_connect/program/tests/factories.py (1)
ProgramFactory(9-20)commcare_connect/solicitations/models.py (8)
Solicitation(10-74)SolicitationResponse(106-149)SolicitationReview(182-205)Type(15-17)Type(82-87)Status(19-22)Status(111-113)Recommendation(187-190)commcare_connect/solicitations/tests/factories.py (7)
SolicitationFactory(69-113)SolicitationResponseFactory(146-160)SolicitationReviewFactory(225-242)SolicitationWithQuestionsFactory(163-188)title(86-89)description(92-96)questions(167-188)commcare_connect/users/tests/factories.py (2)
OrganizationFactory(48-52)UserFactory(10-26)commcare_connect/conftest.py (2)
user(50-51)organization(45-46)commcare_connect/solicitations/forms.py (2)
save(150-173)save(326-343)
commcare_connect/solicitations/migrations/0004_add_response_attachment_model.py (2)
commcare_connect/solicitations/migrations/0001_initial.py (1)
Migration(8-277)commcare_connect/solicitations/migrations/0003_alter_solicitationresponse_responses.py (1)
Migration(6-17)
commcare_connect/solicitations/tests/test_views.py (2)
commcare_connect/solicitations/models.py (5)
Solicitation(10-74)Status(19-22)Status(111-113)Type(15-17)Type(82-87)commcare_connect/solicitations/tests/factories.py (6)
EOIFactory(116-119)RFPFactory(122-125)SolicitationFactory(69-113)title(86-89)description(92-96)scope_of_work(103-108)
commcare_connect/solicitations/migrations/0008_simplify_response_status.py (2)
commcare_connect/solicitations/models.py (1)
SolicitationResponse(106-149)commcare_connect/solicitations/migrations/0007_remove_completed_status.py (1)
Migration(6-22)
commcare_connect/solicitations/tables.py (3)
commcare_connect/solicitations/models.py (9)
Solicitation(10-74)SolicitationResponse(106-149)Meta(61-67)Meta(98-100)Meta(124-130)Meta(164-165)Meta(200-202)Status(19-22)Status(111-113)commcare_connect/utils/tables.py (1)
OrgContextTable(26-29)commcare_connect/organization/models.py (1)
is_admin(48-49)
commcare_connect/solicitations/tests/test_models.py (3)
commcare_connect/solicitations/models.py (9)
Solicitation(10-74)SolicitationQuestion(77-103)SolicitationResponse(106-149)Status(19-22)Status(111-113)can_accept_responses(73-74)is_draft(136-138)is_submitted(141-143)submit(145-149)commcare_connect/solicitations/tests/factories.py (4)
SolicitationFactory(69-113)SolicitationResponseFactory(146-160)questions(167-188)responses(195-222)commcare_connect/conftest.py (2)
user(50-51)organization(45-46)
commcare_connect/solicitations/migrations/0003_alter_solicitationresponse_responses.py (2)
commcare_connect/solicitations/migrations/0001_initial.py (1)
Migration(8-277)commcare_connect/solicitations/migrations/0004_add_response_attachment_model.py (1)
Migration(8-50)
commcare_connect/solicitations/tests/factories.py (3)
commcare_connect/program/tests/factories.py (1)
ProgramFactory(9-20)commcare_connect/solicitations/models.py (14)
Solicitation(10-74)SolicitationQuestion(77-103)SolicitationResponse(106-149)SolicitationReview(182-205)Type(15-17)Type(82-87)Meta(61-67)Meta(98-100)Meta(124-130)Meta(164-165)Meta(200-202)Status(19-22)Status(111-113)Recommendation(187-190)commcare_connect/users/tests/factories.py (2)
OrganizationFactory(48-52)UserFactory(10-26)
commcare_connect/solicitations/tests/test_forms.py (6)
commcare_connect/program/tests/factories.py (1)
ProgramFactory(9-20)commcare_connect/solicitations/forms.py (4)
SolicitationForm(176-343)SolicitationResponseForm(12-173)save(150-173)save(326-343)commcare_connect/solicitations/models.py (6)
Solicitation(10-74)SolicitationQuestion(77-103)Type(15-17)Type(82-87)Status(19-22)Status(111-113)commcare_connect/solicitations/tests/factories.py (6)
SolicitationWithQuestionsFactory(163-188)title(86-89)target_population(99-100)scope_of_work(103-108)questions(167-188)responses(195-222)commcare_connect/users/tests/factories.py (1)
UserFactory(10-26)commcare_connect/conftest.py (2)
user(50-51)organization(45-46)
commcare_connect/solicitations/management/commands/load_solicitation_fixtures.py (5)
commcare_connect/opportunity/models.py (1)
DeliveryType(53-59)commcare_connect/organization/models.py (3)
Organization(11-25)UserOrganizationMembership(28-62)Role(29-32)commcare_connect/program/models.py (1)
Program(9-26)commcare_connect/solicitations/models.py (3)
Solicitation(10-74)SolicitationQuestion(77-103)delete(175-179)commcare_connect/users/models.py (1)
User(13-64)
commcare_connect/solicitations/models.py (5)
commcare_connect/organization/models.py (1)
Organization(11-25)commcare_connect/program/models.py (1)
Program(9-26)commcare_connect/users/models.py (1)
User(13-64)commcare_connect/solicitations/tests/factories.py (9)
description(92-96)target_population(99-100)scope_of_work(103-108)estimated_scale(111-113)Meta(72-73)Meta(129-130)Meta(147-148)Meta(228-229)responses(195-222)commcare_connect/solicitations/forms.py (4)
Meta(181-257)Meta(351-353)save(150-173)save(326-343)
commcare_connect/solicitations/migrations/0007_remove_completed_status.py (1)
commcare_connect/solicitations/migrations/0008_simplify_response_status.py (1)
Migration(22-38)
commcare_connect/program/migrations/0002_alter_program_description.py (1)
commcare_connect/program/migrations/0001_initial.py (1)
Migration(7-99)
commcare_connect/opportunity/migrations/0083_alter_uservisit_location.py (1)
commcare_connect/opportunity/migrations/0030_uservisit_location.py (1)
Migration(6-17)
commcare_connect/solicitations/helpers.py (1)
commcare_connect/solicitations/models.py (9)
Solicitation(10-74)SolicitationQuestion(77-103)SolicitationResponse(106-149)can_accept_responses(73-74)Status(19-22)Status(111-113)Type(15-17)Type(82-87)delete(175-179)
commcare_connect/reports/views.py (2)
commcare_connect/solicitations/views.py (4)
test_func(44-46)test_func(57-63)test_func(76-106)test_func(1100-1115)commcare_connect/program/views.py (1)
test_func(35-40)
commcare_connect/solicitations/urls.py (1)
commcare_connect/solicitations/views.py (13)
SolicitationCreateOrUpdate(296-431)SolicitationDetailView(217-293)SolicitationListView(171-214)SolicitationResponseCreateOrUpdate(586-811)SolicitationResponseDetailView(538-583)SolicitationResponseDraftListView(814-828)SolicitationResponseReviewCreateOrUpdate(834-968)SolicitationResponseSuccessView(522-535)SolicitationResponseTableView(438-519)UnifiedSolicitationDashboard(1084-1415)delete_attachment(1056-1076)save_draft_ajax(973-998)upload_attachment(1003-1051)
commcare_connect/solicitations/views.py (5)
commcare_connect/program/models.py (1)
Program(9-26)commcare_connect/solicitations/forms.py (5)
SolicitationForm(176-343)SolicitationResponseForm(12-173)SolicitationReviewForm(346-414)save(150-173)save(326-343)commcare_connect/solicitations/helpers.py (5)
build_question_context(217-246)calculate_response_permissions(30-88)get_solicitation_dashboard_statistics(146-176)process_solicitation_questions(179-214)update_solicitation_questions(249-288)commcare_connect/solicitations/models.py (9)
ResponseAttachment(152-179)Solicitation(10-74)SolicitationQuestion(77-103)SolicitationResponse(106-149)SolicitationReview(182-205)Status(19-22)Status(111-113)can_accept_responses(73-74)delete(175-179)commcare_connect/solicitations/tables.py (3)
ProgramTable(64-125)SolicitationResponseAndReviewTable(207-342)SolicitationTable(131-204)
commcare_connect/solicitations/admin.py (1)
commcare_connect/solicitations/models.py (4)
Solicitation(10-74)SolicitationQuestion(77-103)SolicitationResponse(106-149)SolicitationReview(182-205)
commcare_connect/solicitations/tests/test_permissions.py (4)
commcare_connect/solicitations/views.py (6)
SolicitationAccessMixin(36-46)SolicitationManagerMixin(49-63)test_func(44-46)test_func(57-63)test_func(76-106)test_func(1100-1115)commcare_connect/users/tests/factories.py (1)
UserFactory(10-26)commcare_connect/conftest.py (2)
user(50-51)organization(45-46)commcare_connect/organization/models.py (1)
Role(29-32)
commcare_connect/solicitations/migrations/0002_alter_solicitationresponse_status.py (2)
commcare_connect/solicitations/migrations/0001_initial.py (1)
Migration(8-277)commcare_connect/solicitations/migrations/0003_alter_solicitationresponse_responses.py (1)
Migration(6-17)
commcare_connect/solicitations/migrations/0001_initial.py (3)
commcare_connect/solicitations/migrations/0002_alter_solicitationresponse_status.py (1)
Migration(6-28)commcare_connect/solicitations/migrations/0003_alter_solicitationresponse_responses.py (1)
Migration(6-17)commcare_connect/solicitations/migrations/0004_add_response_attachment_model.py (1)
Migration(8-50)
commcare_connect/program/models.py (1)
commcare_connect/solicitations/tests/factories.py (1)
description(92-96)
commcare_connect/solicitations/management/commands/load_uat_data.py (7)
commcare_connect/organization/models.py (3)
Organization(11-25)UserOrganizationMembership(28-62)Role(29-32)commcare_connect/opportunity/models.py (1)
DeliveryType(53-59)commcare_connect/program/models.py (1)
Program(9-26)commcare_connect/solicitations/models.py (5)
Solicitation(10-74)SolicitationQuestion(77-103)SolicitationResponse(106-149)SolicitationReview(182-205)delete(175-179)commcare_connect/users/models.py (1)
User(13-64)commcare_connect/solicitations/forms.py (2)
save(150-173)save(326-343)commcare_connect/solicitations/tests/factories.py (2)
responses(195-222)questions(167-188)
commcare_connect/solicitations/management/commands/test_dashboard_links.py (2)
commcare_connect/users/models.py (1)
User(13-64)commcare_connect/solicitations/management/commands/load_uat_data.py (2)
add_arguments(25-34)handle(361-431)
commcare_connect/solicitations/forms.py (3)
commcare_connect/solicitations/helpers.py (1)
process_question_form_data(91-120)commcare_connect/solicitations/models.py (10)
Solicitation(10-74)SolicitationQuestion(77-103)SolicitationResponse(106-149)SolicitationReview(182-205)Meta(61-67)Meta(98-100)Meta(124-130)Meta(164-165)Meta(200-202)Recommendation(187-190)commcare_connect/solicitations/tests/factories.py (9)
questions(167-188)responses(195-222)Meta(72-73)Meta(129-130)Meta(147-148)Meta(228-229)target_population(99-100)scope_of_work(103-108)estimated_scale(111-113)
🪛 GitHub Actions: CI
QUICK_START_GUIDE.md
[error] 1-1: Prettier formatting check failed. File was modified by the hook; run 'prettier --write' to fix code style issues.
commcare_connect/reports/views.py
[error] 36-36: flake8: undefined name 'LoginRequiredMixin' (F821) detected in views.py
[error] 50-50: flake8: undefined name 'UserPassesTestMixin' (F821) detected in views.py
🪛 LanguageTool
commcare_connect/solicitations/instructions/user-organization-program-relationships.md
[style] ~89-~89: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ...nce accepted User Requirements: - Can be ADMIN, MEMBER, or VIEWER - Different...
(MISSING_IT_THERE)
commcare_connect/solicitations/instructions/table_review.md
[uncategorized] ~7-~7: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... names duplicate model definitions. ## HIGH VALUE Simplifications 🔥 ### 1. Remove Redun...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~152-~152: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...enefit:** More predictable behavior ## LOW VALUE Simplifications 🟢 ### 8. Extract Repe...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
QUICK_START_GUIDE.md
[style] ~12-~12: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...✅ ## 🏃♂️ Quick Start (Every Time You Want to Run the App) ### 1. Navigate to Projec...
(REP_WANT_TO_VB)
commcare_connect/solicitations/instructions/QA-prompt.md
[style] ~131-~131: Consider using a different verb for a more formal wording.
Context: ...and include a proposed cursor prompt to fix this issue
(FIX_RESOLVE)
🪛 markdownlint-cli2 (0.18.1)
commcare_connect/solicitations/instructions/labs.md
11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
commcare_connect/solicitations/instructions/review.MD
70-70: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
79-79: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
103-103: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
116-116: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
125-125: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
145-145: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
169-169: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
181-181: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
198-198: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
207-207: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
223-223: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
234-234: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
243-243: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
260-260: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
commcare_connect/solicitations/instructions/user-organization-program-relationships.md
9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
QUICK_START_GUIDE.md
38-38: Bare URL used
(MD034, no-bare-urls)
52-52: Bare URL used
(MD034, no-bare-urls)
170-170: Bare URL used
(MD034, no-bare-urls)
171-171: Bare URL used
(MD034, no-bare-urls)
172-172: Bare URL used
(MD034, no-bare-urls)
173-173: Bare URL used
(MD034, no-bare-urls)
174-174: Bare URL used
(MD034, no-bare-urls)
210-210: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
commcare_connect/solicitations/instructions/QA-prompt.md
10-10: Bare URL used
(MD034, no-bare-urls)
16-16: Bare URL used
(MD034, no-bare-urls)
commcare_connect/solicitations/instructions/implement-plan.md
122-122: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
172-172: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
188-188: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
204-204: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
219-219: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
263-263: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
273-273: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
283-283: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.3)
commcare_connect/solicitations/tests/test_helpers.py
155-155: Unpacked variable error is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
commcare_connect/solicitations/migrations/0005_remove_solicitationresponse_attachments_and_more.py
7-9: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
11-86: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
demo_decorator.py
1-1: Shebang is present but file is not executable
(EXE001)
commcare_connect/solicitations/migrations/0004_add_response_attachment_model.py
9-12: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
14-50: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
commcare_connect/solicitations/tests/test_views.py
55-55: Unused method argument: solicitation_type
(ARG002)
commcare_connect/solicitations/migrations/0008_simplify_response_status.py
6-6: Unused function argument: schema_editor
(ARG001)
23-25: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
27-38: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
commcare_connect/solicitations/tables.py
47-47: Use of mark_safe may expose cross-site scripting vulnerabilities
(S308)
88-88: Unused method argument: record
(ARG002)
125-125: Use of mark_safe may expose cross-site scripting vulnerabilities
(S308)
204-204: Use of mark_safe may expose cross-site scripting vulnerabilities
(S308)
246-246: Unused method argument: record
(ARG002)
268-269: try-except-pass detected, consider logging the exception
(S110)
268-268: Do not catch blind exception: Exception
(BLE001)
278-279: try-except-pass detected, consider logging the exception
(S110)
278-278: Do not catch blind exception: Exception
(BLE001)
327-327: Use of mark_safe may expose cross-site scripting vulnerabilities
(S308)
commcare_connect/solicitations/migrations/0003_alter_solicitationresponse_responses.py
7-9: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
11-17: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
commcare_connect/solicitations/tests/factories.py
87-87: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
93-93: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
94-94: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
100-100: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
104-104: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
112-112: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
167-167: Unused method argument: kwargs
(ARG002)
177-177: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
178-178: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
195-195: Unused method argument: extracted
(ARG002)
195-195: Unused method argument: kwargs
(ARG002)
217-221: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
commcare_connect/solicitations/management/commands/load_solicitation_fixtures.py
49-49: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
49-49: Avoid specifying long messages outside the exception class
(TRY003)
51-51: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
51-51: Avoid specifying long messages outside the exception class
(TRY003)
231-231: Unused method argument: args
(ARG002)
commcare_connect/solicitations/models.py
62-62: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
63-67: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
99-99: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
100-100: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
125-125: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
126-126: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
127-130: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
165-165: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
201-201: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
202-202: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
commcare_connect/solicitations/migrations/0007_remove_completed_status.py
7-9: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
11-22: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
commcare_connect/program/migrations/0002_alter_program_description.py
7-9: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
11-17: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
commcare_connect/opportunity/migrations/0083_alter_uservisit_location.py
7-9: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
11-17: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
commcare_connect/solicitations/helpers.py
209-209: Consider moving this statement to an else block
(TRY300)
212-212: Use explicit conversion flag
Replace with conversion flag
(RUF010)
213-213: Do not catch blind exception: Exception
(BLE001)
214-214: Use explicit conversion flag
Replace with conversion flag
(RUF010)
283-283: Consider moving this statement to an else block
(TRY300)
286-286: Use explicit conversion flag
Replace with conversion flag
(RUF010)
287-287: Do not catch blind exception: Exception
(BLE001)
288-288: Use explicit conversion flag
Replace with conversion flag
(RUF010)
commcare_connect/reports/views.py
36-36: Undefined name LoginRequiredMixin
(F821)
36-36: Undefined name UserPassesTestMixin
(F821)
commcare_connect/solicitations/views.py
157-157: Avoid specifying long messages outside the exception class
(TRY003)
309-309: Unused method argument: queryset
(ARG002)
599-599: Unused method argument: queryset
(ARG002)
614-614: Avoid specifying long messages outside the exception class
(TRY003)
618-618: Avoid specifying long messages outside the exception class
(TRY003)
809-811: try-except-pass detected, consider logging the exception
(S110)
809-809: Do not catch blind exception: Exception
(BLE001)
847-847: Unused method argument: queryset
(ARG002)
983-983: Unpacked variable created is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
997-997: Do not catch blind exception: Exception
(BLE001)
1012-1012: Unpacked variable created is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
1048-1048: Do not catch blind exception: Exception
(BLE001)
1049-1049: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1073-1073: Do not catch blind exception: Exception
(BLE001)
1074-1074: Use explicit conversion flag
Replace with conversion flag
(RUF010)
commcare_connect/solicitations/admin.py
8-8: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
9-9: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
10-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
11-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
25-25: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
26-26: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
27-27: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
28-28: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
37-37: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
38-38: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
39-39: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
40-40: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
52-52: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
53-53: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
54-54: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
55-55: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
commcare_connect/solicitations/migrations/0006_remove_eoi_rfp_relationships.py
7-9: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
11-35: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
commcare_connect/solicitations/tests/test_permissions.py
15-15: Unused method argument: request
(ARG002)
22-22: Unused method argument: request
(ARG002)
commcare_connect/solicitations/migrations/0002_alter_solicitationresponse_status.py
7-9: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
11-28: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
commcare_connect/solicitations/migrations/0001_initial.py
11-15: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
17-277: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
commcare_connect/solicitations/management/commands/load_uat_data.py
227-227: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
233-233: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
249-249: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
277-277: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
279-279: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
330-330: Unused method argument: programs
(ARG002)
353-353: Unpacked variable membership is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
361-361: Unused method argument: args
(ARG002)
390-390: Do not catch blind exception: Exception
(BLE001)
commcare_connect/solicitations/management/commands/test_dashboard_links.py
32-32: Unused method argument: args
(ARG002)
46-46: Consider [*current_allowed_hosts, "testserver"] instead of concatenation
Replace with [*current_allowed_hosts, "testserver"]
(RUF005)
63-63: Do not catch blind exception: Exception
(BLE001)
64-64: Use explicit conversion flag
Replace with conversion flag
(RUF010)
commcare_connect/solicitations/forms.py
183-192: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
193-257: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
319-319: Avoid specifying long messages outside the exception class
(TRY003)
322-322: Avoid specifying long messages outside the exception class
(TRY003)
353-353: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
| env: | ||
| AWS_REGION: us-east-1 | ||
| ECS_CLUSTER: labs-jj-cluster | ||
| ECS_WEB_SERVICE: labs-jj-web | ||
| ECS_WORKER_SERVICE: labs-jj-worker | ||
| ECR_REPOSITORY: labs-jj-commcare-connect | ||
| ECR_REGISTRY: 858923557655.dkr.ecr.us-east-1.amazonaws.com |
There was a problem hiding this comment.
Move sensitive AWS identifiers to secrets and parameterize environment variables.
The workflow exposes hardcoded AWS account ID (858923557655) in the ECR registry and uses hardcoded user-specific resource names (labs-jj-*). This configuration is not suitable for a shared repository and creates a security/operational burden.
Recommended actions:
- Move AWS account ID and region to GitHub environment secrets or organization variables
- Create repository/environment-specific configuration (e.g., via GitHub environments feature)
- Allow different deployments per branch or environment
Example refactor:
env:
- AWS_REGION: us-east-1
- ECS_CLUSTER: labs-jj-cluster
- ECS_WEB_SERVICE: labs-jj-web
- ECS_WORKER_SERVICE: labs-jj-worker
- ECR_REPOSITORY: labs-jj-commcare-connect
- ECR_REGISTRY: 858923557655.dkr.ecr.us-east-1.amazonaws.com
+ AWS_REGION: ${{ secrets.AWS_REGION }}
+ ECS_CLUSTER: ${{ secrets.ECS_CLUSTER }}
+ ECS_WEB_SERVICE: ${{ secrets.ECS_WEB_SERVICE }}
+ ECS_WORKER_SERVICE: ${{ secrets.ECS_WORKER_SERVICE }}
+ ECR_REPOSITORY: ${{ secrets.ECR_REPOSITORY }}
+ ECR_REGISTRY: ${{ secrets.ECR_REGISTRY }}Alternatively, use GitHub Environments for deployment-specific configs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| env: | |
| AWS_REGION: us-east-1 | |
| ECS_CLUSTER: labs-jj-cluster | |
| ECS_WEB_SERVICE: labs-jj-web | |
| ECS_WORKER_SERVICE: labs-jj-worker | |
| ECR_REPOSITORY: labs-jj-commcare-connect | |
| ECR_REGISTRY: 858923557655.dkr.ecr.us-east-1.amazonaws.com | |
| env: | |
| AWS_REGION: ${{ secrets.AWS_REGION }} | |
| ECS_CLUSTER: ${{ secrets.ECS_CLUSTER }} | |
| ECS_WEB_SERVICE: ${{ secrets.ECS_WEB_SERVICE }} | |
| ECS_WORKER_SERVICE: ${{ secrets.ECS_WORKER_SERVICE }} | |
| ECR_REPOSITORY: ${{ secrets.ECR_REPOSITORY }} | |
| ECR_REGISTRY: ${{ secrets.ECR_REGISTRY }} |
🤖 Prompt for AI Agents
.github/workflows/deploy-labs.yml lines 21-27: the workflow hardcodes sensitive
and environment-specific values (AWS account ID, region and lab-specific
resource names), so move those to secrets/repository or environment variables
and parameterize the resource names; replace the literal ECR_REGISTRY value with
a constructed value using a secret like AWS_ACCOUNT_ID and a secret/var for
AWS_REGION, move AWS_REGION into a secret/organization variable, and replace
ECS_CLUSTER/ECS_WEB_SERVICE/ECS_WORKER_SERVICE/ECR_REPOSITORY with inputs or
repo/environment variables (or GitHub Environment-specific variables) so
different branches/environments can supply different names and nobody stores
account IDs in source; update workflow to read the secrets/vars and construct
registry/service names at runtime and add documentation for required
secrets/environment configuration.
| - name: Run database migrations | ||
| if: ${{ github.event.inputs.run_migrations == 'true' }} | ||
| run: | | ||
| echo "🔄 Running database migrations..." | ||
| # Get the latest task definition | ||
| TASK_DEF_ARN=$(aws ecs describe-task-definition \ | ||
| --task-definition ${{ env.ECS_WEB_SERVICE }} \ | ||
| --query 'taskDefinition.taskDefinitionArn' \ | ||
| --output text) | ||
| echo "Using task definition: $TASK_DEF_ARN" | ||
| # Run migration task | ||
| TASK_ARN=$(aws ecs run-task \ | ||
| --cluster ${{ env.ECS_CLUSTER }} \ | ||
| --task-definition $TASK_DEF_ARN \ | ||
| --launch-type FARGATE \ | ||
| --network-configuration "awsvpcConfiguration={subnets=[subnet-06646effb09be2f42],securityGroups=[sg-0666a5ed512c97d9d],assignPublicIp=ENABLED}" \ | ||
| --overrides '{"containerOverrides":[{"name":"web","command":["/start_migrate"]}]}' \ | ||
| --query 'tasks[0].taskArn' \ | ||
| --output text) | ||
| echo "Migration task started: $TASK_ARN" | ||
| echo "Waiting for migration to complete..." | ||
| # Wait for task to complete | ||
| aws ecs wait tasks-stopped \ | ||
| --cluster ${{ env.ECS_CLUSTER }} \ | ||
| --tasks $TASK_ARN | ||
| # Check exit code | ||
| EXIT_CODE=$(aws ecs describe-tasks \ | ||
| --cluster ${{ env.ECS_CLUSTER }} \ | ||
| --tasks $TASK_ARN \ | ||
| --query 'tasks[0].containers[0].exitCode' \ | ||
| --output text) | ||
| if [ "$EXIT_CODE" == "0" ]; then | ||
| echo "✅ Migrations completed successfully" | ||
| else | ||
| echo "❌ Migrations failed with exit code: $EXIT_CODE" | ||
| echo "Check CloudWatch Logs: https://console.aws.amazon.com/cloudwatch/home?region=${{ env.AWS_REGION }}#logsV2:log-groups/log-group/%2Fecs%2F${{ env.ECS_WEB_SERVICE }}" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
Parameterize network configuration and add robustness checks for migration task.
The migration step has several concerns:
- Hardcoded network configuration (line 84): Subnet and security group IDs are hardcoded, making the workflow inflexible and exposing infrastructure details.
- No timeout on wait operation: The
aws ecs wait tasks-stoppedcommand has no explicit timeout; it could hang indefinitely if the task doesn't stop. - Unvalidated container command: The
/start_migratecommand is assumed to exist; verify it's present in the Dockerfile/container definition.
Recommended improvements:
- name: Run database migrations
if: ${{ github.event.inputs.run_migrations == 'true' }}
run: |
echo "🔄 Running database migrations..."
# Get the latest task definition
TASK_DEF_ARN=$(aws ecs describe-task-definition \
--task-definition ${{ env.ECS_WEB_SERVICE }} \
--query 'taskDefinition.taskDefinitionArn' \
--output text)
echo "Using task definition: $TASK_DEF_ARN"
# Run migration task
TASK_ARN=$(aws ecs run-task \
--cluster ${{ env.ECS_CLUSTER }} \
--task-definition $TASK_DEF_ARN \
--launch-type FARGATE \
--network-configuration "awsvpcConfiguration={subnets=[${{ secrets.ECS_SUBNET_IDS }}],securityGroups=[${{ secrets.ECS_SECURITY_GROUPS }}],assignPublicIp=ENABLED}" \
--overrides '{"containerOverrides":[{"name":"web","command":["/start_migrate"]}]}' \
--query 'tasks[0].taskArn' \
--output text)
echo "Migration task started: $TASK_ARN"
echo "Waiting for migration to complete (timeout: 30 minutes)..."
# Wait for task to complete with timeout
timeout 1800 aws ecs wait tasks-stopped \
--cluster ${{ env.ECS_CLUSTER }} \
--tasks $TASK_ARN || { echo "❌ Migration task timed out or failed"; exit 1; }Verify that /start_migrate is a valid entrypoint command in your container.
I'll verify the concerns raised in this review comment by examining the workflow file and related infrastructure code.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and examine the deploy-labs.yml workflow file
fd -t f "deploy-labs.yml" --exec cat {} \;
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for Dockerfile and container definition files
fd -t f -e Dockerfile -o -name "*.dockerfile" | head -20Length of output: 230
🏁 Script executed:
#!/bin/bash
# Search for references to /start_migrate command in the codebase
rg -n "start_migrate" -C 3Length of output: 223
🏁 Script executed:
#!/bin/bash
# Search for AWS network configuration patterns and how subnets/security groups are typically configured
rg -n "subnet-|sg-0" -C 2 | head -50Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for ECS task definition files or configurations
fd -t f -e json -o -e yaml -o -e yml | xargs grep -l "taskDefinition\|ecs" 2>/dev/null | head -20Length of output: 116
🏁 Script executed:
#!/bin/bash
# Check for any existing timeout patterns in GitHub workflows
rg -n "timeout" .github/workflows/ -C 2Length of output: 0
I need to search more thoroughly for the workflow file and environment configuration. Let me refine my approach:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for all workflow files, not just deploy-labs.yml
find .github/workflows -type f -name "*.yml" -o -name "*.yaml" 2>/dev/null
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Search for the deploy-labs workflow with broader patterns
rg -l "deploy-labs" --type yamlLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look for environment variable definitions related to ECS or subnet
rg "ECS_SUBNET|ECS_SECURITY|subnet-|sg-" --type yaml -C 2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for environment or secrets definitions in workflow files
rg "env:|secrets\." .github/workflows/ --type yaml -C 1 | head -60Length of output: 1549
🏁 Script executed:
#!/bin/bash
# Check if there's a .github/workflows directory and list contents
ls -la .github/workflows/ 2>/dev/null || echo "Directory not found"Length of output: 292
Now let me examine the actual deploy-labs.yml file to verify the concerns:
🏁 Script executed:
#!/bin/bash
# Extract and display the deploy-labs.yml file content
cat .github/workflows/deploy-labs.ymlLength of output: 6722
🏁 Script executed:
#!/bin/bash
# Search specifically for the hardcoded subnet and security group IDs
rg "subnet-06646effb09be2f42|sg-0666a5ed512c97d9d" .github/workflows/deploy-labs.ymlLength of output: 157
🏁 Script executed:
#!/bin/bash
# Search for all references to network configuration in the workflow
rg "awsvpcConfiguration|subnets|securityGroups" .github/workflows/deploy-labs.yml -C 3Length of output: 451
🏁 Script executed:
#!/bin/bash
# Check for timeout command in deploy-labs.yml
rg "timeout|tasks-stopped" .github/workflows/deploy-labs.yml -C 2Length of output: 154
🏁 Script executed:
#!/bin/bash
# Search for ECS environment variables across all workflow files
rg "ECS_CLUSTER|ECS_WEB_SERVICE" .github/workflows/ -C 1Length of output: 3908
Now let me verify if the suggested secrets approach is configured in the repository:
🏁 Script executed:
#!/bin/bash
# Search for secrets configuration or documentation
rg "ECS_SUBNET|ECS_SECURITY" . --type md --type yaml --type json -C 2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's any secrets documentation or config
find . -name "*.md" -o -name "secrets*" -o -name ".env*" 2>/dev/null | head -20Length of output: 795
🏁 Script executed:
#!/bin/bash
# Look for any reference to subnet or security group IDs in documentation
rg "subnet|security.group" . --type md -C 2Length of output: 0
Parameterize network configuration and add robustness checks for migration task.
The migration step requires two critical fixes:
- Hardcoded network configuration (line 84): Subnet and security group IDs are hardcoded, making the workflow inflexible and exposing infrastructure details. Move these to environment variables or secrets.
- No timeout on wait operation: The
aws ecs wait tasks-stoppedcommand (line 90) has no explicit timeout; it could hang indefinitely if the task doesn't stop.
Recommended improvements:
- name: Run database migrations
if: ${{ github.event.inputs.run_migrations == 'true' }}
run: |
echo "🔄 Running database migrations..."
# Get the latest task definition
TASK_DEF_ARN=$(aws ecs describe-task-definition \
--task-definition ${{ env.ECS_WEB_SERVICE }} \
--query 'taskDefinition.taskDefinitionArn' \
--output text)
echo "Using task definition: $TASK_DEF_ARN"
# Run migration task
TASK_ARN=$(aws ecs run-task \
--cluster ${{ env.ECS_CLUSTER }} \
--task-definition $TASK_DEF_ARN \
--launch-type FARGATE \
--network-configuration "awsvpcConfiguration={subnets=[${{ secrets.ECS_SUBNET_IDS }}],securityGroups=[${{ secrets.ECS_SECURITY_GROUPS }}],assignPublicIp=ENABLED}" \
--overrides '{"containerOverrides":[{"name":"web","command":["/start_migrate"]}]}' \
--query 'tasks[0].taskArn' \
--output text)
echo "Migration task started: $TASK_ARN"
echo "Waiting for migration to complete (timeout: 30 minutes)..."
# Wait for task to complete with timeout
timeout 1800 aws ecs wait tasks-stopped \
--cluster ${{ env.ECS_CLUSTER }} \
--tasks $TASK_ARN || { echo "❌ Migration task timed out or failed"; exit 1; }🤖 Prompt for AI Agents
In .github/workflows/deploy-labs.yml around lines 66 to 110, the migration step
hardcodes subnet and security group IDs and uses aws ecs wait without a timeout;
update the step to read subnet and security group IDs (and assignPublicIp if
needed) from environment variables or secrets (e.g., ${{ env.MIGRATION_SUBNET }}
and ${{ env.MIGRATION_SECURITY_GROUP }}) and validate they are set before
invoking run-task, and replace the single aws ecs wait call with a robust
polling/timeout approach (or implement a loop that checks task status via
describe-tasks with a maximum timeout/retries and fails if exceeded), also add
checks that TASK_ARN and EXIT_CODE are present and fail fast with clear errors
if run-task or describe-tasks return empty values.
.github/workflows/deploy-labs.yml
Outdated
| - name: Get public IP | ||
| id: get-ip | ||
| run: | | ||
| TASK_ARN=$(aws ecs list-tasks \ | ||
| --cluster ${{ env.ECS_CLUSTER }} \ | ||
| --service-name ${{ env.ECS_WEB_SERVICE }} \ | ||
| --query 'taskArns[0]' \ | ||
| --output text) | ||
| ENI_ID=$(aws ecs describe-tasks \ | ||
| --cluster ${{ env.ECS_CLUSTER }} \ | ||
| --tasks $TASK_ARN \ | ||
| --query 'tasks[0].attachments[0].details[?name==`networkInterfaceId`].value' \ | ||
| --output text) | ||
| PUBLIC_IP=$(aws ec2 describe-network-interfaces \ | ||
| --network-interface-ids $ENI_ID \ | ||
| --query 'NetworkInterfaces[0].Association.PublicIp' \ | ||
| --output text) | ||
| echo "public_ip=$PUBLIC_IP" >> $GITHUB_OUTPUT | ||
| echo "Public IP: $PUBLIC_IP" |
There was a problem hiding this comment.
🧩 Analysis chain
Harden ENI lookup query for robustness.
The public IP extraction assumes a specific structure for ECS task network attachments. If the attachment structure differs, the query will fail silently or return an empty string, leading to broken deployment summary URLs.
Add defensive checks:
- name: Get public IP
id: get-ip
run: |
TASK_ARN=$(aws ecs list-tasks \
--cluster ${{ env.ECS_CLUSTER }} \
--service-name ${{ env.ECS_WEB_SERVICE }} \
--query 'taskArns[0]' \
--output text)
+ if [ -z "$TASK_ARN" ] || [ "$TASK_ARN" == "None" ]; then
+ echo "❌ Failed to find running task for service ${{ env.ECS_WEB_SERVICE }}"
+ exit 1
+ fi
ENI_ID=$(aws ecs describe-tasks \
--cluster ${{ env.ECS_CLUSTER }} \
--tasks $TASK_ARN \
--query 'tasks[0].attachments[0].details[?name==`networkInterfaceId`].value' \
--output text)
+ if [ -z "$ENI_ID" ] || [ "$ENI_ID" == "None" ]; then
+ echo "❌ Failed to extract network interface ID from task"
+ exit 1
+ fi
PUBLIC_IP=$(aws ec2 describe-network-interfaces \
--network-interface-ids $ENI_ID \
--query 'NetworkInterfaces[0].Association.PublicIp' \
--output text)
+ if [ -z "$PUBLIC_IP" ] || [ "$PUBLIC_IP" == "None" ]; then
+ echo "⚠️ Warning: Could not retrieve public IP (might still be assigning)"
+ PUBLIC_IP="<pending>"
+ fi
echo "public_ip=$PUBLIC_IP" >> $GITHUB_OUTPUT
echo "Public IP: $PUBLIC_IP"I'll verify the current state of the workflow file and validate the review comment's concerns.
🏁 Script executed:
#!/bin/bash
# Check the current state of the deploy-labs.yml file around lines 138-159
cat .github/workflows/deploy-labs.yml | sed -n '130,170p' | cat -nLength of output: 1951
🏁 Script executed:
#!/bin/bash
# Search for any existing error handling or defensive checks in the file
rg -n "TASK_ARN|ENI_ID|PUBLIC_IP" .github/workflows/deploy-labs.yml -A2 -B2Length of output: 1593
🏁 Script executed:
#!/bin/bash
# Check how public_ip output is used in the workflow
rg -n "public_ip|get-ip" .github/workflows/deploy-labs.yml -A3 -B3Length of output: 949
🏁 Script executed:
#!/bin/bash
# Verify the file exists and show full section for context
test -f .github/workflows/deploy-labs.yml && echo "File exists" || echo "File not found"
wc -l .github/workflows/deploy-labs.ymlLength of output: 49
Add defensive checks for AWS CLI command outputs to prevent silent failures.
The workflow lacks validation after each AWS CLI call in the "Get public IP" step. If TASK_ARN, ENI_ID, or PUBLIC_IP are empty or return "None", the step silently succeeds but produces broken deployment summary URLs (lines 167-168).
Add validation after each extraction:
- name: Get public IP
id: get-ip
run: |
TASK_ARN=$(aws ecs list-tasks \
--cluster ${{ env.ECS_CLUSTER }} \
--service-name ${{ env.ECS_WEB_SERVICE }} \
--query 'taskArns[0]' \
--output text)
+ if [ -z "$TASK_ARN" ] || [ "$TASK_ARN" == "None" ]; then
+ echo "❌ Failed to find running task for service ${{ env.ECS_WEB_SERVICE }}"
+ exit 1
+ fi
ENI_ID=$(aws ecs describe-tasks \
--cluster ${{ env.ECS_CLUSTER }} \
--tasks $TASK_ARN \
--query 'tasks[0].attachments[0].details[?name==`networkInterfaceId`].value' \
--output text)
+ if [ -z "$ENI_ID" ] || [ "$ENI_ID" == "None" ]; then
+ echo "❌ Failed to extract network interface ID from task"
+ exit 1
+ fi
PUBLIC_IP=$(aws ec2 describe-network-interfaces \
--network-interface-ids $ENI_ID \
--query 'NetworkInterfaces[0].Association.PublicIp' \
--output text)
+ if [ -z "$PUBLIC_IP" ] || [ "$PUBLIC_IP" == "None" ]; then
+ echo "⚠️ Warning: Could not retrieve public IP (might still be assigning)"
+ PUBLIC_IP="<pending>"
+ fi
echo "public_ip=$PUBLIC_IP" >> $GITHUB_OUTPUT
echo "Public IP: $PUBLIC_IP"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Get public IP | |
| id: get-ip | |
| run: | | |
| TASK_ARN=$(aws ecs list-tasks \ | |
| --cluster ${{ env.ECS_CLUSTER }} \ | |
| --service-name ${{ env.ECS_WEB_SERVICE }} \ | |
| --query 'taskArns[0]' \ | |
| --output text) | |
| ENI_ID=$(aws ecs describe-tasks \ | |
| --cluster ${{ env.ECS_CLUSTER }} \ | |
| --tasks $TASK_ARN \ | |
| --query 'tasks[0].attachments[0].details[?name==`networkInterfaceId`].value' \ | |
| --output text) | |
| PUBLIC_IP=$(aws ec2 describe-network-interfaces \ | |
| --network-interface-ids $ENI_ID \ | |
| --query 'NetworkInterfaces[0].Association.PublicIp' \ | |
| --output text) | |
| echo "public_ip=$PUBLIC_IP" >> $GITHUB_OUTPUT | |
| echo "Public IP: $PUBLIC_IP" | |
| - name: Get public IP | |
| id: get-ip | |
| run: | | |
| TASK_ARN=$(aws ecs list-tasks \ | |
| --cluster ${{ env.ECS_CLUSTER }} \ | |
| --service-name ${{ env.ECS_WEB_SERVICE }} \ | |
| --query 'taskArns[0]' \ | |
| --output text) | |
| if [ -z "$TASK_ARN" ] || [ "$TASK_ARN" == "None" ]; then | |
| echo "❌ Failed to find running task for service ${{ env.ECS_WEB_SERVICE }}" | |
| exit 1 | |
| fi | |
| ENI_ID=$(aws ecs describe-tasks \ | |
| --cluster ${{ env.ECS_CLUSTER }} \ | |
| --tasks $TASK_ARN \ | |
| --query 'tasks[0].attachments[0].details[?name==`networkInterfaceId`].value' \ | |
| --output text) | |
| if [ -z "$ENI_ID" ] || [ "$ENI_ID" == "None" ]; then | |
| echo "❌ Failed to extract network interface ID from task" | |
| exit 1 | |
| fi | |
| PUBLIC_IP=$(aws ec2 describe-network-interfaces \ | |
| --network-interface-ids $ENI_ID \ | |
| --query 'NetworkInterfaces[0].Association.PublicIp' \ | |
| --output text) | |
| if [ -z "$PUBLIC_IP" ] || [ "$PUBLIC_IP" == "None" ]; then | |
| echo "⚠️ Warning: Could not retrieve public IP (might still be assigning)" | |
| PUBLIC_IP="<pending>" | |
| fi | |
| echo "public_ip=$PUBLIC_IP" >> $GITHUB_OUTPUT | |
| echo "Public IP: $PUBLIC_IP" |
🤖 Prompt for AI Agents
In .github/workflows/deploy-labs.yml around lines 138 to 159, add defensive
checks after each AWS CLI extraction (TASK_ARN, ENI_ID, PUBLIC_IP) to avoid
silent failures: after assigning TASK_ARN verify it is non-empty and not "None"
and if it is, emit an error (echo "::error::...") and exit 1; repeat the same
check for ENI_ID and for PUBLIC_IP, echo a descriptive error and exit 1 on
failure; keep the existing outputs when valid (echo "public_ip=... >>
$GITHUB_OUTPUT") so the workflow fails fast with a clear message instead of
producing broken URLs.
| operations = [ | ||
| migrations.AlterField( | ||
| model_name="uservisit", | ||
| name="location", | ||
| field=models.CharField(max_length=255, null=True), | ||
| ), | ||
| ] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify if this migration is necessary.
This migration alters the location field to CharField(max_length=255, null=True), which appears to be identical to the definition already added in migration 0030 (from 2024-01-31). Unless there were intermediate migrations between 0030 and 0083 that modified this field, this migration may be redundant.
Run the following script to check for any intermediate migrations that modified the location field:
🏁 Script executed:
#!/bin/bash
# Description: Search for migrations between 0030 and 0083 that reference the location field
# Find all migration files in the opportunity app
fd -e py . commcare_connect/opportunity/migrations/ --exec grep -l "location" {} \; | sort
# Look for the specific migrations to understand the history
rg -A 5 -B 2 "location" commcare_connect/opportunity/migrations/003[0-9]*.py commcare_connect/opportunity/migrations/004*.py commcare_connect/opportunity/migrations/005*.py commcare_connect/opportunity/migrations/006*.py commcare_connect/opportunity/migrations/007*.py commcare_connect/opportunity/migrations/008[0-2]*.pyLength of output: 3023
---
🏁 Script executed:
```shell
#!/bin/bash
# Display the full content of migration 0083 to check dependencies
cat commcare_connect/opportunity/migrations/0083_alter_uservisit_location.py
Length of output: 444
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and display the UserVisit model definition to verify the current location field
rg -A 30 "class UserVisit" commcare_connect/opportunity/ --type py -g "*.py" | head -100
Length of output: 8357
Remove migration 0083—it is redundant and does not make any changes.
Migration 0030 initially set location to CharField(max_length=255, null=True). Migration 0083 attempts to alter the same field to the identical definition. No intermediate migrations between 0030 and 0083 modify the location field. This migration should be removed from the codebase or squashed into a preceding migration.
🧰 Tools
🪛 Ruff (0.14.3)
11-17: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🤖 Prompt for AI Agents
In commcare_connect/opportunity/migrations/0083_alter_uservisit_location.py
around lines 11 to 17, the migration alters UserVisit.location to
CharField(max_length=255, null=True) but this is identical to migration 0030 and
thus redundant; remove migration 0083 from the codebase (or squash it into an
earlier migration) so the duplicate change is not applied—delete the file and
any references to it, run makemigrations/checks to ensure no new migration is
generated, and run tests/migrations locally to confirm the migration history
remains consistent.
| ("name", models.CharField(max_length=255)), | ||
| ("slug", models.SlugField(max_length=255, unique=True)), | ||
| ("description", models.CharField()), | ||
| ("description", models.CharField(max_length=255)), |
There was a problem hiding this comment.
🧩 Analysis chain
Verify migration sequencing for description field.
The description field is defined with max_length=255 in this initial migration, and then a subsequent migration 0002_alter_program_description.py alters the same field to add the same constraint. This pattern suggests the initial migration may have been modified after the second migration was created.
Verify the migration history to ensure there are no conflicts:
🏁 Script executed:
#!/bin/bash
# Check if there are any conflicting migrations or if this was retroactively modified
echo "Checking migration dependencies and field history..."
# Look for any Program model definition in non-migration files
rg -n "class Program" --type=py -g '!*/migrations/*'
# Check for any other migrations that reference Program.description
rg -n "description" commcare_connect/program/migrations/ -A 2 -B 2Length of output: 2820
I need to examine the full migration files and the current Program model to verify the migration sequencing. Let me gather more complete information.
🏁 Script executed:
#!/bin/bash
# Get the full content of migrations 0001 and 0002
echo "=== Migration 0001_initial.py ==="
cat commcare_connect/program/migrations/0001_initial.py
echo -e "\n=== Migration 0002_alter_program_description.py ==="
cat commcare_connect/program/migrations/0002_alter_program_description.py
echo -e "\n=== Current Program model ==="
rg -A 30 "^class Program\(BaseModel\)" commcare_connect/program/models.pyLength of output: 5656
I need to check the git history to determine if migration 0001 was retroactively modified after 0002 was created.
🏁 Script executed:
#!/bin/bash
# Check git history for the migration files
echo "=== Git log for 0001_initial.py ==="
git log --oneline --all -- commcare_connect/program/migrations/0001_initial.py
echo -e "\n=== Git log for 0002_alter_program_description.py ==="
git log --oneline --all -- commcare_connect/program/migrations/0002_alter_program_description.py
echo -e "\n=== Check if there are migrations after 0002 ==="
ls -la commcare_connect/program/migrations/ | grep "^-.*\.py$" | grep -E "000[3-9]|00[1-9][0-9]"
echo -e "\n=== All migration files in order ==="
ls -1 commcare_connect/program/migrations/*.py | sortLength of output: 1301
I found a critical issue beyond the original concern. Let me examine the conflicting 0002 migration:
🏁 Script executed:
#!/bin/bash
echo "=== Migration 0002_update_delivery_type.py ==="
cat commcare_connect/program/migrations/0002_update_delivery_type.py
echo -e "\n=== Checking migration dependencies ==="
rg "dependencies\s*=" commcare_connect/program/migrations/000[12]*.py -A 3Length of output: 1797
Critical migration sequencing conflict detected.
The repository contains two migrations both numbered 0002_*:
0002_alter_program_description.py(alters description field to identical CharField(max_length=255))0002_update_delivery_type.py(data migration for delivery_type)
This violates Django's migration sequencing requirements—each migration number must be unique per app. Django will fail during migration application due to the ambiguous 0002 reference.
Required fixes:
- Remove
0002_alter_program_description.py(it's a no-op: the description field already hasmax_length=255in the initial migration) - Rename
0002_update_delivery_type.pyto0003_update_delivery_type.pyand update its dependency from("program", "0001_initial")to("program", "0002_alter_program_description")OR to the next valid migration if 0002_alter is removed
🤖 Prompt for AI Agents
commcare_connect/program/migrations/0001_initial.py around line 26: there is a
migration sequencing conflict because a redundant
0002_alter_program_description.py (no-op) collides with
0002_update_delivery_type.py; remove
commcare_connect/program/migrations/0002_alter_program_description.py entirely,
keep only commcare_connect/program/migrations/0002_update_delivery_type.py as
the sole 0002 migration, and open 0002_update_delivery_type.py to ensure its
Migration.dependencies point to ("program", "0001_initial") (update the tuple if
needed) so migration numbers are unique and dependency chain remains valid.
| <a href="mailto:{{ solicitation.program.organization.name|slugify }}@commcareconnect.org" | ||
| class="button button-md outline-style"> | ||
| <i class="fa-solid fa-envelope mr-2"></i> | ||
| Get in Touch | ||
| </a> |
There was a problem hiding this comment.
🧩 Analysis chain
Verify email address construction logic.
The email address is constructed by slugifying the organization name and appending @commcareconnect.org. This approach could lead to issues:
- Slugified names may not match actual email addresses
- No validation that the constructed email exists
- Organizations might have different contact email formats
Consider storing the contact email in the Program or Organization model instead of constructing it dynamically.
To verify if organization email addresses are stored elsewhere, run:
🏁 Script executed:
#!/bin/bash
# Search for email-related fields in Organization and Program models
ast-grep --pattern 'class Organization($$$) {
$$$
email = $$$
$$$
}'
ast-grep --pattern 'class Program($$$) {
$$$
email = $$$
$$$
}'
# Also search for any contact_email or similar fields
rg -n --type=py -C3 'contact.*email|email.*contact' commcare_connect/organization/models.py commcare_connect/program/models.pyLength of output: 0
I need to search the codebase with corrected queries. The initial script used incorrect syntax for Python classes. Let me investigate properly:
🏁 Script executed:
#!/bin/bash
# Find the actual model files
echo "=== Finding Organization and Program model files ==="
fd -e py -p '(organization|program).*models' commcare_connect/
echo -e "\n=== Searching for email-related fields in Python files ==="
rg -n --type=py 'email|contact.*email' commcare_connect/organization/ commcare_connect/program/ 2>/dev/null | head -30
echo -e "\n=== Checking for any email field definitions ==="
rg -n --type=py -B2 -A2 '^\s*(email|contact_email|contact_phone|phone)\s*=' commcare_connect/organization/models.py commcare_connect/program/models.py
echo -e "\n=== Searching for slugify usage in templates ==="
rg -n 'slugify' commcare_connect/templates/ -A2 -B2
echo -e "\n=== Checking for mailto or email construction patterns ==="
rg -n 'mailto:' commcare_connect/templates/ -B1 -A1Length of output: 4518
Add contact_email field to Organization or Program model.
The email address is constructed by slugifying the organization name without validation. Search confirms no email or contact_email fields exist in Organization or Program models—only user-related emails. This means:
- Slugified names won't reliably match intended email addresses
- No validation that constructed emails exist
- No support for custom contact email formats
Store a proper contact_email field on the model to replace the dynamic construction.
🤖 Prompt for AI Agents
In commcare_connect/templates/solicitations/solicitation_detail.html around
lines 186-190, the template builds a contact email by slugifying the
organization name which is brittle and unvalidated; add a proper contact_email
field to the persistent model (Organization or Program) and use it in the
template. Modify the chosen model to include a Django EmailField (contact_email)
with null=True, blank=True, and validators; create and run a migration; update
admin/forms/fixtures to expose and populate the field (or provide a sensible
default/fallback), add unit tests to validate formatting and presence where
required, and change the template to use program.contact_email or
organization.contact_email (with a safe fallback if empty) instead of
constructing an address from the slugified name.
| // Form submission handler | ||
| const solicitationForm = document.getElementById('solicitation-form'); | ||
| const self = this; // Capture the correct 'this' context | ||
| if (solicitationForm) { | ||
| solicitationForm.addEventListener('submit', (e) => { | ||
| const questionsDataField = document.getElementById('questions-data'); | ||
| if (questionsDataField) { | ||
| questionsDataField.value = JSON.stringify(self.questions); | ||
| console.log('Questions data being submitted:', self.questions); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Restore hidden questions payload before submit
solicitationQuestionBuilder serializes the question list into an element with id questions-data, but the template never renders that hidden input. When the user saves, questionsDataField is null, so no JSON ever posts back and any edited/added questions are silently dropped on the server. Please add the hidden field (or update the selector) so the form actually submits the serialized questions.
@@
- {% csrf_token %}
-
- {% crispy form %}
+ {% csrf_token %}
+
+ <input type="hidden" id="questions-data" name="questions_data" value="[]">
+
+ {% crispy form %}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In commcare_connect/templates/solicitations/solicitation_form.html around lines
226 to 237, the script expects a hidden input with id "questions-data" but the
template does not render it so questions are never posted; add a hidden input
inside the solicitation form (e.g. <input type="hidden" id="questions-data"
name="questions_data">) ensuring the name matches the server-side field
expected, place it within the <form> before this script runs (or update the
selector in the script to match the existing input id/name if one exists), and
confirm the submit handler sets its value to JSON.stringify(this.questions) so
edited/added questions are submitted.
| ```bash | ||
| cd /Users/sarveshtewari/commcare-connect | ||
| ``` |
There was a problem hiding this comment.
Remove user-specific hardcoded path.
The guide contains a hardcoded path specific to one developer's machine (/Users/sarveshtewari/commcare-connect). This will not work for other developers and should be replaced with a generic placeholder.
Apply this diff:
### 1. Navigate to Project Directory
```bash
-cd /Users/sarveshtewari/commcare-connect
+cd /path/to/commcare-connect
<details>
<summary>🤖 Prompt for AI Agents</summary>
In QUICK_START_GUIDE.md around lines 15 to 17, the guide uses a hardcoded,
user-specific path (/Users/sarveshtewari/commcare-connect); replace it with a
generic placeholder such as /path/to/commcare-connect (or ./commcare-connect) so
instructions work for all developers and update the line accordingly.
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
| ```bash | ||
| cd /Users/sarveshtewari/commcare-connect | ||
| source .venv/bin/activate |
There was a problem hiding this comment.
Remove duplicate user-specific hardcoded path.
Same issue as above—this hardcoded path should use a generic placeholder.
Apply this diff:
```bash
-cd /Users/sarveshtewari/commcare-connect
+cd /path/to/commcare-connect
source .venv/bin/activate
python manage.py runserver🤖 Prompt for AI Agents
In QUICK_START_GUIDE.md around lines 46 to 48, there is a user-specific
hardcoded path "cd /Users/sarveshtewari/commcare-connect"; replace that exact
line with a generic placeholder such as "cd /path/to/commcare-connect" so the
guide is portable for other users, keeping the rest of the snippet (activate
virtualenv and runserver) unchanged.
tatus
Outdated
| jj/EOI-pages[m | ||
| * [32mmain[m |
There was a problem hiding this comment.
Remove this accidental Git output file.
This file appears to be terminal output from a Git command (possibly git branch or git status) that was accidentally committed. It contains ANSI color escape sequences and branch names, which serve no purpose in the repository.
Remove this file from the repository:
#!/bin/bash
# Remove the accidental git output file
git rm tatus🤖 Prompt for AI Agents
In file tatus lines 1-2, the file is accidental Git command output containing
ANSI escape sequences and branch names; remove it from the repository by
deleting the file and committing that deletion (e.g., run git rm tatus, commit
with a clear message like "remove accidental git output file tatus", and push
the branch); if the file must be purged from history as well, run a
history-rewrite tool (git filter-branch or BFG) and coordinate with the team
before force-pushing.
- Enforce LF line endings in .gitattributes (eol=lf) - Simplify database backups section in LABS-IMPLEMENTATION.md
- Complete session-based OAuth flow (no DB storage) - LabsUser model compatible with Django User interface - Custom middleware for authentication and URL whitelisting - Login/logout/status pages with clean UI - Updated user helpers/middleware to support LabsUser - Added labs navigation from audit list page
Workers were being killed by OOM. With 1GB memory, 2 workers is appropriate.
- Remove 'Get public IP' step from workflow - Update deployment summary to show labs.connect.dimagi.com URL - Update architecture diagram to show ALB - Update Key Design Decisions to reflect ALB usage - Remove 'Get Current Public IP' troubleshooting section
- Remove intermediate progress messages (redundant with step names) - Keep only essential debugging info (task ARN) and success/failure messages - Retain deployment summary at end with URLs and links
GitHub Actions UI already shows workflow success/failure and all the info is available in the Actions console.
…ally need to support it.
- Add max_length to UserVisit.location and Program.description fields - Generate corresponding database migrations - Add comprehensive quick start guide for new developers - Fix Django model validation issues
jjackson
force-pushed
the
jj/EOI-pages
branch
from
4ef664a to
0c045cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 37
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
commcare_connect/contrib/sites/migrations/0003_set_site_domain_and_name.py (1)
19-37: Remove unreachable dead code.The outer condition at line 19 already excludes SQLite (
connection.vendor != "sqlite"), making the inner SQLite check at lines 29-30 unreachable and redundant. When the outer condition is met,connection.vendoris guaranteed to NOT be "sqlite", so the inner conditionif connection.vendor == "sqlite":can never evaluate to true.Apply this diff to remove the dead code:
if created and connection.vendor != "sqlite": # We provided the ID explicitly when creating the Site entry, therefore the DB # sequence to auto-generate them wasn't used and is now out of sync. If we # don't do anything, we'll get a unique constraint violation the next time a # site is created. # To avoid this, we need to manually update DB sequence and make sure it's # greater than the maximum value. max_id = site_model.objects.order_by('-id').first().id with connection.cursor() as cursor: - # Skip sequence adjustment on SQLite where sequences are not used - if connection.vendor == "sqlite": - return cursor.execute("SELECT last_value from django_site_id_seq") (current_id,) = cursor.fetchone() if current_id <= max_id: cursor.execute( "alter sequence django_site_id_seq restart with %s", [max_id + 1], )
♻️ Duplicate comments (16)
commcare_connect/reports/views.py (1)
36-38: Add missing imports for mixin base classes.The
SuperUserRequiredMixinreferencesLoginRequiredMixinandUserPassesTestMixin, but these classes are not imported, causing CI pipeline failures.Add the required imports at the top of the file:
from datetime import date, datetime, timedelta import django_filters import django_tables2 as tables from crispy_forms.helper import FormHelper from crispy_forms.layout import Column, Layout, Row from django import forms from django.conf import settings +from django.contrib.auth.mixins import LoginRequiredMixin, UserPassesTestMixin from django.db.models import Count, Sum from django.db.models.functions import TruncDate from django.http import JsonResponseQUICK_START_GUIDE.md (2)
15-17: The hardcoded path/Users/sarveshtewari/commcare-connectshould be replaced with a generic placeholder like/path/to/commcare-connectas previously noted.
46-48: Same hardcoded path issue as above - use a generic placeholder instead of the user-specific path..github/workflows/deploy-labs.yml (2)
21-27: The hardcoded AWS account ID and user-specific resource names should be moved to secrets/environment variables as previously noted. This affects portability and security.
67-106: The migration step requires parameterization of hardcoded network configuration (subnet and security group IDs) and a timeout for theaws ecs waitcommand as previously identified.commcare_connect/solicitations/instructions/implement-plan.md (1)
5-6: Remove or update the "out of date" disclaimer.This documentation still contains the disclaimer "out of date compared to latest code" that was previously flagged. Either update the documentation to reflect the current implementation or remove it if it no longer serves a purpose, as outdated documentation can confuse maintainers.
commcare_connect/solicitations/migrations/0007_remove_completed_status.py (1)
11-21: Add data migration to convert "completed" status to "closed".Existing
Solicitationrecords withstatus="completed"will fail validation after this migration removes the choice. ARunPythonoperation is needed to bulk-update these records before altering the field definition.As noted in the previous review, apply this pattern:
class Migration(migrations.Migration): dependencies = [ ("solicitations", "0006_remove_eoi_rfp_relationships"), ] + + def convert_completed_to_closed(apps, schema_editor): + Solicitation = apps.get_model("solicitations", "Solicitation") + Solicitation.objects.filter(status="completed").update(status="closed") operations = [ + migrations.RunPython(convert_completed_to_closed, migrations.RunPython.noop), migrations.AlterField( model_name="solicitation", name="status",commcare_connect/opportunity/migrations/0083_alter_uservisit_location.py (1)
11-17: Remove redundant migration.This migration attempts to alter
UserVisit.locationtoCharField(max_length=255, null=True), but migration 0030 already set this exact definition. Since no intermediate migrations modified this field, this is a no-op that should be removed.As noted in the previous review, this migration is redundant and should be deleted from the codebase.
commcare_connect/solicitations/admin.py (1)
42-47: Remove nonexistentattachmentsfield from response admin
SolicitationResponsehas noattachmentsfield, so loading this admin form raisesFieldError: Unknown field(s) (attachments). Please drop the “Files” fieldset (or point it at the actual related name) before merging.("Basic Information", {"fields": ("solicitation", "organization", "submitted_by", "submission_date")}), ("Status", {"fields": ("status",)}), ("Responses", {"fields": ("responses",), "classes": ("collapse",)}), - ("Files", {"fields": ("attachments",)}),commcare_connect/solicitations/management/commands/load_solicitation_fixtures.py (1)
77-84: Stop referencing the non-existentacceptedcolumn on memberships.
UserOrganizationMembershiphas noacceptedfield. Passingaccepted=Truein the defaults (Lines 77‑84) triggersTypeError, and the later queryset filter onmemberships__accepted(Lines 162‑164) throwsFieldError. Drop those references or gate them until the model actually exposes that column.Also applies to: 162-164
commcare_connect/solicitations/tests/factories.py (1)
205-210: Factory responses still use the wrong keys.
The form pipeline stores response data keyed byquestion.question_text, but the factory still emitsquestion_{question.id}keys. Anything relying on realistic factory data (forms, helpers, integration tests) will look up the wrong keys and fail. Please switch toquestion.question_textfor both file and non-file answers.- if question.question_type == SolicitationQuestion.Type.FILE: - question_responses[f"question_{question.id}"] = "sample_document.pdf" - else: - question_responses[f"question_{question.id}"] = fake.text(max_nb_chars=300) + key = question.question_text + if question.question_type == SolicitationQuestion.Type.FILE: + question_responses[key] = "sample_document.pdf" + else: + question_responses[key] = fake.text(max_nb_chars=300)commcare_connect/solicitations/management/commands/load_uat_data.py (3)
47-53: Populate DeliveryType slug/description when seeding.
DeliveryType.slugand.descriptionare non-nullable. Callingget_or_create(name=...)leaves them null during the first load, triggering an integrity error. Use the fixture’s slug/description viaupdate_or_createso reruns stay idempotent.- dt_name = dt_data["name"] # Extract name from dict - dt, created = DeliveryType.objects.get_or_create(name=dt_name) + dt, created = DeliveryType.objects.update_or_create( + slug=dt_data["slug"], + defaults={ + "name": dt_data["name"], + "description": dt_data["description"], + }, + ) if created: self.stdout.write(f"Created delivery type: {dt.name}") - delivery_types[dt.name] = dt + delivery_types[dt.name] = dt
80-116: UserOrganizationMembership defaults include unsupportedaccepted.
UserOrganizationMembershiphas noacceptedfield. Passing it throughdefaultsraisesTypeError: 'accepted' is an invalid keyword argumenton the first run. Removeaccepted(here and in the other membership creations on Lines 113-116, 343-357).- UserOrganizationMembership.objects.get_or_create( - user=user, - organization=org, - defaults={"role": UserOrganizationMembership.Role.ADMIN, "accepted": True}, - ) + UserOrganizationMembership.objects.get_or_create( + user=user, + organization=org, + defaults={"role": UserOrganizationMembership.Role.ADMIN}, + )
130-139: Program creation omits required fields.
Program.descriptionandProgram.currencyare required. The currentget_or_createcall leaves them null, so the initial load fails. Switch toupdate_or_createwith the fixture’s description/currency to stay idempotent.- program, created = Program.objects.get_or_create( + program, created = Program.objects.update_or_create( slug=prog_data["slug"], defaults={ "name": prog_data["name"], "organization": org, "delivery_type": delivery_type, "budget": Decimal(prog_data["budget"]), + "description": prog_data["description"], + "currency": prog_data["currency"], "start_date": date.today(), "end_date": date.today() + timedelta(days=730), }, )commcare_connect/solicitations/models.py (2)
111-122: Restore full response status choices.Migrations and existing rows still rely on the full workflow (
draft,submitted,under_review,accepted,rejected,progressed_to_rfp). Restricting the enum to only draft/submitted makes admin/forms reject persisted values and blocks downstream transitions. Please reintroduce the complete set so the model stays aligned with the schema and live data.class Status(models.TextChoices): DRAFT = "draft", _("Draft") SUBMITTED = "submitted", _("Submitted") + UNDER_REVIEW = "under_review", _("Under Review") + ACCEPTED = "accepted", _("Accepted") + REJECTED = "rejected", _("Rejected") + PROGRESSED_TO_RFP = "progressed_to_rfp", _("Progressed to RFP")
187-197: Keep review recommendation tokens aligned with the DB.The migration still stores
accept/reject/needs_more_info. Switching the enum torecommended/not_recommended/neutralcauses validation failures for existing rows and breaks admin/forms. Please retain the original tokens (or ship a data migration + schema change) so the field remains compatible.class Recommendation(models.TextChoices): - RECOMMENDED = "recommended", _("Recommended") - NOT_RECOMMENDED = "not_recommended", _("Not Recommended") - NEUTRAL = "neutral", _("Neutral") + ACCEPT = "accept", _("Accept") + REJECT = "reject", _("Reject") + NEEDS_MORE_INFO = "needs_more_info", _("Needs More Information")
🧹 Nitpick comments (11)
commcare_connect/solicitations/instructions/labs.md (1)
11-30: Consider adding language identifier to fenced code block.The ASCII diagram at lines 11-30 uses a fenced code block without a language identifier. While not critical for diagrams, adding
textor leaving it empty explicitly can satisfy linters.Apply this diff:
-``` +```text ┌─────────────────────┐ ┌─────────────────────┐ │ connect.dimagi.com │ │ labs.connect.dimagi │commcare_connect/labs/README.md (1)
209-224: Consider adding language identifier to fenced code block.The file tree structure at lines 209-224 should specify a language identifier for consistency with linting rules.
Apply this diff:
-``` +```text commcare_connect/labs/ ├── __init__.pycommcare_connect/labs/apps.py (1)
14-30: Consider failing fast when required OAuth settings are missing.The validation logs errors but allows the application to continue with missing OAuth configuration. This could lead to confusing runtime failures later. Consider raising an
ImproperlyConfiguredexception when required settings are missing in the labs environment.Apply this diff:
+from django.core.exceptions import ImproperlyConfigured + def ready(self): """Validate labs configuration on startup.""" # Only validate if we're in labs environment if not getattr(settings, "IS_LABS_ENVIRONMENT", False): return + missing_settings = [] + # Check required OAuth settings if not getattr(settings, "CONNECT_OAUTH_CLIENT_ID", None): logger.error("CONNECT_OAUTH_CLIENT_ID not configured for labs environment") + missing_settings.append("CONNECT_OAUTH_CLIENT_ID") if not getattr(settings, "CONNECT_OAUTH_CLIENT_SECRET", None): logger.error("CONNECT_OAUTH_CLIENT_SECRET not configured for labs environment") + missing_settings.append("CONNECT_OAUTH_CLIENT_SECRET") if not getattr(settings, "CONNECT_PRODUCTION_URL", None): logger.error("CONNECT_PRODUCTION_URL not configured for labs environment") + missing_settings.append("CONNECT_PRODUCTION_URL") + + if missing_settings: + raise ImproperlyConfigured( + f"Labs environment requires the following settings: {', '.join(missing_settings)}" + ) logger.info("Labs OAuth configuration validated")commcare_connect/solicitations/instructions/QA-prompt.md (1)
10-10: Optional: Format bare URLs for better Markdown compliance.The bare URLs on lines 10 and 16 should be wrapped in angle brackets for proper Markdown formatting:
-- **Base URL**: https://flying-kit-curiously.ngrok-free.app/ +- **Base URL**: <https://flying-kit-curiously.ngrok-free.app/> -- **Spec** (for reference only): https://raw.githubusercontent.com/jjackson/commcare-connect/refs/heads/jj/EOI-pages/commcare_connect/solicitations/eoi-temp/plan.md +- **Spec** (for reference only): <https://raw.githubusercontent.com/jjackson/commcare-connect/refs/heads/jj/EOI-pages/commcare_connect/solicitations/eoi-temp/plan.md>Also applies to: 16-16
commcare_connect/audit/management/commands/fix_connect_social_app.py (2)
17-19: Consider making domain configurable for different development environments.The Site domain is hardcoded to "localhost:8000". Consider making it configurable via a command-line argument for developers using different ports or domains.
def handle(self, *args, **options): + domain = options.get('domain', 'localhost:8000') self.stdout.write("\nFixing Connect Social App site association...") # Get or create the default site site, created = Site.objects.get_or_create( - pk=settings.SITE_ID, defaults={"domain": "localhost:8000", "name": "Local Development"} + pk=settings.SITE_ID, defaults={"domain": domain, "name": "Local Development"} )Add argument parser:
def add_arguments(self, parser): parser.add_argument( '--domain', type=str, default='localhost:8000', help='Domain for the development site (default: localhost:8000)' )
46-48: Consider completing the error handling block.The exception handler on line 46 provides a helpful error message but doesn't include closing output. Consider adding a closing newline for consistency with the success path (line 44).
except SocialApp.DoesNotExist: self.stdout.write(self.style.ERROR(" ✗ Connect Social App not found!")) - self.stdout.write(" You need to create it in Django Admin > Social Applications") + self.stdout.write(" You need to create it in Django Admin > Social Applications\n")commcare_connect/audit/management/extractors/sql_queries.py (2)
7-7: Verify the date in the comment.The comment date "9/19/2025" appears to be after the PR creation date (2025-09-17). Please verify whether this date is correct or if it should be adjusted to reflect when the query was actually written.
72-72: Document the hardcoded opportunity IDs.The queries contain hardcoded opportunity IDs (716, 715 on line 72; and a longer list on line 84) without explanation. Consider adding comments to document what these specific opportunities represent (e.g., test data, specific campaigns, etc.) to help future maintainers understand their purpose.
Apply this diff to add documentation:
-# 9/5/2025 Fake Data Analysis +# 9/5/2025 Fake Data Analysis +# Opportunity IDs 716, 715 are [add description here] SQL_FAKE_DATA_PARTY = """+# Opportunity IDs for [add description of what these represent] SQL_ALL_DATA_QUERY = """Also applies to: 84-84
commcare_connect/audit/services/assessment_generator.py (1)
50-62: Consider prefixing unused variables with underscore.The
assessmentvariable returned byget_or_createis unused (lines 50 and 89)—only thecreatedflag is needed for statistics. To silence the linter warning, consider prefixing with an underscore.Apply this pattern:
- assessment, created = Assessment.objects.get_or_create( + _assessment, created = Assessment.objects.get_or_create(Also applies to: 89-101
commcare_connect/audit/tables.py (1)
133-174: Avoidmark_safein table renderersBoth renderers already compose trusted strings, so calling
mark_safejust silences our security lint without adding value. Letformat_htmlhandle the HTML assembly instead.@@ - if value_lower == "pass": - badge_class = "badge badge-sm bg-green-50 text-green-700" - icon = '<i class="fa-solid fa-thumbs-up mr-1"></i>' - text = _("Pass") - elif value_lower == "fail": - badge_class = "badge badge-sm bg-red-50 text-red-700" - icon = '<i class="fa-solid fa-thumbs-down mr-1"></i>' - text = _("Fail") + if value_lower == "pass": + badge_class = "badge badge-sm bg-green-50 text-green-700" + icon_class = "fa-solid fa-thumbs-up mr-1" + text = _("Pass") + elif value_lower == "fail": + badge_class = "badge badge-sm bg-red-50 text-red-700" + icon_class = "fa-solid fa-thumbs-down mr-1" + text = _("Fail") @@ - return format_html('<span class="{}">{}{}</span>', badge_class, mark_safe(icon), text) + return format_html( + '<span class="{}"><i class="{}"></i>{}</span>', + badge_class, + icon_class, + text, + ) @@ - return format_html( - '<div class="flex gap-2 justify-end">{}</div>', mark_safe(" ".join(str(btn) for btn in buttons)) - ) + buttons_html = format_html(" ".join(["{}"] * len(buttons)), *buttons) + return format_html('<div class="flex gap-2 justify-end">{}</div>', buttons_html)commcare_connect/audit/admin.py (1)
58-71: Cacheuser_idsbefore rendering
obj.user_idsexecutes a freshvalues_list(...).distinct()each time, so this block triggers multiple identical queries per row. Cache the lists locally and reuse them to keep the admin detail page snappy.- return format_html( - "<strong>FLW:</strong> {}<br>" - "<strong>Domain:</strong> {}<br>" - "<strong>App ID:</strong> {}<br>" - "<strong>Opportunity IDs:</strong> {}<br>" - "<strong>User IDs:</strong> {}", - obj.flw_username or "N/A", - obj.domain or "N/A", - obj.app_id or "N/A", - ", ".join(map(str, obj.opportunity_ids)) if obj.opportunity_ids else "N/A", - ", ".join(map(str, obj.user_ids[:5])) + ("..." if len(obj.user_ids) > 5 else "") - if obj.user_ids - else "N/A", - ) + opportunity_ids = obj.opportunity_ids or [] + user_ids = obj.user_ids or [] + user_ids_display = ( + ", ".join(map(str, user_ids[:5])) + ("..." if len(user_ids) > 5 else "") + if user_ids + else "N/A" + ) + + return format_html( + "<strong>FLW:</strong> {}<br>" + "<strong>Domain:</strong> {}<br>" + "<strong>App ID:</strong> {}<br>" + "<strong>Opportunity IDs:</strong> {}<br>" + "<strong>User IDs:</strong> {}", + obj.flw_username or "N/A", + obj.domain or "N/A", + obj.app_id or "N/A", + ", ".join(map(str, opportunity_ids)) if opportunity_ids else "N/A", + user_ids_display, + )
| - name: Deploy web service | ||
| run: | | ||
| aws ecs update-service \ | ||
| --cluster ${{ env.ECS_CLUSTER }} \ | ||
| --service ${{ env.ECS_WEB_SERVICE }} \ | ||
| --force-new-deployment \ | ||
| --output text | ||
| - name: Deploy worker service | ||
| run: | | ||
| aws ecs update-service \ | ||
| --cluster ${{ env.ECS_CLUSTER }} \ | ||
| --service ${{ env.ECS_WORKER_SERVICE }} \ | ||
| --force-new-deployment \ | ||
| --output text | ||
| - name: Wait for services to stabilize | ||
| run: | | ||
| aws ecs wait services-stable \ | ||
| --cluster ${{ env.ECS_CLUSTER }} \ | ||
| --services ${{ env.ECS_WEB_SERVICE }} ${{ env.ECS_WORKER_SERVICE }} |
There was a problem hiding this comment.
Add error handling for service deployment steps.
The web and worker service deployment steps lack error handling. If either aws ecs update-service command fails, the workflow continues to the wait step, which may timeout or hang.
Apply this diff:
- name: Deploy web service
run: |
aws ecs update-service \
--cluster ${{ env.ECS_CLUSTER }} \
--service ${{ env.ECS_WEB_SERVICE }} \
--force-new-deployment \
- --output text
+ --output text || {
+ echo "❌ Failed to update web service"
+ exit 1
+ }
- name: Deploy worker service
run: |
aws ecs update-service \
--cluster ${{ env.ECS_CLUSTER }} \
--service ${{ env.ECS_WORKER_SERVICE }} \
--force-new-deployment \
- --output text
+ --output text || {
+ echo "❌ Failed to update worker service"
+ exit 1
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Deploy web service | |
| run: | | |
| aws ecs update-service \ | |
| --cluster ${{ env.ECS_CLUSTER }} \ | |
| --service ${{ env.ECS_WEB_SERVICE }} \ | |
| --force-new-deployment \ | |
| --output text | |
| - name: Deploy worker service | |
| run: | | |
| aws ecs update-service \ | |
| --cluster ${{ env.ECS_CLUSTER }} \ | |
| --service ${{ env.ECS_WORKER_SERVICE }} \ | |
| --force-new-deployment \ | |
| --output text | |
| - name: Wait for services to stabilize | |
| run: | | |
| aws ecs wait services-stable \ | |
| --cluster ${{ env.ECS_CLUSTER }} \ | |
| --services ${{ env.ECS_WEB_SERVICE }} ${{ env.ECS_WORKER_SERVICE }} | |
| - name: Deploy web service | |
| run: | | |
| aws ecs update-service \ | |
| --cluster ${{ env.ECS_CLUSTER }} \ | |
| --service ${{ env.ECS_WEB_SERVICE }} \ | |
| --force-new-deployment \ | |
| --output text || { | |
| echo "❌ Failed to update web service" | |
| exit 1 | |
| } | |
| - name: Deploy worker service | |
| run: | | |
| aws ecs update-service \ | |
| --cluster ${{ env.ECS_CLUSTER }} \ | |
| --service ${{ env.ECS_WORKER_SERVICE }} \ | |
| --force-new-deployment \ | |
| --output text || { | |
| echo "❌ Failed to update worker service" | |
| exit 1 | |
| } | |
| - name: Wait for services to stabilize | |
| run: | | |
| aws ecs wait services-stable \ | |
| --cluster ${{ env.ECS_CLUSTER }} \ | |
| --services ${{ env.ECS_WEB_SERVICE }} ${{ env.ECS_WORKER_SERVICE }} |
🤖 Prompt for AI Agents
In .github/workflows/deploy-labs.yml around lines 108 to 128, the two ecs
update-service steps run without checking failure and the workflow will continue
to the wait step even if deployments fail; modify each deploy step's run block
to enable strict bash error handling (e.g., start with set -euo pipefail) or
explicitly test the aws command exit status and exit with a non-zero code and a
descriptive error message on failure (for example: if ! aws ecs update-service
...; then echo "Error updating <web|worker> service" >&2; exit 1; fi), so the
workflow stops immediately when an update-service command fails and the wait
step only runs on success.
| - name: Wait for services to stabilize | ||
| run: | | ||
| aws ecs wait services-stable \ | ||
| --cluster ${{ env.ECS_CLUSTER }} \ | ||
| --services ${{ env.ECS_WEB_SERVICE }} ${{ env.ECS_WORKER_SERVICE }} |
There was a problem hiding this comment.
Add timeout to service stabilization wait.
The aws ecs wait services-stable command has no explicit timeout and could hang indefinitely if services fail to stabilize. Add a timeout to prevent workflow hangs.
Apply this diff:
- name: Wait for services to stabilize
run: |
+ echo "⏳ Waiting for services to stabilize (timeout: 10 minutes)..."
- aws ecs wait services-stable \
+ timeout 600 aws ecs wait services-stable \
--cluster ${{ env.ECS_CLUSTER }} \
- --services ${{ env.ECS_WEB_SERVICE }} ${{ env.ECS_WORKER_SERVICE }}
+ --services ${{ env.ECS_WEB_SERVICE }} ${{ env.ECS_WORKER_SERVICE }} || {
+ echo "❌ Services failed to stabilize within timeout"
+ exit 1
+ }🤖 Prompt for AI Agents
.github/workflows/deploy-labs.yml around lines 124-128: the aws ecs wait
services-stable command can hang indefinitely; wrap the wait command with a
timeout (e.g. prefix with timeout 10m) so the step fails if services don't
stabilize within the window, and ensure the step treats a non-zero exit as
failure (leave the pipeline to fail or add an explicit check); replace the run
block to use timeout 10m aws ecs wait services-stable --cluster ... --services
... and optionally handle the timeout exit code to surface a clear error
message.
| return ( | ||
| UserVisit.objects.filter( | ||
| user=flw_user, | ||
| opportunity=opportunity, | ||
| visit_date__date__range=[start_date, end_date], | ||
| status=VisitValidationStatus.approved, | ||
| ) | ||
| .exclude( | ||
| # Exclude visits that don't have images | ||
| Q(images__isnull=True) | ||
| | Q(images__content_type__isnull=True) | ||
| ) | ||
| .distinct() | ||
| .prefetch_related("images") | ||
| .order_by("visit_date") | ||
| ) |
There was a problem hiding this comment.
Remove ORM lookups on the images property.
UserVisit.images is a computed property (see opportunity models), not a related field. Filtering with images__… and calling prefetch_related("images") therefore throws FieldError: Cannot resolve keyword 'images' into field before any audit session can be validated. Rework this lookup—e.g., join to the concrete blob meta model or post-filter in Python—so the queryset executes without referencing the property.
| from commcare_connect.audit.oauth_views import ConnectOAuth2Adapter | ||
|
|
There was a problem hiding this comment.
Fix adapter import to avoid crash
ConnectOAuth2Adapter lives in commcare_connect.audit.provider, not oauth_views. This import raises ImportError, so the management command can’t even start. Update the import:
-from commcare_connect.audit.oauth_views import ConnectOAuth2Adapter
+from commcare_connect.audit.provider import ConnectOAuth2Adapter📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from commcare_connect.audit.oauth_views import ConnectOAuth2Adapter | |
| from commcare_connect.audit.provider import ConnectOAuth2Adapter |
🤖 Prompt for AI Agents
In commcare_connect/audit/management/commands/debug_oauth_flow.py around lines
11 to 12, the command imports ConnectOAuth2Adapter from
commcare_connect.audit.oauth_views which raises ImportError; update the import
to pull ConnectOAuth2Adapter from commcare_connect.audit.provider instead so the
management command can load without crashing.
| def _get_visit_count_preview_superset(self, params: AuditParameters) -> dict[str, int]: | ||
| """Get visit count preview using Superset data.""" | ||
| flw_ids_str = ",".join(map(str, params.flw_ids)) | ||
|
|
||
| sql = f""" | ||
| SELECT | ||
| status, | ||
| COUNT(*) as count | ||
| FROM opportunity_uservisit | ||
| WHERE opportunity_id = {params.opportunity_id} | ||
| AND user_id IN ({flw_ids_str}) | ||
| AND visit_date::date >= '{params.start_date}' | ||
| AND visit_date::date <= '{params.end_date}' | ||
| """ | ||
|
|
||
| if not params.include_test_data: | ||
| sql += " AND opportunity_id NOT IN (SELECT id FROM opportunity_opportunity WHERE is_test = true)" | ||
|
|
||
| if params.include_flagged_only: | ||
| sql += " AND flagged = true" | ||
|
|
||
| sql += " GROUP BY status" | ||
|
|
There was a problem hiding this comment.
Handle empty FLW lists before building SQL
When params.flw_ids is empty we produce user_id IN (), which Superset rejects. Guard for the empty case (return {"total": 0} early or skip the IN clause) before formatting the SQL. Apply the same fix to _download_audit_data_superset, which builds the same clause.
🧰 Tools
🪛 Ruff (0.14.3)
941-950: Possible SQL injection vector through string-based query construction
(S608)
🤖 Prompt for AI Agents
In commcare_connect/audit/management/extractors/connect_api_facade.py around
lines 937 to 959, the code builds a SQL fragment "user_id IN (...)" even when
params.flw_ids is empty which produces "user_id IN ()" and breaks Superset; add
a guard that checks if not params.flw_ids and return {"total": 0} immediately
(or alternatively skip adding the IN clause), then proceed to build the SQL only
when flw_ids is non-empty; apply the same empty-list guard/fix to the
_download_audit_data_superset function that constructs the same IN clause.
| solicitation, created = Solicitation.objects.get_or_create( | ||
| title=sol_data["title"], | ||
| defaults={ | ||
| "program": program, | ||
| "description": sol_data["description"], | ||
| "target_population": sol_data.get("target_population", "General population"), | ||
| "estimated_scale": sol_data.get("estimated_scale", "Regional"), | ||
| "status": sol_data.get("status", "active"), | ||
| "solicitation_type": sol_data.get("solicitation_type", "eoi"), | ||
| "application_deadline": date.today() + timedelta(days=30), | ||
| }, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Scope of work is missing from solicitation defaults.
Solicitation.scope_of_work is required (null=False, blank=False). Attempting to create a solicitation without it raises an IntegrityError. Please load the YAML value (or provide a safe fallback) and include it in the defaults.
"description": sol_data["description"],
"target_population": sol_data.get("target_population", "General population"),
+ "scope_of_work": sol_data["scope_of_work"],
"estimated_scale": sol_data.get("estimated_scale", "Regional"),🤖 Prompt for AI Agents
In commcare_connect/solicitations/management/commands/load_uat_data.py around
lines 156 to 168, the Solicitation.get_or_create defaults are missing the
required field scope_of_work which causes an IntegrityError; read scope_of_work
from sol_data (e.g. sol_data.get("scope_of_work")) and include it in the
defaults dictionary, falling back to a safe non-empty string (for example "Scope
not provided" or the YAML description) so the created Solicitation meets
null=False/blank=False constraints.
| current_allowed_hosts = list(settings.ALLOWED_HOSTS) | ||
| if "testserver" not in current_allowed_hosts: | ||
| settings.ALLOWED_HOSTS = current_allowed_hosts + ["testserver"] | ||
|
|
||
| # Create test client and login | ||
| client = Client() | ||
| client.force_login(user) | ||
|
|
||
| self.stdout.write(f"\n=== Testing Unified Dashboard for {user.email} ===") | ||
|
|
||
| def test_url(url, description): | ||
| try: | ||
| response = client.get(url, follow=True) | ||
| if response.status_code == 200: | ||
| self.stdout.write(f"✓ {description}") | ||
| return response | ||
| else: | ||
| self.stdout.write(self.style.ERROR(f"✗ {description}: HTTP {response.status_code}")) | ||
| return None | ||
| except Exception as e: | ||
| self.stdout.write(self.style.ERROR(f"✗ {description}: {str(e)}")) | ||
| return None | ||
|
|
||
| def extract_solicitation_links(html_content): | ||
| """Extract solicitation-related action links from HTML""" | ||
| links = [] | ||
| href_pattern = r'<a[^>]+href=["\']([^"\']+)["\'][^>]*>(.*?)</a>' | ||
|
|
||
| for match in re.finditer(href_pattern, html_content, re.DOTALL | re.IGNORECASE): | ||
| href = match.group(1) | ||
| link_content = match.group(2) | ||
|
|
||
| # Only include solicitation-related links, but exclude sort links | ||
| if ("/solicitations/" in href or href.startswith("/a/")) and "sort=" not in href: | ||
| # Get description from icon or text | ||
| description = "Action" | ||
| if "fa-eye" in link_content: | ||
| description = "View" | ||
| elif "fa-edit" in link_content or "fa-pen-to-square" in link_content: | ||
| description = "Edit" | ||
| elif "fa-inbox" in link_content: | ||
| description = "View Responses" | ||
| elif "Create" in link_content: | ||
| description = "Create" | ||
|
|
||
| links.append((href, description)) | ||
|
|
||
| return links | ||
|
|
||
| def extract_pagination_links(html_content): | ||
| """Extract pagination links from django-tables2 pagination""" | ||
| pagination_links = [] | ||
|
|
||
| # Look for pagination links in django-tables2 format | ||
| pagination_pattern = r'<a[^>]+href=["\']([^"\']*\?[^"\']*page=[^"\']*)["\'][^>]*>' | ||
|
|
||
| for match in re.finditer(pagination_pattern, html_content): | ||
| href = match.group(1) | ||
| parsed_url = urlparse(href) | ||
| query_params = parse_qs(parsed_url.query) | ||
|
|
||
| if "page" in query_params: | ||
| page_num = query_params["page"][0] | ||
| pagination_links.append((href, f"Page {page_num}")) | ||
|
|
||
| return pagination_links | ||
|
|
||
| # 1. Test Main Dashboard | ||
| self.stdout.write("\n--- Testing Main Dashboard ---") | ||
| dashboard_response = test_url("/solicitations/dashboard/", "Main dashboard") | ||
| if not dashboard_response: | ||
| return | ||
|
|
||
| dashboard_content = dashboard_response.content.decode() | ||
|
|
||
| if not pagination_only: | ||
| # 2. Test all action links on dashboard | ||
| self.stdout.write("\n--- Testing Dashboard Action Links ---") | ||
| dashboard_links = extract_solicitation_links(dashboard_content) | ||
|
|
||
| for url, description in dashboard_links: | ||
| if not test_url(url, f"Dashboard {description}: {url}"): | ||
| return | ||
|
|
||
| # 3. Test Pagination (if pagination links exist on the page) | ||
| self.stdout.write("\n--- Testing Pagination ---") | ||
|
|
||
| # Extract pagination links from the main dashboard page | ||
| pagination_links = extract_pagination_links(dashboard_content) | ||
|
|
||
| if pagination_links: | ||
| # Test the first few pagination links found on the page | ||
| for url, desc in pagination_links[:5]: # Test first 5 pagination links | ||
| if not test_url(url, f"Pagination: {desc}"): | ||
| return | ||
| else: | ||
| self.stdout.write("No pagination links found on dashboard") |
There was a problem hiding this comment.
Restore ALLOWED_HOSTS even when the command exits early.
The command mutates settings.ALLOWED_HOSTS, but every failure path after this point returns immediately, so the original value is never restored. When this command is invoked via call_command inside the test suite (or any long-lived process), the mutated host list leaks into subsequent code, potentially hiding genuine host configuration regressions. Please wrap the rest of the logic in a try/finally and restore the original list in the finally block so we leave global settings exactly as we found them.
- # Temporarily add testserver to ALLOWED_HOSTS for testing
- current_allowed_hosts = list(settings.ALLOWED_HOSTS)
- if "testserver" not in current_allowed_hosts:
- settings.ALLOWED_HOSTS = current_allowed_hosts + ["testserver"]
-
- # Create test client and login
- client = Client()
- client.force_login(user)
-
- self.stdout.write(f"\n=== Testing Unified Dashboard for {user.email} ===")
+ current_allowed_hosts = list(settings.ALLOWED_HOSTS)
+ if "testserver" not in current_allowed_hosts:
+ settings.ALLOWED_HOSTS = [*current_allowed_hosts, "testserver"]
+
+ try:
+ client = Client()
+ client.force_login(user)
+
+ self.stdout.write(f"\n=== Testing Unified Dashboard for {user.email} ===")
@@
- # Restore original ALLOWED_HOSTS
- settings.ALLOWED_HOSTS = current_allowed_hosts
-
- self.stdout.write(self.style.SUCCESS("All tests passed! Dashboard working correctly."))
+ finally:
+ settings.ALLOWED_HOSTS = current_allowed_hostsCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.3)
46-46: Consider [*current_allowed_hosts, "testserver"] instead of concatenation
Replace with [*current_allowed_hosts, "testserver"]
(RUF005)
63-63: Do not catch blind exception: Exception
(BLE001)
64-64: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In commcare_connect/solicitations/management/commands/test_dashboard_links.py
around lines 44 to 140, the command mutates settings.ALLOWED_HOSTS but returns
early on failures without restoring the original value; capture the original
settings.ALLOWED_HOSTS into a variable before changing it, wrap the remaining
logic (client creation, tests, extraction and all early returns) in a try block,
and put a finally block that restores settings.ALLOWED_HOSTS to the saved
original (use a shallow copy/list to avoid aliasing) so the global settings are
always reset even if the command returns or raises.
| old_statuses = ['under_review', 'accepted', 'rejected'] | ||
| for old_status in old_statuses: | ||
| SolicitationResponse.objects.filter(status=old_status).update(status='submitted') | ||
|
|
||
|
|
||
| def reverse_convert_old_statuses(apps, schema_editor): | ||
| """Reverse migration - not possible to restore original statuses""" | ||
| # We can't restore the original statuses since we don't know what they were | ||
| pass | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
| dependencies = [ | ||
| ("solicitations", "0007_remove_completed_status"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| # First convert the data | ||
| migrations.RunPython(convert_old_statuses, reverse_convert_old_statuses, hints={'run_on_secondary': False}), | ||
| # Then alter the field | ||
| migrations.AlterField( | ||
| model_name="solicitationresponse", | ||
| name="status", | ||
| field=models.CharField( | ||
| choices=[("draft", "Draft"), ("submitted", "Submitted")], default="draft", max_length=20 |
There was a problem hiding this comment.
Handle legacy progressed_to_rfp statuses before tightening choices
convert_old_statuses() only rewrites under_review, accepted, and rejected. We still have rows with the progressed_to_rfp value introduced earlier (see 0002). After this data migration, those rows remain, and the subsequent AlterField to choices [("draft", "Draft"), ("submitted", "Submitted")] will fail and roll back the deploy. Please fold progressed_to_rfp into the conversion (and any other legacy status we expect) before altering the field.
- old_statuses = ['under_review', 'accepted', 'rejected']
+ old_statuses = ['under_review', 'accepted', 'rejected', 'progressed_to_rfp']🧰 Tools
🪛 Ruff (0.14.3)
23-25: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
🤖 Prompt for AI Agents
In commcare_connect/solicitations/migrations/0008_simplify_response_status.py
around lines 11 to 35, convert_old_statuses currently only maps
['under_review','accepted','rejected'] to 'submitted' but misses legacy
'progressed_to_rfp' (and any other old statuses), causing the subsequent
AlterField choices tighten to fail; update the old_statuses list to include
'progressed_to_rfp' (and any other expected legacy values) so those rows are
also updated to 'submitted' before the AlterField runs, keeping
reverse_convert_old_statuses as a no-op.
| def render_two_line_text(title, subtitle=None): | ||
| """Render two-line text content""" | ||
| if subtitle: | ||
| return format_html( | ||
| '<div><div class="font-medium">{}</div><div class="text-xs text-gray-500">{}</div></div>', | ||
| title, | ||
| mark_safe(subtitle) if isinstance(subtitle, str) and ("<" in subtitle and ">" in subtitle) else subtitle, | ||
| ) |
There was a problem hiding this comment.
Remove unsafe mark_safe usage on subtitles.
render_two_line_text calls mark_safe on any string containing </>. Program descriptions are rich-text fields sourced from user input, so this bypasses Django’s escaping and lets a stored <script> (or any HTML payload) execute in dashboards. Let format_html escape the subtitle unless you’ve already sanitized it upstream.
- mark_safe(subtitle) if isinstance(subtitle, str) and ("<" in subtitle and ">" in subtitle) else subtitle,
+ subtitle,🧰 Tools
🪛 Ruff (0.14.3)
47-47: Use of mark_safe may expose cross-site scripting vulnerabilities
(S308)
🤖 Prompt for AI Agents
In commcare_connect/solicitations/tables.py around lines 41–48, remove the
unsafe mark_safe call on raw subtitle strings; instead only treat subtitles as
already-safe if they are an explicit SafeString (from django.utils.safestring
import SafeString), otherwise pass the subtitle through to format_html (or
str(subtitle)) so format_html will escape it. Concretely: drop the
mark_safe(...) branch that checks for "<" and ">", replace it with an
isinstance(subtitle, SafeString) check to allow previously-sanitized HTML, and
ensure non-safe subtitles are passed unchanged so format_html performs proper
escaping.
| # 🚀 CommCare Connect - Quick Start Guide | ||
|
|
||
| This guide will help you run the CommCare Connect app every time you want to see it. | ||
|
|
||
| ## 📋 Prerequisites | ||
|
|
||
| - **Python 3.11** installed ✅ | ||
| - **Docker & Docker Compose** installed (optional - app can run with SQLite) | ||
| - **Node.js & npm** installed ✅ | ||
| - **Git** installed ✅ | ||
|
|
||
| ## 🏃♂️ Quick Start (Every Time You Want to Run the App) | ||
|
|
||
| ### 1. Navigate to Project Directory | ||
| ```bash | ||
| cd /Users/sarveshtewari/commcare-connect | ||
| ``` | ||
|
|
||
| ### 2. Activate Virtual Environment | ||
| ```bash | ||
| source .venv/bin/activate | ||
| ``` | ||
|
|
||
| ### 3. Start Services (Optional - for PostgreSQL/Redis) | ||
| ```bash | ||
| # If you have Docker installed, start database services: | ||
| inv up | ||
|
|
||
| # If no Docker, the app will use SQLite (already configured) | ||
| ``` | ||
|
|
||
| ### 4. Start the Django Server | ||
| ```bash | ||
| python manage.py runserver | ||
| ``` | ||
|
|
||
| ### 5. Open Your Browser | ||
| Navigate to: **http://localhost:8000** | ||
|
|
||
| --- | ||
|
|
||
| ## 🚀 **Super Quick Start (No Docker Required)** | ||
|
|
||
| If you just want to run the app quickly without Docker: | ||
|
|
||
| ```bash | ||
| cd /Users/sarveshtewari/commcare-connect | ||
| source .venv/bin/activate | ||
| python manage.py runserver | ||
| ``` | ||
|
|
||
| Then open: **http://localhost:8000** | ||
|
|
||
| --- | ||
|
|
||
| ## 🔧 First-Time Setup (Only Once) | ||
|
|
||
| If this is your first time running the app, follow these additional steps: | ||
|
|
||
| ### 1. Install Python Dependencies | ||
| ```bash | ||
| # Activate virtual environment first | ||
| source .venv/bin/activate | ||
|
|
||
| # Install requirements | ||
| pip install -r requirements-dev.txt | ||
| ``` | ||
|
|
||
| ### 2. Install JavaScript Dependencies | ||
| ```bash | ||
| npm ci | ||
| ``` | ||
|
|
||
| ### 3. Build JavaScript Assets | ||
| ```bash | ||
| # Build once | ||
| inv build-js | ||
|
|
||
| # Or build and watch for changes | ||
| inv build-js -w | ||
| ``` | ||
|
|
||
| ### 4. Set Up Environment File | ||
| ```bash | ||
| # Copy template if you haven't already | ||
| cp .env_template .env | ||
|
|
||
| # Edit .env file if needed (usually not required for local development) | ||
| ``` | ||
|
|
||
| ### 5. Run Database Migrations | ||
| ```bash | ||
| python manage.py migrate | ||
| ``` | ||
|
|
||
| ### 6. Load Sample Data (Optional) | ||
| ```bash | ||
| # Load solicitation data for testing | ||
| python manage.py load_uat_data | ||
| ``` | ||
|
|
||
| ### 7. Create Superuser (Optional) | ||
| ```bash | ||
| python manage.py createsuperuser | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## 🛑 Stopping the App | ||
|
|
||
| ### Stop Django Server | ||
| Press `Ctrl+C` in the terminal where Django is running | ||
|
|
||
| ### Stop Docker Services | ||
| ```bash | ||
| # Option A: Using invoke | ||
| inv down | ||
|
|
||
| # Option B: Using docker-compose directly | ||
| docker compose down | ||
| ``` | ||
|
|
||
| ### Deactivate Virtual Environment | ||
| ```bash | ||
| deactivate | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## 🔍 Troubleshooting | ||
|
|
||
| ### Database Connection Issues | ||
| ```bash | ||
| # Check if Docker services are running | ||
| docker compose ps | ||
|
|
||
| # Restart Docker services | ||
| docker compose down | ||
| docker compose up -d | ||
| ``` | ||
|
|
||
| ### Python Dependencies Issues | ||
| ```bash | ||
| # Reinstall requirements | ||
| source .venv/bin/activate | ||
| pip install -r requirements-dev.txt | ||
| ``` | ||
|
|
||
| ### JavaScript Build Issues | ||
| ```bash | ||
| # Reinstall npm packages | ||
| rm -rf node_modules package-lock.json | ||
| npm ci | ||
| inv build-js | ||
| ``` | ||
|
|
||
| ### Port Already in Use | ||
| If port 8000 is already in use: | ||
| ```bash | ||
| # Use a different port | ||
| python manage.py runserver 8001 | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## 📱 Accessing the App | ||
|
|
||
| Once running, you can access: | ||
|
|
||
| - **Main App**: http://localhost:8000 | ||
| - **Admin Panel**: http://localhost:8000/admin | ||
| - **Solicitations**: http://localhost:8000/solicitations/ | ||
| - **EOIs Only**: http://localhost:8000/solicitations/eoi/ | ||
| - **RFPs Only**: http://localhost:8000/solicitations/rfp/ | ||
|
|
||
| --- | ||
|
|
||
| ## 🎯 Common Commands Reference | ||
|
|
||
| | Command | Purpose | | ||
| |---------|---------| | ||
| | `inv up` | Start Docker services | | ||
| | `inv down` | Stop Docker services | | ||
| | `inv build-js` | Build JavaScript assets | | ||
| | `inv build-js -w` | Build and watch JS for changes | | ||
| | `python manage.py runserver` | Start Django development server | | ||
| | `python manage.py migrate` | Run database migrations | | ||
| | `python manage.py load_uat_data` | Load sample solicitation data | | ||
| | `python manage.py createsuperuser` | Create admin user | | ||
|
|
||
| --- | ||
|
|
||
| ## 💡 Pro Tips | ||
|
|
||
| 1. **Keep Docker Running**: You can leave Docker services running between development sessions | ||
| 2. **Use Two Terminals**: Keep one terminal for Django server, another for other commands | ||
| 3. **Watch Mode**: Use `inv build-js -w` to automatically rebuild JS when files change | ||
| 4. **Sample Data**: The `load_uat_data` command creates realistic test data for solicitations | ||
|
|
||
| --- | ||
|
|
||
| ## 🆘 Need Help? | ||
|
|
||
| - Check the main [README.md](README.md) for detailed setup instructions | ||
| - Look at the [tasks.py](tasks.py) file for available commands | ||
| - Ensure all prerequisites are installed correctly | ||
|
|
||
| --- | ||
|
|
||
| **Happy coding! 🎉** |
There was a problem hiding this comment.
Fix Prettier formatting to resolve CI pipeline failure.
The Prettier formatting check failed for this file. Run prettier --write QUICK_START_GUIDE.md to auto-format the file according to the project's style guidelines.
#!/bin/bash
# Fix prettier formatting
npx prettier --write QUICK_START_GUIDE.md🧰 Tools
🪛 GitHub Actions: CI
[error] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues in this file.
🪛 LanguageTool
[style] ~12-~12: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...✅ ## 🏃♂️ Quick Start (Every Time You Want to Run the App) ### 1. Navigate to Projec...
(REP_WANT_TO_VB)
🪛 markdownlint-cli2 (0.18.1)
38-38: Bare URL used
(MD034, no-bare-urls)
52-52: Bare URL used
(MD034, no-bare-urls)
170-170: Bare URL used
(MD034, no-bare-urls)
171-171: Bare URL used
(MD034, no-bare-urls)
172-172: Bare URL used
(MD034, no-bare-urls)
173-173: Bare URL used
(MD034, no-bare-urls)
174-174: Bare URL used
(MD034, no-bare-urls)
210-210: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🤖 Prompt for AI Agents
In QUICK_START_GUIDE.md (lines 1-210) the file fails the Prettier formatting
check; run the project's Prettier formatter (e.g., npx prettier --write
QUICK_START_GUIDE.md or prettier --write QUICK_START_GUIDE.md), review the
formatted changes, stage and commit the updated file, and push the commit to
update the PR so CI formatting check passes.
2 participants
This PR adds Expression of Interest (EOI) pages and solicitation functionality to the CommCare Connect platform.