Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds document resource support: new Document model, sync/async DocumentService implementations and mixins, ProgramsService accessors, E2E and unit tests for document flows, and an e2e config update with a program document file ID. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/e2e/program/program/document/conftest.py (1)
10-11: Remove unused fixture dependency ininvalid_document_id.
e2e_configis not used here; dropping it will keep fixture dependencies minimal.♻️ Suggested simplification
`@pytest.fixture` -def invalid_document_id(e2e_config): +def invalid_document_id(): return "PDM-0000-0000-0000"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/program/program/document/conftest.py` around lines 10 - 11, The fixture function invalid_document_id currently declares an unused parameter e2e_config; remove that unused dependency by changing the function signature to invalid_document_id() (no parameters) and keep the same return value, and update any tests or fixtures that call or reference invalid_document_id to not pass e2e_config when invoking it.tests/unit/resources/program/mixin/test_document_mixin.py (1)
63-64: Assert mock call count before indexing calls for clearer failures.A missing request currently fails with
IndexError; assertingcall_count == 1first gives a precise test failure reason.🧪 Suggested assertion hardening
- result = mock_route.calls[0].request + assert mock_route.call_count == 1 + result = mock_route.calls[0].request ... - request = mock_route.calls[0].request + assert mock_route.call_count == 1 + request = mock_route.calls[0].request ... - request = mock_route.calls[0].request + assert mock_route.call_count == 1 + request = mock_route.calls[0].request ... - request = mock_route.calls[0].request + assert mock_route.call_count == 1 + request = mock_route.calls[0].requestAlso applies to: 88-89, 120-121, 144-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/resources/program/mixin/test_document_mixin.py` around lines 63 - 64, Add explicit assertions on the mock call count before indexing into mock_route.calls to avoid IndexError and give clearer failures: e.g., assert mock_route.call_count == 1 (or >= 1 as appropriate) immediately before each occurrence that dereferences mock_route.calls[0].request in the test methods in test_document_mixin.py (the occurrences around the lines with result = mock_route.calls[0].request and the similar places at the other noted locations). This ensures tests fail with a clear assertion message if the expected call wasn't made.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/program/program/document/conftest.py`:
- Around line 10-11: The fixture function invalid_document_id currently declares
an unused parameter e2e_config; remove that unused dependency by changing the
function signature to invalid_document_id() (no parameters) and keep the same
return value, and update any tests or fixtures that call or reference
invalid_document_id to not pass e2e_config when invoking it.
In `@tests/unit/resources/program/mixin/test_document_mixin.py`:
- Around line 63-64: Add explicit assertions on the mock call count before
indexing into mock_route.calls to avoid IndexError and give clearer failures:
e.g., assert mock_route.call_count == 1 (or >= 1 as appropriate) immediately
before each occurrence that dereferences mock_route.calls[0].request in the test
methods in test_document_mixin.py (the occurrences around the lines with result
= mock_route.calls[0].request and the similar places at the other noted
locations). This ensures tests fail with a clear assertion message if the
expected call wasn't made.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: bb3e4f74-4354-4ba5-8ed9-f16437a5ec44
📒 Files selected for processing (11)
e2e_config.test.jsonmpt_api_client/resources/program/mixins/__init__.pympt_api_client/resources/program/mixins/document_mixin.pympt_api_client/resources/program/programs.pympt_api_client/resources/program/programs_documents.pytests/e2e/program/program/document/conftest.pytests/e2e/program/program/document/test_async_document.pytests/e2e/program/program/document/test_sync_document.pytests/unit/resources/program/mixin/test_document_mixin.pytests/unit/resources/program/test_programs.pytests/unit/resources/program/test_programs_documents.py
robcsegal
changed the title
robcsegal
force-pushed
the
MPT-20324-add-endpoints-and-e-2-e-tests-for-program-documents
branch
from
6915df7 to
db185d1
Compare
robcsegal
changed the title
robcsegal
force-pushed
the
MPT-20324-add-endpoints-and-e-2-e-tests-for-program-documents
branch
from
db185d1 to
ce11996
Compare
|
albertsola
approved these changes
Apr 16, 2026
robcsegal
deleted the
MPT-20324-add-endpoints-and-e-2-e-tests-for-program-documents
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces a comprehensive "Document" resource for the Program domain in the API client, including full support for creating, retrieving, updating, deleting, publishing, and downloading program documents both synchronously and asynchronously. The implementation includes new service classes, mixins, and extensive end-to-end and unit test coverage to ensure robustness and correctness.
Key changes include:
Program Document Resource Implementation
Added
Documentmodel and correspondingDocumentServiceandAsyncDocumentServiceclasses, providing CRUD, publish/unpublish, and file upload/download capabilities for program documents. These services are now accessible via thedocuments()method on both sync and asyncProgramsServiceclasses. [1] [2] [3] [4]Introduced
DocumentMixinandAsyncDocumentMixinto encapsulate document-specific logic, including file handling and publishing features, and registered them in the program mixins module. [1] [2]Configuration and Test Data
e2e_config.test.jsonto include a new test document file ID for use in end-to-end tests.End-to-End and Unit Testing
Added extensive end-to-end tests (sync and async) for all document operations, including create (from file and URL), update, get, download, iteration, filtering, error handling, publishing, and unpublishing. [1] [2] [3]
Implemented unit tests for the new document mixins, validating correct multipart request construction for both file and URL-based document creation, for both sync and async services.
Extended unit tests for
ProgramsServiceandAsyncProgramsServiceto verify the newdocuments()service property returns the correct service class and endpoint parameters. [1] [2]These changes together provide a robust, fully-tested document management feature for program resources in the API client.
Closes MPT-20324(https://softwareone.atlassian.net/browse/MPT-20324)