Validate configuration fields in fetchData#34
Merged
Conversation
Added validation for required configuration fields in fetchData function.
Reviewer's GuideAdds upfront configuration validation to fetchData and refactors messaging and error handling across export flows to provide clearer, typed alerts and prevent operations when required data is missing. Sequence diagram for fetchData configuration validation and export flowsequenceDiagram
actor User
participant Browser
participant fetchData
participant hasValidConfig
participant checkCategory
participant checkArticle
participant postArticle
participant patchArticle
participant ApiServer
User->>Browser: Click export button
Browser->>fetchData: fetchData()
fetchData->>Browser: window.Joomla.getOptions a_export
fetchData->>hasValidConfig: hasValidConfig(options)
hasValidConfig-->>fetchData: { ok, message, apiKey, auth, getUrl, postUrl }
alt invalid configuration
fetchData->>Browser: showMessage(message, error)
fetchData-->>User: Display configuration error alert
else valid configuration
fetchData->>Browser: Insert loader spinner
fetchData->>checkCategory: checkCategory(options)
checkCategory->>ApiServer: GET category
alt category check ok
ApiServer-->>checkCategory: 2xx response with data
checkCategory->>Browser: showMessage(Category check, success)
checkCategory-->>fetchData: true
fetchData->>checkArticle: checkArticle(options)
checkArticle->>ApiServer: GET article
alt article not found
ApiServer-->>checkArticle: 2xx response, empty data
checkArticle->>postArticle: postArticle(options)
postArticle->>Browser: validate article fields
alt missing title or catid
postArticle->>Browser: showMessage(Cannot create article..., error)
postArticle-->>checkArticle: false
else valid article fields
postArticle->>ApiServer: POST article
ApiServer-->>postArticle: response
alt POST ok
postArticle->>Browser: showMessage(Article created, success)
postArticle-->>checkArticle: true
else POST error
postArticle->>Browser: showMessage(Article not created..., error)
postArticle-->>checkArticle: false
end
end
else article found
ApiServer-->>checkArticle: 2xx response with data
checkArticle->>patchArticle: patchArticle(options, articleId)
patchArticle->>Browser: validate article title
alt missing title
patchArticle->>Browser: showMessage(Cannot update article..., error)
patchArticle-->>checkArticle: false
else valid title
patchArticle->>ApiServer: PATCH article
ApiServer-->>patchArticle: response
alt PATCH ok
patchArticle->>Browser: showMessage(Article exported, success)
patchArticle-->>checkArticle: true
else PATCH error
patchArticle->>Browser: showMessage(status text, error)
patchArticle-->>checkArticle: false
end
end
end
checkArticle-->>fetchData: done
else category check error
ApiServer-->>checkCategory: error or non 2xx
checkCategory->>Browser: showMessage(Category check: status..., error)
checkCategory-->>fetchData: false
end
fetchData->>Browser: hide loader spinner
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The validation block in
fetchDatais getting dense and hard to read; consider extracting it into a small helper (e.g.,hasValidConfig(options)) or at least breaking the conditions into named boolean variables for each field to make intent clearer. - Using
trim()onoptions.apiKey,options.auth,options.get, andoptions.postassumes they are always strings; it may be safer to coerce to string or guard withtypeof === 'string'to avoid runtime errors when options are missing or of unexpected types. - The specific check
options.apiKey === 'Bearer 'is quite narrow; if the goal is to prevent bare Bearer tokens without a value, you might want to validate with something likeoptions.apiKey.startsWith('Bearer ') && options.apiKey.trim() === 'Bearer'or similar, and potentially surface a more specific error message for this case.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The validation block in `fetchData` is getting dense and hard to read; consider extracting it into a small helper (e.g., `hasValidConfig(options)`) or at least breaking the conditions into named boolean variables for each field to make intent clearer.
- Using `trim()` on `options.apiKey`, `options.auth`, `options.get`, and `options.post` assumes they are always strings; it may be safer to coerce to string or guard with `typeof === 'string'` to avoid runtime errors when options are missing or of unexpected types.
- The specific check `options.apiKey === 'Bearer '` is quite narrow; if the goal is to prevent bare Bearer tokens without a value, you might want to validate with something like `options.apiKey.startsWith('Bearer ') && options.apiKey.trim() === 'Bearer'` or similar, and potentially surface a more specific error message for this case.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
fetchData,hasValidConfigreturns normalizedapiKey/auth/getUrl/postUrlbut you never use them to updateoptions, so the subsequent calls still rely on the original (possibly untrimmed/invalid) values; consider either mutatingoptionsor passing the normalized values through. - The new CORS handling branches use
error.message.includes('CORS')without first checking thaterror.messageis a non-empty string, which can throw for unexpected error shapes; guard this with a type check before calling.includes. - The added
console.log('options', options);infetchDatalooks like debugging output and may be noisy in production—consider removing it or wrapping it behind a debug flag.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `fetchData`, `hasValidConfig` returns normalized `apiKey/auth/getUrl/postUrl` but you never use them to update `options`, so the subsequent calls still rely on the original (possibly untrimmed/invalid) values; consider either mutating `options` or passing the normalized values through.
- The new CORS handling branches use `error.message.includes('CORS')` without first checking that `error.message` is a non-empty string, which can throw for unexpected error shapes; guard this with a type check before calling `.includes`.
- The added `console.log('options', options);` in `fetchData` looks like debugging output and may be noisy in production—consider removing it or wrapping it behind a debug flag.
## Individual Comments
### Comment 1
<location path="src/plugins/content/export/media/js/aexport.js" line_range="15" />
<code_context>
async function fetchData() {
const options = window.Joomla.getOptions('a-export');
- //console.log('options',options);
+ console.log('options',options);
+ const validation = hasValidConfig(options);
+
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Remove or gate the `console.log` to avoid leaking configuration in production.
Logging the full `options` object can expose secrets (e.g., API keys) and adds noise in production. Please remove this log, guard it behind a debug flag, or log only non-sensitive fields.
</issue_to_address>
### Comment 2
<location path="src/plugins/content/export/media/js/aexport.js" line_range="16-18" />
<code_context>
const options = window.Joomla.getOptions('a-export');
- //console.log('options',options);
+ console.log('options',options);
+ const validation = hasValidConfig(options);
+
+ if (!validation.ok) {
+ showMessage(validation.message, 'error');
+ return;
</code_context>
<issue_to_address>
**suggestion:** Either use the normalized values returned from `hasValidConfig` or simplify its return type.
`hasValidConfig` normalizes `apiKey`, `auth`, `getUrl`, and `postUrl`, but `fetchData` only reads `ok`/`message` and keeps using the original `options`. This can be misleading if future changes assume `fetchData` uses the sanitized values. Either have `fetchData` consume the normalized values (e.g., merge them back into `options` or pass them through), or change `hasValidConfig` to only return `{ ok, message }` if normalization isn’t needed.
Suggested implementation:
```javascript
async function fetchData() {
const rawOptions = window.Joomla.getOptions('a-export');
console.log('options', rawOptions);
// hasValidConfig is expected to normalize config fields and return:
// { ok, message, apiKey, auth, getUrl, postUrl, ... }
const { ok, message, ...normalized } = hasValidConfig(rawOptions);
if (!ok) {
showMessage(message, 'error');
return;
}
// Prefer normalized values if provided, fall back to raw options
const options = {
...rawOptions,
...normalized,
};
document.getElementById('toolbar-upload')
.insertAdjacentHTML(
'afterbegin',
'<span id="loader" class="spinner-grow spinner-grow-sm" role="status" aria-hidden="true"></span>'
);
if (await checkCategory(options)) {
await checkArticle(options);
}
const loader = document.getElementById('loader');
loader.style.display = 'none';
```
To fully support this change, ensure `hasValidConfig` is implemented to:
1. Accept the raw `options` object.
2. Return an object of the form:
`{ ok: boolean, message: string, apiKey, auth, getUrl, postUrl, ...otherNormalizedFields }`
where the additional properties are the normalized versions of the corresponding fields from `options`.
If `hasValidConfig` currently returns a nested structure (e.g. `{ ok, message, options: normalizedOptions }`), adapt the destructuring in `fetchData` accordingly, e.g.:
`const { ok, message, options: normalized } = hasValidConfig(rawOptions);`
and then spread `normalized` into the final `options` object.
</issue_to_address>
### Comment 3
<location path="src/plugins/content/export/media/js/aexport.js" line_range="63" />
<code_context>
} catch (error) {
- showMessage(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) {
</code_context>
<issue_to_address>
**issue:** Guard against `error.message` being undefined before calling `.includes`.
If `error.message` is undefined or not a string, `error.message.includes('CORS')` will throw and hide the original error. Consider guarding the check, e.g.:
```js
if (
error.name === 'TypeError' ||
(typeof error.message === 'string' && error.message.includes('CORS'))
) {
// ...
}
```
The same adjustment should be applied in the POST and PATCH handlers for consistency and safety.
</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
+16
to
+18
| const validation = hasValidConfig(options); | ||
|
|
||
| if (!validation.ok) { |
There was a problem hiding this comment.
suggestion: Either use the normalized values returned from hasValidConfig or simplify its return type.
hasValidConfig normalizes apiKey, auth, getUrl, and postUrl, but fetchData only reads ok/message and keeps using the original options. This can be misleading if future changes assume fetchData uses the sanitized values. Either have fetchData consume the normalized values (e.g., merge them back into options or pass them through), or change hasValidConfig to only return { ok, message } if normalization isn’t needed.
Suggested implementation:
async function fetchData() {
const rawOptions = window.Joomla.getOptions('a-export');
console.log('options', rawOptions);
// hasValidConfig is expected to normalize config fields and return:
// { ok, message, apiKey, auth, getUrl, postUrl, ... }
const { ok, message, ...normalized } = hasValidConfig(rawOptions);
if (!ok) {
showMessage(message, 'error');
return;
}
// Prefer normalized values if provided, fall back to raw options
const options = {
...rawOptions,
...normalized,
};
document.getElementById('toolbar-upload')
.insertAdjacentHTML(
'afterbegin',
'<span id="loader" class="spinner-grow spinner-grow-sm" role="status" aria-hidden="true"></span>'
);
if (await checkCategory(options)) {
await checkArticle(options);
}
const loader = document.getElementById('loader');
loader.style.display = 'none';To fully support this change, ensure hasValidConfig is implemented to:
- Accept the raw
optionsobject. - Return an object of the form:
{ ok: boolean, message: string, apiKey, auth, getUrl, postUrl, ...otherNormalizedFields }
where the additional properties are the normalized versions of the corresponding fields fromoptions.
IfhasValidConfigcurrently returns a nested structure (e.g.{ ok, message, options: normalizedOptions }), adapt the destructuring infetchDataaccordingly, e.g.:
const { ok, message, options: normalized } = hasValidConfig(rawOptions);
and then spreadnormalizedinto the finaloptionsobject.
| } catch (error) { | ||
| showMessage(error); | ||
| let errorMsg = 'Network error occurred'; | ||
| if (error.name === 'TypeError' || error.message.includes('CORS')) { |
There was a problem hiding this comment.
issue: Guard against error.message being undefined before calling .includes.
If error.message is undefined or not a string, error.message.includes('CORS') will throw and hide the original error. Consider guarding the check, e.g.:
if (
error.name === 'TypeError' ||
(typeof error.message === 'string' && error.message.includes('CORS'))
) {
// ...
}The same adjustment should be applied in the POST and PATCH handlers for consistency and safety.
from twitter to x
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.
Added validation for required configuration fields in fetchData function.
Summary by Sourcery
Add configuration validation and improved user messaging for article export operations.
New Features:
Bug Fixes:
Enhancements: