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

rm sponsored, verified #22671

Merged

Conversation

eviljeff
Copy link
Member

@eviljeff eviljeff commented Sep 17, 2024

Fixes: mozilla/addons#14996

Description

Removes sponsored and verified promoted groups. Also add an API gate to silently ignore those promoted group names if passed as a search parameter - currently enabled for v3, v4, and v5, but we could drop v5 once frontend is fixed (as it's explicitly positioned as unstable).

Context

Frontend (and probably autograph) would be separate PRs.

Testing

  • use django admin to try to create a promoted addon; see Verified and Sponsored are no longer available
  • make a search API call with a ?promoted=sponsored param
    • see no results but no error
    • repeat with verified
    • repeat with other promoted groups both separately and with verified/sponsored (it's a comma-separated list)

Checklist

  • Add #ISSUENUM at the top of your PR to an existing open issue in the mozilla/addons repository.
  • Successfully verified the change locally.
  • The change is covered by automated tests, or otherwise indicated why doing so is unnecessary/impossible.
  • Add before and after screenshots (Only for changes that impact the UI).
  • Add or update relevant docs reflecting the changes made.

@eviljeff eviljeff force-pushed the 14996-rm-verified-sponsored-promoted-groups branch from 57e0eda to 423e812 Compare September 17, 2024 17:20
@eviljeff eviljeff force-pushed the 14996-rm-verified-sponsored-promoted-groups branch from 212cc66 to b8edcc8 Compare September 18, 2024 18:23
@eviljeff eviljeff requested review from a team September 18, 2024 18:42
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.

Code looks good. Why are there a bunch of places replacing request.GET with request?

Verified locally. Works as expected.

Screenshot 2024-09-19 at 11 38 35 Screenshot 2024-09-19 at 11 39 18 Screenshot 2024-09-19 at 11 40 15

@@ -278,14 +278,12 @@ This endpoint allows you to fetch a specific add-on by id, slug or guid.
line "By Firefox" category
notable Notable category
recommended Recommended category
sponsored Sponsored category
spotlight Spotlight category
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you have a look at the addons swagger api documentation and see if the same can be applied?

Copy link
Member Author

Choose a reason for hiding this comment

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

None of the query parameters are documented in swagger according to mozilla/addons#15019

src/olympia/addons/tests/test_views.py Show resolved Hide resolved
@eviljeff
Copy link
Member Author

eviljeff commented Sep 19, 2024

This is still a draft PR :)
now ready

@eviljeff eviljeff marked this pull request as ready for review September 19, 2024 16:32
@eviljeff
Copy link
Member Author

Code looks good. Why are there a bunch of places replacing request.GET with request?

The signature changed for AddonQueryParam and AddonQueryMultiParam - they used to be initialized with the query dict (from the request); now they are initialized with the request. We need the request to perform the api_gate check.

@eviljeff eviljeff requested review from KevinMind and removed request for a team September 19, 2024 16:45
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.

In general, I would think that one push to remove the code references and a separate one to actually clean up the data would be the safer way to do this.

If the migration runs and you delete all the data, but then there is some unseen issue with the code.. well too late.

I can understand not wanting to do that since our current delivery schedule would mean that's a month long process... 🤷

Is this something normal for us? To couple migrations removing data with removing code references in the same patch? @diox do you have an opinion here?

@eviljeff eviljeff merged commit ddc1070 into mozilla:master Sep 20, 2024
31 checks passed
@diox
Copy link
Member

diox commented Sep 20, 2024

Is this something normal for us? To couple migrations removing data with removing code references in the same patch? @diox do you have an opinion here?

Depend on what the migration is.

I would agree we should remove the code first in the cases where the code is responsible for creating that data, and is accessible to the outside world, but in this case it's safe because we know there are no verified/sponsored promotions at the moment (there never was any in prod, as the project was cancelled before going live), and the only way to add any is to have someone with permissions go in the admin to make some.

(for schema migrations, we definitely care about backwards-compatibility to allow for rollbacks and just make the deploy painless when we are deploying the new schema with old code still serving requests, and in that case we typically make fields nullable through a migration while removing them from the code, and then remove the column entirely later)

This comment was marked as off-topic.

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.

[Task]: Remove Verified/Sponsored promoted tiers
3 participants