Write Promoted Information to New Models #23216

chrstinalin · 2025-03-24T14:40:02Z

Description

Changes promoted group information to be written directly to the new models rather than synced from PromotedAddon and PromotedApproval. This mainly includes:

Changing PrimaryHero's promoted_addon OneToOneField to two ForeignKeys, addon and promoted_group.
i. promoted_group allowsnull=True to represent the NOT_PROMOTED state. Its approval (i.e existing PromotedAddonPromotion of that addon and promoted_group) is implicitly checked via the approved_applications_for() call.
Retired the PromotedAddonAdmin intoPromotedAddonPromotionAdmin and PrimaryHeroAdmin -- splintering PrimaryHeroInline off due to loss of direct FK.
Refactoring to be creating PromotedAddonPromotions & PromotedAddonVersions.

Testing

PromotedAddonPromotion

Create or delete a PromotedAddonPromotion for a group and application via the admin panel.
Approve that group for the add-on (i.e create a PromotedAddonVersion).
Behaviour associated with a promotion should work as expected (ex. the banner on Reviewer Tools, the badge on the landing page if recommended or line).

Ex. Deleting PromotedAddonPromotions:

And recreating one:

PrimaryHero

Create or modify a PrimaryHero through the primary hero admin page.
PrimaryHero should be created or updated as expected, just as it behaved as an Inline.

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).

KevinMind

Looks good so far. A lot to review so still need some more time to get through it. Initial thoughts are two fold:

anything you can do to reduce the diff size would be great. I've added comments for specifics
Can you add some logic to ensure we are no longer writing to the PromotedAddon or PromotedAddonVersion models? The important part is there should be 2 tests both trying to create or update the deprecated models and those tests should expect this is not possible.

src/olympia/promoted/models.py

src/olympia/promoted/migrations/0024_alter_promotedaddon_group_id_and_more.py

src/olympia/promoted/tasks.py

src/olympia/promoted/tests/test_admin.py

src/olympia/abuse/tests/test_cinder.py

src/olympia/addons/models.py

KevinMind

If you remove the help_text update and the manual migration, that makes this PR a lot easier to ship since it will no longer contain any migrations. The way I see it, even if you modify the choices removing NOT_PROMOTED it's not strictly necessary to remove that promoted group since every where it is used, it is evaluated as Falsy anyhow.

That would make it a lot easier for me to say yes on this PR, especially since it may need to go out in a cherry pick anyway.

chrstinalin · 2025-03-31T17:15:32Z

If you remove the help_text update and the manual migration, that makes this PR a lot easier to ship since it will no longer contain any migrations.

I can understand the non-removal of the NOT_PROMOTED group, but I still believe the help_text update should be there. It's a behavioural change and it would be confusing to use the admin if it tells you to use NOT_PROMOTED where you shouldn't (and should instead delete).

KevinMind · 2025-04-01T10:21:16Z

src/olympia/discovery/admin.py

@@ -9,14 +9,18 @@
 from django.utils.safestring import mark_safe
 from django.utils.translation import gettext

-from olympia import promoted
+from olympia import hero, promoted
 from olympia.addons.models import Addon
 from olympia.amo.admin import AMOModelAdmin
 from olympia.amo.templatetags.jinja_helpers import vite_asset
 from olympia.discovery.models import DiscoveryItem


Is all of this really necessary for this PR? From what I can see this is a convenience? Or am I missing something? If, so I'd cut the 200 lines from the diff.

I'm really not trying to be a kill joy, just we have had several bugs during the roll out of this feature and the smaller the PR the less likely it is something goes wrong.

There's not really a way to avoid this that I can think of --

There needs to be an admin page to write to PromotedAddonPromotion in stead of PromotedAddon

PromotedAddonAdmin includes the use of PrimaryHeroInline

PrimaryHeroInline cannot be used without the direct FK PromotedAddon provided
It feels a bit cornered.

KevinMind · 2025-04-01T10:50:29Z

src/olympia/hero/models.py

