Refactor Export plugin to handle multiple article views - #82
Open
alikon wants to merge 13 commits into
Open
Conversation
Contributor
Reviewer's GuideRefactors the Export content plugin so a single JS and PHP codepath supports both single-article and bulk exports from the articles list view, adds a secured AJAX endpoint for fetching sanitized article payloads, centralizes plugin configuration/state enforcement, strengthens error handling and messaging, and introduces Cypress tests to cover the new bulk export behavior and security constraints. Sequence diagram for bulk article export flow from articles list viewsequenceDiagram
actor Admin
participant BrowserJS as Browser_JS_aexport
participant ExportPlugin as Export_onAjaxExport
participant RemoteAPI as Remote_API
Admin->>BrowserJS: click toolbar-upload
BrowserJS->>BrowserJS: fetchData(view=articles)
BrowserJS->>BrowserJS: checkCategory(options)
alt category ok
BrowserJS->>BrowserJS: processBulkExport(options)
BrowserJS->>ExportPlugin: POST onAjaxExport(ids[])
ExportPlugin-->>ExportPlugin: validate_ids(MAX_BULK_IDS)
ExportPlugin-->>ExportPlugin: loadObjectList()
ExportPlugin-->>BrowserJS: return articles_array
loop for each article
BrowserJS->>BrowserJS: checkArticle(options, article)
alt article exists remotely
BrowserJS->>RemoteAPI: PATCH patchArticle
else article missing remotely
BrowserJS->>RemoteAPI: POST postArticle
end
end
else category check failed
BrowserJS-->>Admin: showMessage(error)
end
BrowserJS-->>Admin: showMessage(PLG_CONTENT_EXPORT_BULK_COMPLETE)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Added new keys for bulk export functionality and error handling.
Owner
Author
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
onAjaxExport, users with onlycore.edit.ownoncom_contentcan currently export any selected article, not just their own; consider restricting the query with acreated_by = $user->idcondition (or filtering IDs) when onlycore.edit.ownis granted. - The
hasValidConfighelper inaexport.jsreturns normalizedapiKey,auth,getUrl, andpostUrlbutfetchDatastill uses the originaloptionsobject, so either updateoptionswith the normalized values or simplify the helper to avoid unused output. - You now have three separate sources for the bulk limit (
Export::MAX_BULK_IDS,scriptOptions['maxBulk'], and the hard-codedMAX_BULK_IDSin JS); relying solely on the server-providedmaxBulkin the client (and dropping the JS constant) would eliminate the risk of these drifting out of sync.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `onAjaxExport`, users with only `core.edit.own` on `com_content` can currently export any selected article, not just their own; consider restricting the query with a `created_by = $user->id` condition (or filtering IDs) when only `core.edit.own` is granted.
- The `hasValidConfig` helper in `aexport.js` returns normalized `apiKey`, `auth`, `getUrl`, and `postUrl` but `fetchData` still uses the original `options` object, so either update `options` with the normalized values or simplify the helper to avoid unused output.
- You now have three separate sources for the bulk limit (`Export::MAX_BULK_IDS`, `scriptOptions['maxBulk']`, and the hard-coded `MAX_BULK_IDS` in JS); relying solely on the server-provided `maxBulk` in the client (and dropping the JS constant) would eliminate the risk of these drifting out of sync.
## Individual Comments
### Comment 1
<location path="src/plugins/content/export/src/Extension/Export.php" line_range="197-201" />
<code_context>
+ throw new \Exception(Text::_('JINVALID_TOKEN'), 403);
+ }
+
+ $user = $this->app->getIdentity();
+
+ // Gate the whole endpoint behind a real ACL check: a valid CSRF
+ // token alone is not authorization to read article bodies.
+ if ($user === null || $user->guest || (!$user->authorise('core.edit', 'com_content') && !$user->authorise('core.edit.own', 'com_content'))) {
+ throw new \Exception(Text::_('JERROR_ALERTNOAUTHOR'), 403);
+ }
</code_context>
<issue_to_address>
**🚨 issue (security):** Restrict bulk export results when the user only has `core.edit.own` to avoid exposing other authors' content.
The ACL check lets both `core.edit` and `core.edit.own` reach this endpoint, but the export query returns any selected non‑trashed article:
```php
$query->whereIn($db->quoteName('id'), $ids)
->where($db->quoteName('state') . ' != -2');
```
For `core.edit.own` users this exposes full content for articles they don’t own. To match permissions, either add `AND created_by = :userId` when the user only has `core.edit.own`, or limit access to users with full `core.edit`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+197
to
+201
| $user = $this->app->getIdentity(); | ||
|
|
||
| // Gate the whole endpoint behind a real ACL check: a valid CSRF | ||
| // token alone is not authorization to read article bodies. | ||
| if ($user === null || $user->guest || (!$user->authorise('core.edit', 'com_content') && !$user->authorise('core.edit.own', 'com_content'))) { |
Contributor
There was a problem hiding this comment.
🚨 issue (security): Restrict bulk export results when the user only has core.edit.own to avoid exposing other authors' content.
The ACL check lets both core.edit and core.edit.own reach this endpoint, but the export query returns any selected non‑trashed article:
$query->whereIn($db->quoteName('id'), $ids)
->where($db->quoteName('state') . ' != -2');For core.edit.own users this exposes full content for articles they don’t own. To match permissions, either add AND created_by = :userId when the user only has core.edit.own, or limit access to users with full core.edit.
Refactor bulk article creation in Cypress test to use a loop for better clarity and manageability.
Refactor bulk export test to create articles using promises and streamline the request process.
alikon
marked this pull request as ready for review
August 3, 2026 13:53
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The bulk export ID limits are currently inconsistent: PHP uses MAX_BULK_IDS = 15, the JS fallback is 200, and the Cypress test explicitly expects a limit of 10; it would be good to centralize this limit (e.g. via scriptOptions only) and align both the client-side constant and tests with the server-side value.
- The client-side bulk limit check in processBulkExport uses
options.maxBulk || MAX_BULK_IDS, but fetchData never updatesoptionswith the normalized values returned from hasValidConfig; consider either mutatingoptionswith the validated/normalized values or returning a single configuration object to avoid subtle mismatches between validated and used settings.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The bulk export ID limits are currently inconsistent: PHP uses MAX_BULK_IDS = 15, the JS fallback is 200, and the Cypress test explicitly expects a limit of 10; it would be good to centralize this limit (e.g. via scriptOptions only) and align both the client-side constant and tests with the server-side value.
- The client-side bulk limit check in processBulkExport uses `options.maxBulk || MAX_BULK_IDS`, but fetchData never updates `options` with the normalized values returned from hasValidConfig; consider either mutating `options` with the validated/normalized values or returning a single configuration object to avoid subtle mismatches between validated and used settings.
## Individual Comments
### Comment 1
<location path="src/plugins/content/export/media/js/aexport.js" line_range="15-24" />
<code_context>
+ const MAX_BULK_IDS = 200;
</code_context>
<issue_to_address>
**issue (bug_risk):** Client-side bulk limit is out of sync with the server-side MAX_BULK_IDS, leading to confusing failures.
The JS comment notes this should match `Export::MAX_BULK_IDS`, but PHP is `15` while this client constant is `200`/`options.maxBulk`. This mismatch lets users pass the client check and then get a server-side 400 for too many IDs. Please either align these values or source the limit from PHP (e.g., via `options.maxBulk`) and drop the hardcoded `200` to avoid future drift.
</issue_to_address>
### Comment 2
<location path="src/plugins/content/export/media/js/aexport.js" line_range="275-284" />
<code_context>
- // Insert after toolbar parent container to avoid affecting button layout
const toolbarContainer = toolbar.closest('.subhead');
const insertTarget = toolbarContainer || toolbar.parentElement;
- insertTarget.insertAdjacentHTML('afterend',
+
+ insertTarget.insertAdjacentHTML('afterend',
`<div id="msg" class="alert ${alertClass}" role="alert" style="margin: 10px; border: 2px solid; border-radius: 4px;">${message}</div>`);
+
const msgBox = document.getElementById('msg');
</code_context>
<issue_to_address>
**🚨 issue (security):** Unescaped insertion of server-controlled messages into innerHTML risks XSS if messages ever contain HTML.
`showMessage` passes `localData.message` and server exception strings directly into `${message}`, which is then injected with `insertAdjacentHTML`. If those server messages ever include HTML (e.g., echoed user input), this is an XSS vector. Please either escape/sanitize `message` before injection (e.g., use a text node instead of `insertAdjacentHTML`) or restrict `showMessage` to trusted, predefined strings rather than raw server errors.
</issue_to_address>
### Comment 3
<location path="src/plugins/content/export/media/js/aexport.js" line_range="292-295" />
<code_context>
- getUrl,
- postUrl,
- };
+ if (!apiKey || !auth || !getUrl || !postUrl) {
+ return { ok: false, message: Joomla.Text._('PLG_CONTENT_EXPORT_INVALID_CONFIG_REQUIRED') };
+ }
+ return { ok: true, apiKey, auth, getUrl, postUrl };
}
</code_context>
<issue_to_address>
**suggestion:** The Bearer-token sanity check was removed; consider reintroducing it to fail fast on misconfiguration.
Previously, we explicitly rejected a `Bearer` header without a token (e.g., just `'Bearer'`) and surfaced a clear config error. Now, such a misconfigured header will be treated as valid as long as `apiKey` is non-empty, deferring the failure to later 401/403 responses. Please add a small check that, when `apiKey` starts with `'bearer'`, verifies there is a non-empty token segment before marking the config as ok.
```suggestion
if (!apiKey || !auth || !getUrl || !postUrl) {
return { ok: false, message: Joomla.Text._('PLG_CONTENT_EXPORT_INVALID_CONFIG_REQUIRED') };
}
// When using a Bearer token, ensure there is a non-empty token segment
const lowerApiKey = String(apiKey).trim().toLowerCase();
if (lowerApiKey.startsWith('bearer')) {
const token = String(apiKey).slice(6).trim(); // strip 'Bearer' prefix
if (!token) {
return { ok: false, message: Joomla.Text._('PLG_CONTENT_EXPORT_INVALID_CONFIG_REQUIRED') };
}
}
return { ok: true, apiKey, auth, getUrl, postUrl };
```
</issue_to_address>
### Comment 4
<location path="tests/cypress/integration/plugins/ExportArticle.cy.js" line_range="163-180" />
<code_context>
+ });
+ });
+
+ it('rejects a bulk export request that exceeds the configured ID limit', () => {
+ stubRemoteApi();
+
+ const articlePromises = Array.from({ length: 12 }, (_, i) =>
+ cy.db_createArticle({ title: `Test export article bulk ${i}` })
+ );
+
+ // Wait for all DB insertions to finish before visiting the page
+ cy.wrap(Promise.all(articlePromises)).then(() => {
+ cy.visit('/administrator/index.php?option=com_content&view=articles&filter=');
+ cy.searchForItem('Test export article bulk');
+ cy.checkAllResults();
+ cy.get('#toolbar-upload').click();
+
+ // Verify the error message contains the max limit threshold (10)
+ cy.get('#msg').should('contain.text', '10');
+ });
</code_context>
<issue_to_address>
**suggestion (testing):** Also assert that no AJAX bulk-export request is sent when the client-side ID limit is exceeded
This only checks the error message includes the max limit (10) but doesn’t confirm the client stops before calling the local `com_ajax` endpoint. Please also intercept `index.php?option=com_ajax&plugin=export&group=content&format=json` (as in the "no article selected" test) and assert that no such request is made when the selection exceeds the limit, so the test covers the short-circuit behaviour as well.
```suggestion
it('rejects a bulk export request that exceeds the configured ID limit', () => {
stubRemoteApi();
cy.intercept(
'POST',
'/administrator/index.php?option=com_ajax&plugin=export&group=content&format=json'
).as('exportRequest');
const articlePromises = Array.from({ length: 12 }, (_, i) =>
cy.db_createArticle({ title: `Test export article bulk ${i}` })
);
// Wait for all DB insertions to finish before visiting the page
cy.wrap(Promise.all(articlePromises)).then(() => {
cy.visit('/administrator/index.php?option=com_content&view=articles&filter=');
cy.searchForItem('Test export article bulk');
cy.checkAllResults();
cy.get('#toolbar-upload').click();
// Verify the error message contains the max limit threshold (10)
cy.get('#msg').should('contain.text', '10');
// Confirm that no bulk-export AJAX request is sent when the client-side ID limit is exceeded
cy.get('@exportRequest.all').should('have.length', 0);
});
});
```
</issue_to_address>
### Comment 5
<location path="tests/cypress/integration/plugins/ExportArticle.cy.js" line_range="30-32" />
<code_context>
+ body: {},
+ }).as('remoteCategoryCheck');
+
+ // checkArticle(): pretend the article never existed remotely, so the
+ // client always goes through postArticle() (creation).
+ cy.intercept('GET', `${remoteDomain}/api/index.php/v1/content/articles*`, {
+ statusCode: 200,
+ body: { data: [] },
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for the "update" path when the remote article already exists (PATCH flow)
Right now `stubRemoteApi()` always returns `{ data: [] }` for `GET .../articles`, so tests only cover the create (`POST`) path. Please add a test where the `GET` intercept returns at least one article (e.g. `{ data: [{ id: 123, attributes: { title: '...' } }] }`), and assert that a `PATCH` is sent to the correct URL and that the UI shows the expected updated/exported message. This ensures the refactored single-article export logic is verified for both create and update flows.
Suggested implementation:
```javascript
const stubRemoteApi = () => {
// checkCategory(): any 2xx response is treated as "category is valid".
cy.intercept('GET', `${remoteDomain}/api/index.php/v1/content/categories/*`, {
statusCode: 200,
body: {},
}).as('remoteCategoryCheck');
// checkArticle(): pretend the article never existed remotely, so the
// client always goes through postArticle() (creation).
cy.intercept('GET', `${remoteDomain}/api/index.php/v1/content/articles*`, {
statusCode: 200,
body: { data: [] },
}).as('remoteArticleSearch');
// postArticle(): pretend creation succeeded.
cy.intercept('POST', `${remoteDomain}/api/index.php/v1/content/articles`, {
statusCode: 200,
body: { data: { id: 999 } },
}).as('remoteArticleCreate');
};
// Variant of stubRemoteApi() that simulates an existing remote article so the
// client goes through the update (PATCH) flow instead of creation (POST).
const stubRemoteApiWithExistingArticle = (articleId = 123) => {
// checkCategory(): any 2xx response is treated as "category is valid".
cy.intercept('GET', `${remoteDomain}/api/index.php/v1/content/categories/*`, {
statusCode: 200,
body: {},
}).as('remoteCategoryCheck');
// checkArticle(): pretend the article already exists remotely, so the
// client goes through the patchArticle() (update) path.
cy.intercept('GET', `${remoteDomain}/api/index.php/v1/content/articles*`, {
statusCode: 200,
body: {
data: [
{
id: articleId,
attributes: {
title: 'Existing remote article',
},
},
],
},
}).as('remoteArticleSearch');
// patchArticle(): pretend update succeeded.
cy.intercept('PATCH', `${remoteDomain}/api/index.php/v1/content/articles/${articleId}`, {
statusCode: 200,
body: { data: { id: articleId } },
}).as('remoteArticleUpdate');
};
const getCsrfToken = () => cy.window().its('Joomla').invoke('getOptions', 'csrf.token');
```
To fully implement the requested coverage, you should also:
1. Add a new Cypress test that uses `stubRemoteApiWithExistingArticle()` to exercise the update flow. For example, in the same `describe` block where the create/export test lives:
- Call `stubRemoteApiWithExistingArticle()` in the `beforeEach`/test setup instead of `stubRemoteApi()`.
- Trigger the same UI action that exports the article.
- `cy.wait('@remoteArticleSearch')` to ensure the "search" request is issued.
- `cy.wait('@remoteArticleUpdate')` and assert:
- The request method is `PATCH`.
- The request URL matches `${remoteDomain}/api/index.php/v1/content/articles/123`.
- Optionally, the request body contains the expected payload (e.g. updated title/content).
- Assert that the UI shows the expected "updated/exported" success message, using the same selectors and wording pattern as the existing "created/exported" test (e.g. `cy.contains('Article successfully updated')` or whatever is used in the app).
2. Ensure the new test name and structure follow the existing conventions in `ExportArticle.cy.js`, e.g. something like:
```js
it('exports an article by updating an existing remote article via PATCH', () => {
stubRemoteApiWithExistingArticle();
// ...rest of the test...
});
```
3. If there are shared setup hooks (`before`, `beforeEach`) that currently call `stubRemoteApi()`, either:
- Override them within the new test (by calling `stubRemoteApiWithExistingArticle()` before performing the export), or
- Create a separate `describe` block for the "update" flow that uses `stubRemoteApiWithExistingArticle()` in its own `beforeEach`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+15
to
+24
| const MAX_BULK_IDS = 200; | ||
|
|
||
| toolbar.addEventListener('click', fetchData); | ||
|
|
||
| async function fetchData() { | ||
| /** | ||
| * Small sprintf-like helper so user-facing messages can stay in the | ||
| * language files instead of being hardcoded in JS. | ||
| */ | ||
| function t(key, ...args) { | ||
| const str = Joomla.Text._(key) || key; |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Client-side bulk limit is out of sync with the server-side MAX_BULK_IDS, leading to confusing failures.
The JS comment notes this should match Export::MAX_BULK_IDS, but PHP is 15 while this client constant is 200/options.maxBulk. This mismatch lets users pass the client check and then get a server-side 400 for too many IDs. Please either align these values or source the limit from PHP (e.g., via options.maxBulk) and drop the hardcoded 200 to avoid future drift.
Comment on lines
+275
to
+284
| return false; | ||
| } catch (error) { | ||
| let errorMsg = 'Network error occurred'; | ||
| if (error.name === 'TypeError' || error.message.includes('CORS')) { | ||
| errorMsg = 'CORS error: The GET method is not allowed. Add "GET" to Access-Control-Allow-Methods header on the API server.'; | ||
| } else if (error.message) { | ||
| errorMsg = error.message; | ||
| } | ||
| showMessage(errorMsg, 'error'); | ||
| console.error(`[export] patchArticle (${current}/${total}) failed:`, error); | ||
| showMessage(t('PLG_CONTENT_EXPORT_ARTICLE_UPDATE_NETWORK_ERROR', error.message), 'error'); | ||
| throw error; | ||
| } | ||
| } | ||
| function hasValidConfig(options) { | ||
| if (!options || typeof options !== 'object') { | ||
| return { ok: false, message: Joomla.Text._('PLG_CONTENT_EXPORT_INVALID_CONFIG_OBJECT') }; | ||
| } | ||
|
|
||
| // Coerce to string safely | ||
| const apiKey = String(options.apiKey ?? '').trim(); | ||
| const auth = String(options.auth ?? '').trim(); | ||
| const getUrl = String(options.get ?? '').trim(); | ||
| const postUrl = String(options.post ?? '').trim(); | ||
|
|
||
| const hasApiKey = apiKey.length > 0; | ||
| const hasAuth = auth.length > 0; | ||
| const hasGet = getUrl.length > 0; | ||
| const hasPost = postUrl.length > 0; | ||
|
|
||
| if (!hasApiKey || !hasAuth || !hasGet || !hasPost) { | ||
| return { | ||
| ok: false, | ||
| message: 'Invalid configuration: API key, auth, GET and POST URLs are required.', | ||
| }; | ||
| } | ||
|
|
||
| // Detect a bare Bearer header with no token | ||
| if (apiKey.toLowerCase().startsWith('bearer')) { | ||
| const bearerParts = apiKey.split(/\s+/).filter(Boolean); // ["Bearer", "token"] or ["Bearer"] | ||
| if (bearerParts.length < 2 || bearerParts[1].length === 0) { | ||
| return { | ||
| ok: false, | ||
| message: 'Invalid configuration: Bearer token is missing a value.', | ||
| }; | ||
| } | ||
| } | ||
| function hasValidConfig(options) { | ||
| if (!options || typeof options !== 'object') { |
Contributor
There was a problem hiding this comment.
🚨 issue (security): Unescaped insertion of server-controlled messages into innerHTML risks XSS if messages ever contain HTML.
showMessage passes localData.message and server exception strings directly into ${message}, which is then injected with insertAdjacentHTML. If those server messages ever include HTML (e.g., echoed user input), this is an XSS vector. Please either escape/sanitize message before injection (e.g., use a text node instead of insertAdjacentHTML) or restrict showMessage to trusted, predefined strings rather than raw server errors.
Comment on lines
+292
to
+295
| if (!apiKey || !auth || !getUrl || !postUrl) { | ||
| return { ok: false, message: Joomla.Text._('PLG_CONTENT_EXPORT_INVALID_CONFIG_REQUIRED') }; | ||
| } | ||
| return { ok: true, apiKey, auth, getUrl, postUrl }; |
Contributor
There was a problem hiding this comment.
suggestion: The Bearer-token sanity check was removed; consider reintroducing it to fail fast on misconfiguration.
Previously, we explicitly rejected a Bearer header without a token (e.g., just 'Bearer') and surfaced a clear config error. Now, such a misconfigured header will be treated as valid as long as apiKey is non-empty, deferring the failure to later 401/403 responses. Please add a small check that, when apiKey starts with 'bearer', verifies there is a non-empty token segment before marking the config as ok.
Suggested change
| if (!apiKey || !auth || !getUrl || !postUrl) { | |
| return { ok: false, message: Joomla.Text._('PLG_CONTENT_EXPORT_INVALID_CONFIG_REQUIRED') }; | |
| } | |
| return { ok: true, apiKey, auth, getUrl, postUrl }; | |
| if (!apiKey || !auth || !getUrl || !postUrl) { | |
| return { ok: false, message: Joomla.Text._('PLG_CONTENT_EXPORT_INVALID_CONFIG_REQUIRED') }; | |
| } | |
| // When using a Bearer token, ensure there is a non-empty token segment | |
| const lowerApiKey = String(apiKey).trim().toLowerCase(); | |
| if (lowerApiKey.startsWith('bearer')) { | |
| const token = String(apiKey).slice(6).trim(); // strip 'Bearer' prefix | |
| if (!token) { | |
| return { ok: false, message: Joomla.Text._('PLG_CONTENT_EXPORT_INVALID_CONFIG_REQUIRED') }; | |
| } | |
| } | |
| return { ok: true, apiKey, auth, getUrl, postUrl }; |
Comment on lines
+163
to
+180
| it('rejects a bulk export request that exceeds the configured ID limit', () => { | ||
| stubRemoteApi(); | ||
|
|
||
| const articlePromises = Array.from({ length: 12 }, (_, i) => | ||
| cy.db_createArticle({ title: `Test export article bulk ${i}` }) | ||
| ); | ||
|
|
||
| // Wait for all DB insertions to finish before visiting the page | ||
| cy.wrap(Promise.all(articlePromises)).then(() => { | ||
| cy.visit('/administrator/index.php?option=com_content&view=articles&filter='); | ||
| cy.searchForItem('Test export article bulk'); | ||
| cy.checkAllResults(); | ||
| cy.get('#toolbar-upload').click(); | ||
|
|
||
| // Verify the error message contains the max limit threshold (10) | ||
| cy.get('#msg').should('contain.text', '10'); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
suggestion (testing): Also assert that no AJAX bulk-export request is sent when the client-side ID limit is exceeded
This only checks the error message includes the max limit (10) but doesn’t confirm the client stops before calling the local com_ajax endpoint. Please also intercept index.php?option=com_ajax&plugin=export&group=content&format=json (as in the "no article selected" test) and assert that no such request is made when the selection exceeds the limit, so the test covers the short-circuit behaviour as well.
Suggested change
| it('rejects a bulk export request that exceeds the configured ID limit', () => { | |
| stubRemoteApi(); | |
| const articlePromises = Array.from({ length: 12 }, (_, i) => | |
| cy.db_createArticle({ title: `Test export article bulk ${i}` }) | |
| ); | |
| // Wait for all DB insertions to finish before visiting the page | |
| cy.wrap(Promise.all(articlePromises)).then(() => { | |
| cy.visit('/administrator/index.php?option=com_content&view=articles&filter='); | |
| cy.searchForItem('Test export article bulk'); | |
| cy.checkAllResults(); | |
| cy.get('#toolbar-upload').click(); | |
| // Verify the error message contains the max limit threshold (10) | |
| cy.get('#msg').should('contain.text', '10'); | |
| }); | |
| }); | |
| it('rejects a bulk export request that exceeds the configured ID limit', () => { | |
| stubRemoteApi(); | |
| cy.intercept( | |
| 'POST', | |
| '/administrator/index.php?option=com_ajax&plugin=export&group=content&format=json' | |
| ).as('exportRequest'); | |
| const articlePromises = Array.from({ length: 12 }, (_, i) => | |
| cy.db_createArticle({ title: `Test export article bulk ${i}` }) | |
| ); | |
| // Wait for all DB insertions to finish before visiting the page | |
| cy.wrap(Promise.all(articlePromises)).then(() => { | |
| cy.visit('/administrator/index.php?option=com_content&view=articles&filter='); | |
| cy.searchForItem('Test export article bulk'); | |
| cy.checkAllResults(); | |
| cy.get('#toolbar-upload').click(); | |
| // Verify the error message contains the max limit threshold (10) | |
| cy.get('#msg').should('contain.text', '10'); | |
| // Confirm that no bulk-export AJAX request is sent when the client-side ID limit is exceeded | |
| cy.get('@exportRequest.all').should('have.length', 0); | |
| }); | |
| }); |
Comment on lines
+30
to
+32
| // checkArticle(): pretend the article never existed remotely, so the | ||
| // client always goes through postArticle() (creation). | ||
| cy.intercept('GET', `${remoteDomain}/api/index.php/v1/content/articles*`, { |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add coverage for the "update" path when the remote article already exists (PATCH flow)
Right now stubRemoteApi() always returns { data: [] } for GET .../articles, so tests only cover the create (POST) path. Please add a test where the GET intercept returns at least one article (e.g. { data: [{ id: 123, attributes: { title: '...' } }] }), and assert that a PATCH is sent to the correct URL and that the UI shows the expected updated/exported message. This ensures the refactored single-article export logic is verified for both create and update flows.
Suggested implementation:
const stubRemoteApi = () => {
// checkCategory(): any 2xx response is treated as "category is valid".
cy.intercept('GET', `${remoteDomain}/api/index.php/v1/content/categories/*`, {
statusCode: 200,
body: {},
}).as('remoteCategoryCheck');
// checkArticle(): pretend the article never existed remotely, so the
// client always goes through postArticle() (creation).
cy.intercept('GET', `${remoteDomain}/api/index.php/v1/content/articles*`, {
statusCode: 200,
body: { data: [] },
}).as('remoteArticleSearch');
// postArticle(): pretend creation succeeded.
cy.intercept('POST', `${remoteDomain}/api/index.php/v1/content/articles`, {
statusCode: 200,
body: { data: { id: 999 } },
}).as('remoteArticleCreate');
};
// Variant of stubRemoteApi() that simulates an existing remote article so the
// client goes through the update (PATCH) flow instead of creation (POST).
const stubRemoteApiWithExistingArticle = (articleId = 123) => {
// checkCategory(): any 2xx response is treated as "category is valid".
cy.intercept('GET', `${remoteDomain}/api/index.php/v1/content/categories/*`, {
statusCode: 200,
body: {},
}).as('remoteCategoryCheck');
// checkArticle(): pretend the article already exists remotely, so the
// client goes through the patchArticle() (update) path.
cy.intercept('GET', `${remoteDomain}/api/index.php/v1/content/articles*`, {
statusCode: 200,
body: {
data: [
{
id: articleId,
attributes: {
title: 'Existing remote article',
},
},
],
},
}).as('remoteArticleSearch');
// patchArticle(): pretend update succeeded.
cy.intercept('PATCH', `${remoteDomain}/api/index.php/v1/content/articles/${articleId}`, {
statusCode: 200,
body: { data: { id: articleId } },
}).as('remoteArticleUpdate');
};
const getCsrfToken = () => cy.window().its('Joomla').invoke('getOptions', 'csrf.token');To fully implement the requested coverage, you should also:
-
Add a new Cypress test that uses
stubRemoteApiWithExistingArticle()to exercise the update flow. For example, in the samedescribeblock where the create/export test lives:- Call
stubRemoteApiWithExistingArticle()in thebeforeEach/test setup instead ofstubRemoteApi(). - Trigger the same UI action that exports the article.
cy.wait('@remoteArticleSearch')to ensure the "search" request is issued.cy.wait('@remoteArticleUpdate')and assert:- The request method is
PATCH. - The request URL matches
${remoteDomain}/api/index.php/v1/content/articles/123. - Optionally, the request body contains the expected payload (e.g. updated title/content).
- The request method is
- Assert that the UI shows the expected "updated/exported" success message, using the same selectors and wording pattern as the existing "created/exported" test (e.g.
cy.contains('Article successfully updated')or whatever is used in the app).
- Call
-
Ensure the new test name and structure follow the existing conventions in
ExportArticle.cy.js, e.g. something like:it('exports an article by updating an existing remote article via PATCH', () => { stubRemoteApiWithExistingArticle(); // ...rest of the test... });
-
If there are shared setup hooks (
before,beforeEach) that currently callstubRemoteApi(), either:- Override them within the new test (by calling
stubRemoteApiWithExistingArticle()before performing the export), or - Create a separate
describeblock for the "update" flow that usesstubRemoteApiWithExistingArticle()in its ownbeforeEach.
- Override them within the new test (by calling
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
1 participant
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 Sourcery
Refactor the Export content plugin to support both single-article and bulk export from the articles list, with stricter validation and safer server-side handling.
New Features:
Enhancements:
Tests: