-
Notifications
You must be signed in to change notification settings - Fork 153
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
[7434] - FE/BE Structured data field for Mozfest pages #7568
Changes from 7 commits
b75fee6
5ec54d6
4bec8f2
9ee28fd
0f17204
6fbbdb4
ffb815e
fad1be5
2528770
c2d47d6
c909a41
8d4051f
c61d0b2
c1aee37
990d60b
fdf620d
99230c3
0504981
979cd95
00865f6
ac8af7c
fa38a60
de7502a
8f0eaab
4c3219f
33f1f5b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
# Generated by Django 3.1.11 on 2021-10-19 10:21 | ||
|
||
from django.db import migrations, models | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
('mozfest', '0021_add_spaces_cards'), | ||
] | ||
|
||
operations = [ | ||
migrations.AddField( | ||
model_name='mozfestprimarypage', | ||
name='structured_data', | ||
field=models.TextField(blank=True, help_text='Structured data JSON for Google search results. Do not include the <script> tag. See https://schema.org/ for properties and https://validator.schema.org/ to test validity.', max_length=1000, null=True), | ||
), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
from django import forms | ||
from django.db import models | ||
from wagtail.admin.edit_handlers import FieldPanel, StreamFieldPanel | ||
from wagtail.core.fields import StreamField, RichTextField | ||
|
@@ -81,6 +82,18 @@ class MozfestPrimaryPage(FoundationMetadataPageMixin, FoundationBannerInheritanc | |
FieldPanel('use_wide_template') | ||
] | ||
|
||
structured_data = models.TextField( | ||
max_length=1000, | ||
help_text='Structured data JSON for Google search results. Do not include the <script> tag. ' | ||
'See https://schema.org/ for properties and https://validator.schema.org/ to test validity.', | ||
blank=True, | ||
null=True, | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a structured schema (custom) field that we can use instead, that runs schema validation as part of its Non-technical staff will have no idea what to do with those websites, and I suspect even some technical staff will be scratching their heads =) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be too late but this might work: https://pypi.org/project/wagtail-schema.org/ |
||
|
||
promote_panels = FoundationMetadataPageMixin.promote_panels + [ | ||
FieldPanel('structured_data', widget=forms.Textarea(attrs={"rows": 10})) | ||
] | ||
|
||
def get_template(self, request): | ||
if self.use_wide_template: | ||
return 'mozfest/mozfest_primary_page_wide.html' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've commented out this function because it was causing issues in the CI.
Specifically this line was attempting to access the
structured_data
field when it doesn't exist at the time of migration and throwsdjango.db.utils.ProgrammingError: column mozfest_mozfestprimarypage.structured_data does not exist
:https://github.com/mozilla/foundation.mozilla.org/blob/ffb815e07d195664db5d9bb96023359456f9e3fd/network-api/networkapi/mozfest/migrations/0017_update_wide_images.py#L15
This is an example of the failed CI run: https://github.com/mozilla/foundation.mozilla.org/runs/3937484149?check_suite_focus=true#step:9:453
In case the log is unavailable at the time of review, here's a summary of the run:
CI Summary
I figured it would be safe to comment out this line since this migration has likely already been done on production, but I'm not sure if you have a protocol in place for these kinds of events. Please let me know if there are alternate ways we can resolve this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Migration 0017 should only ever run if it hasn't already been applied, and any models that are used in the migration functions should be "constructed" from the database schema rather than loaded from any .py file to avoid exactly this situation... looks like we forgot that for this file!
Rather than commenting out the code, let's fix how that
MozfestPrimaryPage
gets loaded in: rather than having afrom networkapi.mozfest.models import MozfestPrimaryPage
up top, the migration code that uses it should get that model from the current run state, usingMozfestPrimaryPage = apps.get_model('mozfest', 'MozfestPrimaryPage')
as first line in the migration function.See, for example, https://github.com/mozilla/foundation.mozilla.org/blob/main/network-api/networkapi/mozfest/migrations/0001_to_0015.py#L14
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Pomax
I've fixed the migration that was causing the issue. It was tricky to test but I have console output of before and after running it locally. The second call to
p = MozfestPrimaryPage.objects.get(id=161)
is after the migration is run, note the'image_width': 'wide'
in the secondp.body.raw_data
output.It won't be run on production anyway, but it does seem to work.
Note: the old
wide_image
field wasn't removed during my local test but is in the submitted migration.