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

Document 'app' query parameter in ratings API #22648

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

diox
Copy link
Member

@diox diox commented Sep 6, 2024

Fixes: mozilla/addons#1910

app parameter is handled a bit transparently in addons-server and so we had forgotten to document it in this specific API, but it does have a (minor) impact if provided.

@diox diox requested review from a team and KevinMind and removed request for a team September 6, 2024 15:16
@@ -27,6 +27,7 @@ user has already posted a rating for the current version of an add-on.
.. http:get:: /api/v5/ratings/rating/

:query string addon: The :ref:`add-on <addon-detail>` id, slug, or guid to fetch ratings from. When passed, the ratings shown will always be the latest posted by each user on this particular add-on (which means there should only be one rating per user in the results), unless the ``version`` parameter is also passed.
:query string app: Set the :ref:`add-on application <addon-detail-application>` for that query. This won't filter the results, but can affect the URLs being returned. Defaults to ``firefox``.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think anything added to rst docs should also be added to swagger at this point. Let's at least not get further behind.

@KevinMind
Copy link
Contributor

Could you have a look at the ratings api view and see if the swagger documentation is also up to date as much as possible?

@diox
Copy link
Member Author

diox commented Sep 10, 2024

Swagger docs for ratings are missing... everything really. None of the query parameters except for pagination are documented in swagger.

@diox diox requested a review from KevinMind September 11, 2024 13:00
@KevinMind
Copy link
Contributor

Then wouldn't this be a perfect opportunity to add at least some definition to the schema? I'm not saying you need to aim for equivalence but if we make a habit of always slowly improving the coverage then over time it will get much better without requiring some massive uplift.

Copy link
Contributor

@KevinMind KevinMind left a comment

Choose a reason for hiding this comment

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

I think you should at least attempt to add some definition to the swagger definition of the endpoint/method you touched here.

Otherwise our swagger API will always be behind 🤷 I'm not gonna block merging such a small patch but I'm at least going to verbally protest 🙃

@diox
Copy link
Member Author

diox commented Sep 12, 2024

Filed mozilla/addons#15019 because it's really all our APIs and I had this particular issue as a 0 story point 😅

@diox diox merged commit e3d55d9 into mozilla:master Sep 12, 2024
31 checks passed
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.

Ratings API returns URLs depending in app parameter, but this isn't documented
2 participants