-    promoted_addon = models.OneToOneField(
-        PromotedAddon, on_delete=models.CASCADE, null=False
+
+    addon = models.ForeignKey(Addon, on_delete=models.CASCADE, null=False)


My concern with this is that we no longer have a built in way to ensure that a PrimaryHero actually relates to an existing PromotedAddonPromotion.

Even considering the current approach, we would have multiple PrimaryHero instances per addon right, one per group. If that is the case, and the code relying on PrimaryHero is already supporting multiple instances per addon, then why not just directly FK to PromotedAddonPromotion. That seems like the more obvious path.. did you try that?

Thinking through and walking through the code.. couldn't we get the promoted_groups from the addon? We just need a 1:1 from PrimaryHero:Addon and then we can go self.addon.promoted_groups() to get the promoted groups. We can check if any promoted groups for that addon have can_primary_hero with the query manager you made? Would that not work?

then why not just directly FK to PromotedAddonPromotion. That seems like the more obvious path.. did you try that?

It is, but we can't because of the multiple application_id. No single PromotedAddonPromotion represents an entire promotion.

That, and PromotedAddon was set to NOT_PROMOTED to disable promotion, so that the PrimaryHero could still exist and be tied to the promotion, but not be active. If we tied it to a deletable PromotedAddonPromotion, it would cascade.

Thinking through and walking through the code.. couldn't we get the promoted_groups from the addon? We just need a 1:1 from PrimaryHero:Addon and then we can go self.addon.promoted_groups() to get the promoted groups. We can check if any promoted groups for that addon have can_primary_hero with the query manager you made? Would that not work?

Hmm.. If we want just one PrimaryHeros on an add-on basis rather than per-promotion basis, that may work. This check is still done on a per-group basis for the addon via the all_applications_for() call.

However, that logic also applies to the previous state -- the PrimaryHero was tied to PromotedAddon, and not to the add-on itself, when addon.promoted could have been used instead, and they could have avoided some of the headache. So why wasn't it?

Now that you're scrutinizing it, I think this detail might depend on the use-case.

There also might be a separate issue here. PromotedAddon, on deletion, would have deleted the PrimaryHero... Since we represent deactivation with deletion rather than setting the group to NOT_PROMOTED, the PrimaryHero would remain.

KevinMind · 2025-04-01T11:08:44Z

src/olympia/hero/migrations/0020_primaryhero_addon_primaryhero_promoted_group.py

+    Hero = apps.get_model('hero', 'PrimaryHero')
+    for hero in Hero.objects.all():
+        hero.addon = hero.promoted_addon.addon
+        hero.promoted_group = PromotedGroup.objects.get(group_id=hero.promoted_addon.group.id)


hero.promoted_addon.group_id

This is accessing the old class PromotedClass, where its structured asgroup.id

KevinMind · 2025-04-01T11:10:22Z

src/olympia/hero/migrations/0020_primaryhero_addon_primaryhero_promoted_group.py

+        ),
+        migrations.AddField(
+            model_name='primaryhero',
+            name='addon',


Applying hero.0020_primaryhero_addon_primaryhero_promoted_group...Traceback (most recent call last): File "/data/olympia/deps/lib/python3.12/site-packages/django/db/backends/utils.py", line 89, in _execute return self.cursor.execute(sql, params) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/data/olympia/deps/lib/python3.12/site-packages/django/db/backends/mysql/base.py", line 75, in execute return self.cursor.execute(query, args) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/data/olympia/deps/lib/python3.12/site-packages/MySQLdb/cursors.py", line 179, in execute res = self._query(mogrified_query) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/data/olympia/deps/lib/python3.12/site-packages/MySQLdb/cursors.py", line 330, in _query db.query(q) File "/data/olympia/deps/lib/python3.12/site-packages/MySQLdb/connections.py", line 280, in query _mysql.connection.query(self, query) MySQLdb.OperationalError: (1060, "Duplicate column name 'addon_id'")

That might have been a fluke. I will try running against a clean master.

Huh, that's a weird one. Yeah, I never encountered that one.

KevinMind · 2025-04-01T11:29:12Z

src/olympia/promoted/admin.py

        return qs


-class PromotedAddonAdmin(AMOModelAdmin):
+class PromotedAddonPromotionAdmin(AMOModelAdmin):


We have lost the ability to see PrimaryHero relations in promotion edit page. Additionally we have no way of sorting/filtering for all promotions related to a single addon.

Try adding

search_fields = ('addon__guid', 'addon__slug')

We have lost the ability to see PrimaryHero relations in promotion edit page.

Yes, with the loss of the direct FK Django inlines don't support inlines that don't have one (this is also what I have a feeling this is about), which is why PrimaryHeroInline was turned into PrimaryHeroAdmin.

chrstinalin marked this pull request as draft March 24, 2025 14:40

chrstinalin force-pushed the #15476-write-promoted-models branch 20 times, most recently from c1be62e to 5f49b6e Compare March 30, 2025 16:14

chrstinalin marked this pull request as ready for review March 30, 2025 16:55

chrstinalin requested a review from KevinMind March 30, 2025 16:55

KevinMind reviewed Mar 31, 2025

View reviewed changes

chrstinalin force-pushed the #15476-write-promoted-models branch 2 times, most recently from 6d0db22 to a529ac9 Compare March 31, 2025 13:44

This comment was marked as resolved.

Sign in to view

chrstinalin force-pushed the #15476-write-promoted-models branch 3 times, most recently from 1a6aec4 to adf1bbe Compare March 31, 2025 14:41

chrstinalin requested a review from KevinMind March 31, 2025 14:53

KevinMind requested changes Mar 31, 2025

View reviewed changes

chrstinalin force-pushed the #15476-write-promoted-models branch from f7de029 to 8e7f4b6 Compare March 31, 2025 17:30

chrstinalin added 4 commits March 31, 2025 13:50

Write Promoted Information to New Models

fb79955

Refactoring Tests to Use PromotedAddonVersion and PromotedAddonPromotion

a256ec4

PrimaryHero Migration

6941f5f

Admin Changes

1060c73

chrstinalin force-pushed the #15476-write-promoted-models branch from 8e7f4b6 to 1060c73 Compare March 31, 2025 17:53

chrstinalin requested a review from KevinMind March 31, 2025 18:07

KevinMind reviewed Apr 1, 2025

View reviewed changes

review

72584fc

chrstinalin requested a review from KevinMind April 1, 2025 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Write Promoted Information to New Models #23216

Write Promoted Information to New Models #23216

chrstinalin commented Mar 24, 2025 •

edited

Loading

KevinMind left a comment

This comment was marked as resolved.

KevinMind left a comment

chrstinalin commented Mar 31, 2025 •

edited

Loading

KevinMind Apr 1, 2025

chrstinalin Apr 1, 2025

KevinMind Apr 1, 2025 •

edited

Loading

KevinMind Apr 1, 2025

chrstinalin Apr 1, 2025

chrstinalin Apr 1, 2025 •

edited

Loading

KevinMind Apr 1, 2025

chrstinalin Apr 1, 2025

KevinMind Apr 1, 2025

KevinMind Apr 1, 2025

chrstinalin Apr 1, 2025

KevinMind Apr 1, 2025

chrstinalin Apr 1, 2025

Write Promoted Information to New Models #23216

Are you sure you want to change the base?

Write Promoted Information to New Models #23216

Conversation

chrstinalin commented Mar 24, 2025 • edited Loading

Description

Testing

Checklist

KevinMind left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

KevinMind left a comment

Choose a reason for hiding this comment

chrstinalin commented Mar 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinMind Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrstinalin Apr 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrstinalin commented Mar 24, 2025 •

edited

Loading

chrstinalin commented Mar 31, 2025 •

edited

Loading

KevinMind Apr 1, 2025 •

edited

Loading

chrstinalin Apr 1, 2025 •

edited

Loading