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

Read Promoted Group Information Via Models #23082

Merged
merged 18 commits into from
Mar 19, 2025

Conversation

chrstinalin
Copy link
Contributor

@chrstinalin chrstinalin commented Feb 14, 2025

Fixes: mozilla/addons#15350

Description

Reads promoted group information via models.

Testing

Promoted add-on behaviour should be largely the same (as this PR is only switching the read behaviour, and not explicitly allowing multiple promoted addons.), except:

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 or update relevant docs reflecting the changes made.

@chrstinalin chrstinalin marked this pull request as draft February 14, 2025 22:45
@chrstinalin chrstinalin force-pushed the #15350-read-promoted-models branch 3 times, most recently from ef58b87 to d3fe342 Compare February 18, 2025 20:40
@chrstinalin chrstinalin force-pushed the #15350-read-promoted-models branch 4 times, most recently from d497061 to 4928d11 Compare February 28, 2025 20:04
@chrstinalin chrstinalin force-pushed the #15350-read-promoted-models branch 8 times, most recently from ac814ce to 047c447 Compare March 6, 2025 15:14
@chrstinalin chrstinalin force-pushed the #15350-read-promoted-models branch from 047c447 to c24f1fe Compare March 6, 2025 16:17
@chrstinalin chrstinalin requested a review from KevinMind March 6, 2025 16:49
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.

A few questions and wondering if it would be possible to split this into 2 PRs. This PR should be able to move the "read" operations from PromotedAddon to PromotedAddonPromotion without otherwise changing much of anything like function signatures.

Then we would migrate the "write" operations from PromotedAddon to PromotedAddonPromotion.

Finally, you can modify the method signatures to support multiple promoted groups.

I think it might be easier/safer to get this out in these smaller PRs that focus on one specific part.

@chrstinalin
Copy link
Contributor Author

A few questions and wondering if it would be possible to split this into 2 PRs. This PR should be able to move the "read" operations from PromotedAddon to PromotedAddonPromotion without otherwise changing much of anything like function signatures.

I think its at the point where splitting it further would make things more difficult. All the changes here are pretty interconnected at this point, the signals part was the only part I could manage to split off (and even then it wasn't much).

@chrstinalin chrstinalin requested a review from KevinMind March 6, 2025 19:32
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 have a strong-ish opinion about the API changes. Modifying the signature and source of truth from the Promotion to the Group is not really what I thought we were going to do and I think it was better the way it was.

Let's discuss on a call and figure out what to do and move from there.

@chrstinalin chrstinalin marked this pull request as ready for review March 13, 2025 13:05
@chrstinalin chrstinalin force-pushed the #15350-read-promoted-models branch from 7683eea to e1370cc Compare March 13, 2025 16:03
@chrstinalin chrstinalin force-pushed the #15350-read-promoted-models branch from 94ef792 to 879e0c2 Compare March 13, 2025 16:55
@KevinMind KevinMind self-requested a review March 14, 2025 09:23
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.

Approved, fix the tests and make sure to notify in the next release notes that we need QA to do a full test of promoted addon behavior.

@chrstinalin chrstinalin force-pushed the #15350-read-promoted-models branch from 208a168 to 761bfb1 Compare March 14, 2025 14:51
@chrstinalin chrstinalin force-pushed the #15350-read-promoted-models branch from 72e4c6f to 57984c3 Compare March 17, 2025 16:38
@KevinMind KevinMind self-requested a review March 18, 2025 11:13
@chrstinalin chrstinalin merged commit bd3cda6 into mozilla:master Mar 19, 2025
41 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.

[Task]: Read promoted group properties for multiple promoted groups
3 participants