[MPT-18938] Added render() as queryable mixin#226
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds QueryState support for a with_render flag that makes build() append render(), introduces QueryableMixin.options(render) to create cloned queryables with rendering configured, propagates with_render through chainable methods (order_by/filter/select), and updates unit and e2e tests accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mpt_api_client/http/mixins/queryable_mixin.py`:
- Around line 66-79: with_render() sets QueryState.with_render but other
mutating methods like filter(), order_by(), and select() rebuild QueryState
without preserving that flag, so calls like service.with_render().select("id")
drop it; update every place that constructs a new QueryState (including in
filter, order_by, select, and any other query-mutator that calls
_create_new_instance with QueryState(...)) to pass through the existing flag by
adding with_render=self.query_state.with_render (or use
getattr(self.query_state, "with_render", False)) to the QueryState constructor,
and keep with_render() itself setting with_render=True when it creates the new
instance.
In `@tests/unit/http/mixins/test_queryable_mixin.py`:
- Around line 83-88: Add an assertion that verifies the with_render flag
survives being chained by other query methods: after calling
dummy_service.with_render(), chain another query method (e.g., .select("id") or
.filter(...)) and assert the resulting object's query_state.with_render is still
True; update the test function test_queryable_mixin_with_render to call
dummy_service.with_render().select("id") (or .filter(...)) and assert that
result.query_state.with_render remains True while still checking
dummy_service.query_state.with_render is False and the original non-chained
result is a different instance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: d62f6201-5f8a-49f1-939d-101f3744a0c7
📒 Files selected for processing (6)
mpt_api_client/http/mixins/queryable_mixin.pympt_api_client/http/query_state.pytests/e2e/audit/records/test_async_records.pytests/e2e/audit/records/test_sync_records.pytests/unit/http/mixins/test_queryable_mixin.pytests/unit/http/test_query_state.py
robcsegal
force-pushed
the
MPT-18938-add-render-to-api-client-and-audit
branch
2 times, most recently
from
85a633c to
970ccc8
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/e2e/audit/records/test_sync_records.py (1)
48-56: Avoid reading the full result set here.The test only asserts on one record, so
list(service.iterate())adds unnecessary work and makes this e2e path slower than needed.Possible cleanup
def test_get_records_with_render(mpt_vendor: MPTClient, product_id) -> None: template_chars = ["{{", "}}"] service = mpt_vendor.audit.records.filter(RQLQuery(object__id=product_id)).with_render() - records = list(service.iterate()) - - result = records[0] + for result in service.iterate(): + break + else: + pytest.fail("Expected at least one rendered audit record") assert result.object.id == product_id assert not any(char in result.details for char in template_chars)Based on learnings, "In end-to-end tests (e.g., tests/e2e/...), reuse existing API resources for read-only operations and safe mutations to speed up test execution."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/audit/records/test_sync_records.py` around lines 48 - 56, The test eagerly materializes all records with list(service.iterate()) which is unnecessary and slow; replace that with retrieving only the first item from the iterator (e.g., result = next(service.iterate(), None)) in test_get_records_with_render so you don't fetch the whole result set, and add an assertion that result is not None before accessing result.object.id and result.details to avoid StopIteration errors. Reference: service and its iterate() method in test_get_records_with_render.tests/e2e/audit/records/test_async_records.py (1)
47-55: Avoid collecting the entire async iterator here.The test only inspects one record, so building a full list does extra paging/work and slows the e2e path.
Possible cleanup
async def test_get_records_with_render(async_mpt_vendor: AsyncMPTClient, product_id: str) -> None: template_chars = ["{{", "}}"] service = async_mpt_vendor.audit.records.filter(RQLQuery(object__id=product_id)).with_render() - records = [record async for record in service.iterate()] - - result = records[0] + async for result in service.iterate(): + break + else: + pytest.fail("Expected at least one rendered audit record") assert result.object.id == product_id assert not any(char in result.details for char in template_chars)Based on learnings, "In end-to-end tests (e.g., tests/e2e/...), reuse existing API resources for read-only operations and safe mutations to speed up test execution."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/audit/records/test_async_records.py` around lines 47 - 55, The test test_get_records_with_render currently exhausts the async iterator returned by service.iterate() into a list (records = [record async for record in service.iterate()]) which causes unnecessary paging; change it to only fetch the first record by either using anext(awaitable) or an async for loop that breaks after the first item (i.e., iterate service.iterate() and assign the first yielded record to result), keeping the rest of the assertions the same and referring to async_mpt_vendor, service = async_mpt_vendor.audit.records.filter(...).with_render() and service.iterate() to locate the code to modify.
🤖 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/audit/records/test_async_records.py`:
- Around line 47-55: The test test_get_records_with_render currently exhausts
the async iterator returned by service.iterate() into a list (records = [record
async for record in service.iterate()]) which causes unnecessary paging; change
it to only fetch the first record by either using anext(awaitable) or an async
for loop that breaks after the first item (i.e., iterate service.iterate() and
assign the first yielded record to result), keeping the rest of the assertions
the same and referring to async_mpt_vendor, service =
async_mpt_vendor.audit.records.filter(...).with_render() and service.iterate()
to locate the code to modify.
In `@tests/e2e/audit/records/test_sync_records.py`:
- Around line 48-56: The test eagerly materializes all records with
list(service.iterate()) which is unnecessary and slow; replace that with
retrieving only the first item from the iterator (e.g., result =
next(service.iterate(), None)) in test_get_records_with_render so you don't
fetch the whole result set, and add an assertion that result is not None before
accessing result.object.id and result.details to avoid StopIteration errors.
Reference: service and its iterate() method in test_get_records_with_render.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 40e21418-b032-4313-a4ca-95dcac2d359b
📒 Files selected for processing (6)
mpt_api_client/http/mixins/queryable_mixin.pympt_api_client/http/query_state.pytests/e2e/audit/records/test_async_records.pytests/e2e/audit/records/test_sync_records.pytests/unit/http/mixins/test_queryable_mixin.pytests/unit/http/test_query_state.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
mpt_api_client/http/mixins/queryable_mixin.py (1)
22-27: Consider centralizingQueryStatecloning.
QueryState(...)is rebuilt in four places here. A small internal helper to copy the current state with overrides would make future query-state additions much harder to miss.Also applies to: 39-44, 61-66, 75-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mpt_api_client/http/mixins/queryable_mixin.py` around lines 22 - 27, Create a small helper on the same class (e.g., def _clone_query_state(self, **overrides) -> QueryState) that builds a new QueryState by taking defaults from self.query_state (rql/filter, order_by, select, with_render, etc.) and applying any overrides passed in; then replace the four direct QueryState(...) constructions in this file (the places that currently rebuild query_state with new rql/order_by/select/with_render) to call this helper with only the changed fields (e.g., order_by=list(fields) or rql=self.query_state.filter) so future additions to QueryState only need to be handled in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/audit/records/test_async_records.py`:
- Around line 47-55: The test test_get_records_with_render currently only
inspects records[0] which can raise IndexError on empty responses and fails to
validate all returned entries; modify it to first assert that the list produced
by iterating service.iterate() is non-empty (e.g., assert records) and then
iterate over each record in records (from the
async_mpt_vendor.audit.records.filter(...).with_render() service) asserting both
result.object.id == product_id and that no template delimiters (["{{","}}"])
appear in result.details for every record to ensure the render invariant across
the full result set.
---
Nitpick comments:
In `@mpt_api_client/http/mixins/queryable_mixin.py`:
- Around line 22-27: Create a small helper on the same class (e.g., def
_clone_query_state(self, **overrides) -> QueryState) that builds a new
QueryState by taking defaults from self.query_state (rql/filter, order_by,
select, with_render, etc.) and applying any overrides passed in; then replace
the four direct QueryState(...) constructions in this file (the places that
currently rebuild query_state with new rql/order_by/select/with_render) to call
this helper with only the changed fields (e.g., order_by=list(fields) or
rql=self.query_state.filter) so future additions to QueryState only need to be
handled in one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e60d9587-dd29-4a54-baf6-7137ca59154e
📒 Files selected for processing (6)
mpt_api_client/http/mixins/queryable_mixin.pympt_api_client/http/query_state.pytests/e2e/audit/records/test_async_records.pytests/e2e/audit/records/test_sync_records.pytests/unit/http/mixins/test_queryable_mixin.pytests/unit/http/test_query_state.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/http/test_query_state.py
- tests/e2e/audit/records/test_sync_records.py
robcsegal
force-pushed
the
MPT-18938-add-render-to-api-client-and-audit
branch
from
970ccc8 to
f71c5fd
Compare
albertsola
requested changes
Mar 12, 2026
robcsegal
force-pushed
the
MPT-18938-add-render-to-api-client-and-audit
branch
from
f71c5fd to
8747ba1
Compare
robcsegal
force-pushed
the
MPT-18938-add-render-to-api-client-and-audit
branch
from
8747ba1 to
5de0d7f
Compare
|
albertsola
approved these changes
Mar 12, 2026
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 support for a new
render()query parameter in the API client, allowing users to request rendered output in their queries. The implementation includes updates to the core query-building logic, a new method for chainingrender()into queries, and comprehensive tests to ensure correctness.Enhancements to query parameter handling:
with_renderboolean option to theQueryStateclass, allowing the inclusion of therender()parameter in the query string when building API requests. (mpt_api_client/http/query_state.py) [1] [2] [3]with_render()method inQueryableMixin, enabling method chaining to easily add therender()parameter to queries. (mpt_api_client/http/mixins/queryable_mixin.py)Testing and validation:
render()do not contain template characters and that the correct object is returned. (tests/e2e/audit/records/test_async_records.py,tests/e2e/audit/records/test_sync_records.py) [1] [2]with_render()method and for query string building with and withoutrender(). (tests/unit/http/mixins/test_queryable_mixin.py,tests/unit/http/test_query_state.py) [1] [2] [3]Closes MPT-18938