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

feat(api): add public endpoint for querying wikis #635

Merged
merged 15 commits into from
Aug 23, 2023
Merged

Conversation

m90
Copy link
Contributor

@m90 m90 commented Aug 22, 2023

Ticket https://phabricator.wikimedia.org/T344581

Pagination behavior is documented here: https://laravel.com/docs/8.x/eloquent-resources#pagination, i.e. the UI can pass per_page and page, and reads cursor info in the meta property.

Available sort options are pages and sitename (default) with direction values of asc (default) and desc.

@m90 m90 force-pushed the fr/public-wiki-endpoint branch 15 times, most recently from ff49ecf to 2c75c93 Compare August 22, 2023 13:05
*/
public function show($id)
{
return new PublicWikiResource(Wiki::findOrFail($id));
Copy link
Contributor Author

@m90 m90 Aug 22, 2023

Choose a reason for hiding this comment

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

This allows clients to look up wikis by their integer id. Instead, we could also look them up by domain and hide the id from the public, not sure if I have a preference.

app/Wiki.php Outdated Show resolved Hide resolved
routes/api.php Outdated
@@ -22,6 +22,8 @@
$router->post('user/resetPassword', ['uses' => 'Auth\ResetPasswordController@reset']);
$router->post('contact/sendMessage', ['uses' => 'ContactController@sendMessage']);

$router->apiResource('wiki', 'PublicWikiController');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this needs stricter throttling than the default one? Current settings allows for 45 request to be made in one minute per IP. I am not sure however if we actually see the forwarded IP or if this is effectively a global setting, throttling the "ingress client".

@m90 m90 force-pushed the fr/public-wiki-endpoint branch 10 times, most recently from fa3d97a to 1d752fa Compare August 22, 2023 20:04
@m90 m90 force-pushed the fr/public-wiki-endpoint branch 4 times, most recently from 26661e7 to 9717f24 Compare August 23, 2023 08:39
'is_featured' => 'boolean',
'is_active' => 'boolean',
'per_page' => 'numeric',
'page' => 'numeric'
Copy link
Contributor Author

@m90 m90 Aug 23, 2023

Choose a reason for hiding this comment

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

The page parameter is somehow magically picked up by Laravel itself, we never pass it on or look at it. Not sure if we need to validate it, but I thought it might be good to put it here so all possible query params are documented here.

}

if (array_key_exists('is_active', $params) && $params['is_active']) {
$query = $query->whereRelation('wikiSiteStats', 'pages', '>', 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ticket currently say > 1, I need to double check if this is correct or needs to be changed.

https://phabricator.wikimedia.org/T339334

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ticket is correct. I fixed this to be > 1

@m90 m90 force-pushed the fr/public-wiki-endpoint branch 2 times, most recently from 3d044a5 to ee5c4bd Compare August 23, 2023 13:07
Copy link
Contributor

@dati18 dati18 left a comment

Choose a reason for hiding this comment

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

Everything is great in my eyes. Didn't catch anything off

@m90 m90 merged commit 07248a9 into main Aug 23, 2023
5 checks passed
@m90 m90 deleted the fr/public-wiki-endpoint branch August 23, 2023 14:43
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