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 a workflow (.github/workflows/front-docs.yml) that generates an OpenAPI JSON via PHP/Composer and vendor/bin/openapi, uploads it as an artifact, and conditionally pushes an updated openapi.json to phpList/web-frontend. Tightens .github/workflows/client-docs.yml triggers. Adds PaginatedDataProvider injection and GET /subscribe-pages/ in SubscribePageController, introduces an optional structured Sequence Diagram(s)sequenceDiagram
participant Actions as GitHubActions/generate-openapi
participant PHP as PHP+Composer+openapi
participant Artifact as ActionsArtifact
participant WebFrontend as phpList/web-frontend
Actions->>PHP: run vendor/bin/openapi -> docs/latest-restapi.json
PHP->>Artifact: upload docs/latest-restapi.json
Actions->>WebFrontend: checkout target branch
Actions->>Artifact: download latest-restapi.json
Actions->>WebFrontend: compare latest-restapi.json with openapi.json
Actions->>WebFrontend: commit & push updated openapi.json (if different)
sequenceDiagram
participant Client as API Client
participant Controller as SubscribePageController
participant Provider as PaginatedDataProvider
participant Manager as SubscribePageManager
participant DB as Database
Client->>Controller: GET /subscribe-pages?cursor...
Controller->>Provider: getPaginatedList(PaginatedFilter)
Provider->>DB: query SubscribePage with pagination
DB-->>Provider: paginated rows + cursor
Provider-->>Controller: Paginated result (items, pagination)
Controller-->>Client: 200 { items: [...], pagination: {...} }
Client->>Controller: POST /subscribe-pages { data: [...] }
Controller->>Manager: create page entity
Manager->>DB: persist page
DB-->>Manager: page id
Controller->>Manager: syncPageData(page, dataMap) (if data provided)
Manager->>DB: upsert page data entries
Manager-->>Controller: ack
Controller-->>Client: 201 Created
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
…ent-docs to specific branches
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
.github/workflows/client-docs.yml (1)
1-121: Advisory: Apply the same security hardening as front-docs.yml.This workflow shares the same structure and security vulnerabilities as the newly added
front-docs.yml:
- Template injection (lines 22, 24):
github.head_refandgithub.ref_nameexpanded directly in shell- Unpinned actions (lines 28, 31, 37, 51, 63, 81): Using mutable tags instead of commit SHAs
- Missing permissions block: Runs with broad default permissions
- Credential persistence (lines 28, 63): Checkout actions don't set
persist-credentials: falseWhile these are pre-existing issues, consider applying the same fixes proposed for
front-docs.ymlto maintain consistent security posture across both documentation workflows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/client-docs.yml around lines 1 - 121, The Determine source branch step uses untrusted shell expansion of github.head_ref and github.ref_name (template injection) and several actions are unpinned and keep credentials; fix by: (1) replace the shell-based output assignment in the "Determine source branch" step with GitHub expression-based outputs (use the official event expressions like github.event.pull_request.head.ref or github.ref_name via the step outputs syntax) so no untrusted expansion occurs in run; (2) pin all third-party actions referenced (actions/checkout, shivammathur/setup-php, actions/cache, actions/upload-artifact, actions/download-artifact, etc.) to immutable commit SHAs instead of floating tags; (3) add a top-level permissions block with minimal required rights; and (4) set persist-credentials: false on both checkout steps ("Checkout Source Repository" and "Checkout phplist-api-client Repository") to avoid credential persistence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/front-docs.yml:
- Around line 26-28: The checkout steps using actions/checkout@v3 are leaving
credentials in .git/config; update both checkout steps (the initial "Checkout
Source Repository" and the later checkout that uses the PUSH_WEB_FRONTEND token)
to add persist-credentials: false to the action inputs so credentials are not
persisted, and ensure the subsequent commit/push logic (where you reconfigure
git credentials) remains intact to set credentials only when needed.
- Around line 1-11: Add an explicit top-level permissions block to this workflow
(named "Update phplist-web-frontend OpenAPI") to enforce least-privilege for
GITHUB_TOKEN; replace the default broad permissions with explicit, minimal
permissions required by the job(s) (for example, set contents: read and only
include write permissions for specific scopes actually needed like
pull-requests: write or workflows: write if the job updates PRs or workflow
files), placing the permissions: block directly under the workflow header so it
applies to all jobs.
- Around line 17-24: The Determine source branch step is vulnerable because it
injects github.head_ref/github.ref_name directly into a shell command; replace
this run block with a non-shell implementation (e.g., use actions/github-script
or an official action) that reads the branch from the GitHub context and sets
the step output via the Actions toolkit (e.g., core.setOutput('source_branch',
branch)) to avoid shell interpolation; locate the step with id "branch" / name
"Determine source branch" and remove direct echo of ${{ github.head_ref }} and
${{ github.ref_name }} in shell commands.
- Line 27: The workflow uses mutable action tags (e.g., the uses entry
"actions/checkout@v3") which must be replaced with immutable commit SHAs to
prevent supply-chain risks; update every "uses: actions/checkout@v3" occurrence
to pin to de0fac2e4500dabe0009e67214ff5f5447ce83dd, and similarly replace the
other mutable usages mentioned (shivammathur/setup-php, actions/cache,
actions/upload-artifact, actions/download-artifact) with the recommended commit
SHAs (7c071dfe9dc99bdf297fa79cb49ea005b9fcadbc,
27d5ce7f107fe9357f9df03efb73ab90386fccae,
043fb46d1a93c77aae656e7c1c64a875d1fc6a0a,
3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c respectively), ensuring all “uses:”
lines in the workflow file are updated to those exact SHAs.
In `@composer.json`:
- Line 45: The composer entry "phplist/core": "dev-dev" appears to track the dev
branch (not main) — if the PR intends to follow the stable main line change it
back to "dev-main"; otherwise explicitly confirm and document the intent to
track the less-stable dev branch. Update the composer.json dependency for
phplist/core (the "phplist/core" version string) to "dev-main" for
consistency/stability or add a short comment/PR description explaining why
"dev-dev" (dev) is intentionally required.
In `@src/Subscription/Controller/SubscribePageController.php`:
- Line 59: The OpenAPI schema for the after_id parameter in
SubscribePageController (schema: new OA\Schema(...)) incorrectly sets minimum
and default to 1; change the schema so it allows 0 by setting minimum to 0 and
default to 0 (or remove the minimum/default constraint) so cursor pagination
starting at 0 is valid; update the OA\Schema for after_id accordingly.
---
Nitpick comments:
In @.github/workflows/client-docs.yml:
- Around line 1-121: The Determine source branch step uses untrusted shell
expansion of github.head_ref and github.ref_name (template injection) and
several actions are unpinned and keep credentials; fix by: (1) replace the
shell-based output assignment in the "Determine source branch" step with GitHub
expression-based outputs (use the official event expressions like
github.event.pull_request.head.ref or github.ref_name via the step outputs
syntax) so no untrusted expansion occurs in run; (2) pin all third-party actions
referenced (actions/checkout, shivammathur/setup-php, actions/cache,
actions/upload-artifact, actions/download-artifact, etc.) to immutable commit
SHAs instead of floating tags; (3) add a top-level permissions block with
minimal required rights; and (4) set persist-credentials: false on both checkout
steps ("Checkout Source Repository" and "Checkout phplist-api-client
Repository") to avoid credential persistence.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7eaa983b-b065-4955-8333-f00eca6c5837
📒 Files selected for processing (5)
.github/workflows/client-docs.yml.github/workflows/front-docs.ymlcomposer.jsonsrc/PhpListRestBundle.phpsrc/Subscription/Controller/SubscribePageController.php
✅ Files skipped from review due to trivial changes (1)
- src/PhpListRestBundle.php
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/Integration/Subscription/Controller/SubscribePageControllerTest.php (2)
89-92: ⚡ Quick winAssert returned
datato lock in the new API contract.These tests now send
data, but they still don’t assert the response includes normalizeddata(key/value). Adding that assertion would prevent silent regressions in serializer/controller wiring.Also applies to: 155-157
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Integration/Subscription/Controller/SubscribePageControllerTest.php` around lines 89 - 92, Add assertions to SubscribePageControllerTest to verify the response body includes a normalized "data" array with objects containing "key" and "value" entries matching the input (e.g. key "intro_text" with value "Welcome"); update the assertions in the test block where the request JSON is built (the snippet around the JSON_THROW_ON_ERROR call) and the second occurrence (lines referenced 155-157) so both response checks assert the returned "data" structure (key/value) to lock in the new API contract.
126-143: ⚡ Quick winStrengthen the 422 test with field-level error assertions.
Right now it only checks status. Please also assert the validation payload references the missing
data[0].valuefield, so it fails for the right reason.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Integration/Subscription/Controller/SubscribePageControllerTest.php` around lines 126 - 143, In the testCreateSubscribePageWithDataMissingValueReturnsUnprocessableEntity test, enhance the 422 assertion to check the validation payload contains a field-level error for the missing data[0].value; after calling authenticatedJsonRequest and assertHttpUnprocessableEntity(), decode the JSON response (or use existing test helper to get response body) and assert that the validation errors include an entry for "data.0.value" or "data[0].value" (depending on project convention) and that the message/constraint is present, so the failure is confirmed to be due to the missing value field rather than a different validation error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Subscription/Controller/SubscribePageController.php`:
- Around line 233-237: The create flow currently calls
$this->entityManager->flush() before calling
$this->subscribePageManager->syncPageData(...), risking partial commits if
syncPageData throws; change the flow so create and data sync are executed
atomically: remove the early flush and perform the flush only after syncPageData
completes, or wrap the create + sync sequence in a single DB transaction (use
the EntityManager transaction API or a transactional helper) so that
SubscribePageController's create path (checking createRequest->hasData(),
calling subscribePageManager->syncPageData(createRequest->getDataMap(), $page),
then $this->entityManager->flush()) is all committed or rolled back together.
In `@src/Subscription/Serializer/SubscribePageNormalizer.php`:
- Around line 28-31: Constructor for SubscribePageNormalizer now requires a
SubscribePageDataNormalizer in addition to AdministratorNormalizer; update all
test and manual instantiations (e.g., in SubscribePageNormalizerTest and any
places using new SubscribePageNormalizer(...)) to provide the new dependency.
Replace calls like new SubscribePageNormalizer($adminNormalizer) with new
SubscribePageNormalizer($adminNormalizer, $dataNormalizer), and in tests create
a suitable stub/mock for SubscribePageDataNormalizer (or a real instance) in
setUp or inline using your test framework's createMock/createStub method so the
constructor receives both AdministratorNormalizer and
SubscribePageDataNormalizer.
---
Nitpick comments:
In `@tests/Integration/Subscription/Controller/SubscribePageControllerTest.php`:
- Around line 89-92: Add assertions to SubscribePageControllerTest to verify the
response body includes a normalized "data" array with objects containing "key"
and "value" entries matching the input (e.g. key "intro_text" with value
"Welcome"); update the assertions in the test block where the request JSON is
built (the snippet around the JSON_THROW_ON_ERROR call) and the second
occurrence (lines referenced 155-157) so both response checks assert the
returned "data" structure (key/value) to lock in the new API contract.
- Around line 126-143: In the
testCreateSubscribePageWithDataMissingValueReturnsUnprocessableEntity test,
enhance the 422 assertion to check the validation payload contains a field-level
error for the missing data[0].value; after calling authenticatedJsonRequest and
assertHttpUnprocessableEntity(), decode the JSON response (or use existing test
helper to get response body) and assert that the validation errors include an
entry for "data.0.value" or "data[0].value" (depending on project convention)
and that the message/constraint is present, so the failure is confirmed to be
due to the missing value field rather than a different validation error.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e3fbcab1-b459-40cd-a80c-b5481b4cfbdb
📒 Files selected for processing (5)
src/Subscription/Controller/SubscribePageController.phpsrc/Subscription/Request/SubscribePageRequest.phpsrc/Subscription/Serializer/SubscribePageDataNormalizer.phpsrc/Subscription/Serializer/SubscribePageNormalizer.phptests/Integration/Subscription/Controller/SubscribePageControllerTest.php
TatevikGr
force-pushed
the
dev
branch
2 times, most recently
from
f449fa6 to
a455314
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Subscription/Controller/SubscribePageController.php (1)
121-126:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOpenAPI auth requirement doesn’t match
getPageruntime behavior.Line 156 allows requests with no admin, and Line 159 only blocks inactive pages for anonymous users. But the schema still marks
php-auth-pwas required. This will mislead clients and generated SDKs.📌 Suggested docs fix
new OA\Parameter( name: 'php-auth-pw', description: 'Session key obtained from login', in: 'header', - required: true, + required: false, schema: new OA\Schema(type: 'string') ),Also applies to: 154-160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Subscription/Controller/SubscribePageController.php` around lines 121 - 126, The OpenAPI parameter declaration for the header 'php-auth-pw' in SubscribePageController (used by getPage) incorrectly sets required: true while runtime accepts anonymous requests; change the OA\Parameter for name 'php-auth-pw' to required: false (or remove the required flag) and update the description to indicate the header is optional and only needed for admin-authenticated behavior / access to inactive pages so generated clients are not forced to supply it.
🧹 Nitpick comments (2)
tests/Integration/Subscription/Controller/SubscribePageControllerTest.php (1)
83-109: ⚡ Quick winAdd assertions for the new
dataresponse contract.These tests now submit
data, but they don’t assert that returned payload includes the expected normalizeddataarray. Adding that check would catch regressions in sync/serialization immediately.Also applies to: 145-168
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Integration/Subscription/Controller/SubscribePageControllerTest.php` around lines 83 - 109, Update the testCreateSubscribePageWithSessionCreatesPage (and the other test at lines 145-168) to assert the returned payload includes the normalized "data" array: verify $data['data'] exists and is an array, assert its count and that it contains an entry with 'key' => 'intro_text' and 'value' => 'Welcome' (or the expected normalized form), and assert each item has the expected keys ('key','value') to ensure serialization/syncing is correct; locate assertions in SubscribePageControllerTest methods to add these checks near the other response assertions (id,title,active,owner).tests/Unit/Subscription/Serializer/SubscribePageNormalizerTest.php (1)
29-66: ⚡ Quick winPlease add one happy-path unit test for non-empty page data normalization.
Current test only covers empty
data. A case with one/twoSubscribePageDataitems (and assertedSubscribePageDataNormalizercalls/output) would lock down the new behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Subscription/Serializer/SubscribePageNormalizerTest.php` around lines 29 - 66, Add a second "happy path" unit test in SubscribePageNormalizerTest that verifies non-empty page data is normalized: create one or two mocked SubscribePageData objects, stub SubscribePage::getData() to return them, mock SubscribePageDataNormalizer::normalize() to be called with each SubscribePageData and return sample arrays, construct the SubscribePageNormalizer with the mocked AdministratorNormalizer and SubscribePageDataNormalizer, call SubscribePageNormalizer::normalize($page) and assert the resulting 'data' field contains the returned arrays in order (and other fields remain as in the existing test).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/Subscription/Controller/SubscribePageController.php`:
- Around line 121-126: The OpenAPI parameter declaration for the header
'php-auth-pw' in SubscribePageController (used by getPage) incorrectly sets
required: true while runtime accepts anonymous requests; change the OA\Parameter
for name 'php-auth-pw' to required: false (or remove the required flag) and
update the description to indicate the header is optional and only needed for
admin-authenticated behavior / access to inactive pages so generated clients are
not forced to supply it.
---
Nitpick comments:
In `@tests/Integration/Subscription/Controller/SubscribePageControllerTest.php`:
- Around line 83-109: Update the testCreateSubscribePageWithSessionCreatesPage
(and the other test at lines 145-168) to assert the returned payload includes
the normalized "data" array: verify $data['data'] exists and is an array, assert
its count and that it contains an entry with 'key' => 'intro_text' and 'value'
=> 'Welcome' (or the expected normalized form), and assert each item has the
expected keys ('key','value') to ensure serialization/syncing is correct; locate
assertions in SubscribePageControllerTest methods to add these checks near the
other response assertions (id,title,active,owner).
In `@tests/Unit/Subscription/Serializer/SubscribePageNormalizerTest.php`:
- Around line 29-66: Add a second "happy path" unit test in
SubscribePageNormalizerTest that verifies non-empty page data is normalized:
create one or two mocked SubscribePageData objects, stub
SubscribePage::getData() to return them, mock
SubscribePageDataNormalizer::normalize() to be called with each
SubscribePageData and return sample arrays, construct the
SubscribePageNormalizer with the mocked AdministratorNormalizer and
SubscribePageDataNormalizer, call SubscribePageNormalizer::normalize($page) and
assert the resulting 'data' field contains the returned arrays in order (and
other fields remain as in the existing test).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a76a901f-a181-40d9-bdb6-24c636a6c1b4
📒 Files selected for processing (6)
src/Subscription/Controller/SubscribePageController.phpsrc/Subscription/Request/SubscribePageRequest.phpsrc/Subscription/Serializer/SubscribePageDataNormalizer.phpsrc/Subscription/Serializer/SubscribePageNormalizer.phptests/Integration/Subscription/Controller/SubscribePageControllerTest.phptests/Unit/Subscription/Serializer/SubscribePageNormalizerTest.php
TatevikGr
force-pushed
the
dev
branch
2 times, most recently
from
ffc949c to
980696c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/Integration/Subscription/Controller/SubscribePageControllerTest.php (1)
145-168: ⚡ Quick winConsider asserting
datais present in the response.The test sends a
datapayload but doesn't verify the response includes it. Given the normalizer now emitsdata, addingself::assertArrayHasKey('data', $data)would confirm end-to-end integration. Same applies to the create success test above.♻️ Suggested assertion
self::assertSame('updated-page@example.org', $data['title']); self::assertFalse($data['active']); self::assertIsArray($data['owner']); + self::assertArrayHasKey('data', $data); + self::assertIsArray($data['data']); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Integration/Subscription/Controller/SubscribePageControllerTest.php` around lines 145 - 168, Add an assertion that the response includes the emitted "data" key to the test method testUpdateSubscribePageWithSessionReturnsOk: after decoding the JSON ($data) add self::assertArrayHasKey('data', $data) (and optionally assert its shape/contents, e.g. assertIsArray($data['data']) or check the expected key/value), and make the same addition to the create-success test referenced above so both end-to-end flows verify the normalizer's "data" output.src/Subscription/Controller/SubscriberListController.php (1)
185-190: 💤 Low valueConsider: Update description to clarify this is a public (unauthenticated) endpoint.
The description and summary are copy-pasted from the authenticated
getListendpoint. A small tweak would help API consumers understand this is intentionally unauthenticated.📝 Suggested tweak
#[OA\Get( path: '/api/v2/lists/{listId}/public', description: '🚧 **Status: Beta** – This method is under development. Avoid using in production. ' . - 'Returns a single subscriber list with specified ID.', - summary: 'Gets a subscriber list.', + 'Returns basic public information (id, name, description) for a subscriber list. No authentication required.', + summary: 'Gets public subscriber list info (unauthenticated).', tags: ['lists'],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Subscription/Controller/SubscriberListController.php` around lines 185 - 190, Update the OpenAPI Get annotation for the public list endpoint in SubscriberListController (the OA\Get with path '/api/v2/lists/{listId}/public') to explicitly state this is a public, unauthenticated endpoint; modify the description and/or summary to mention "public (unauthenticated) access" and that it intentionally omits auth requirements so consumers understand it differs from the authenticated getList endpoint.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Subscription/Controller/SubscriberListController.php`:
- Around line 200-221: The OA\MediaType instantiation inside the OA\Response is
missing the required mediaType property and should be replaced with
OA\JsonContent as used elsewhere; update the responses block in
SubscriberListController (the OA\Response that currently constructs new
OA\MediaType) to use new OA\JsonContent(...) and move the OA\Schema
(properties/type) into that OA\JsonContent call so the response body is
correctly typed for 'application/json' (replace references to OA\MediaType with
OA\JsonContent in the same OA\Response block where OA\Response, OA\Schema and
OA\Property are used).
---
Nitpick comments:
In `@src/Subscription/Controller/SubscriberListController.php`:
- Around line 185-190: Update the OpenAPI Get annotation for the public list
endpoint in SubscriberListController (the OA\Get with path
'/api/v2/lists/{listId}/public') to explicitly state this is a public,
unauthenticated endpoint; modify the description and/or summary to mention
"public (unauthenticated) access" and that it intentionally omits auth
requirements so consumers understand it differs from the authenticated getList
endpoint.
In `@tests/Integration/Subscription/Controller/SubscribePageControllerTest.php`:
- Around line 145-168: Add an assertion that the response includes the emitted
"data" key to the test method testUpdateSubscribePageWithSessionReturnsOk: after
decoding the JSON ($data) add self::assertArrayHasKey('data', $data) (and
optionally assert its shape/contents, e.g. assertIsArray($data['data']) or check
the expected key/value), and make the same addition to the create-success test
referenced above so both end-to-end flows verify the normalizer's "data" output.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d4ddaf96-2308-4a6c-9b89-bba1c6ca3f8d
📒 Files selected for processing (3)
src/Subscription/Controller/SubscribePageController.phpsrc/Subscription/Controller/SubscriberListController.phptests/Integration/Subscription/Controller/SubscribePageControllerTest.php
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.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
Thanks for contributing to phpList!