Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Make use of the new AdminController #151

Open
wants to merge 1 commit into
base: 4
Choose a base branch
from

Conversation

GuySartorelli
Copy link
Member

'TrackID' => $track->ID,
'Status' => $track->Status,
'Completed' => $track->getCompletedPages(),
'Total' => $track->getTotalPages()
]);
}
return $this->jsonSuccess(200, []);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning nothing (by returning $this->jsonSuccess(200);) resulted in a "parsererror" toast message. So I had to include the [] there.
Looks like the toast was being triggered by something upstream of the js in this module itself, so I couldn't just solve it there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing with start() below.

Comment on lines -28 to -30
if (!Permission::check('CMS_ACCESS_CMSMain')) {
return $this->httpError(403, 'You do not have permission to access this resource');
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handled by AdminController::init()

return $this->httpError(403, 'You do not have permission to access this resource');
}
// Set headers
HTTPCacheControlMiddleware::singleton()->setMaxAge(0);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This middleware is disabled in AdminController::init()

Comment on lines -33 to -35
$this->response
->addHeader('Content-Type', 'application/json')
->addHeader('Content-Encoding', 'UTF-8')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UTF-8 encoding is either not needed anymore or is done by default - works without that line.
The content type is set by AdminController::jsonSuccess()

@@ -99,7 +99,7 @@
async: true,
success(data) {
// No report, so let user create one
if (!data) {
if (!data || (typeof data === 'object' && data.length < 1)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no data is no option anymore, but I kept it there as defensive programming.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay we don't need this silly routing rule anymore

Comment on lines +17 to +19
private static string|array $required_permission_codes = [
'CMS_ACCESS_CMSMain',
];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken from the explicit permission checks that used to be in the actions.

@GuySartorelli GuySartorelli marked this pull request as ready for review October 16, 2024 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants