Add multi-language authoring support (API changes) - #2857
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## DEP-234-authoring-language-switching #2857 +/- ##
========================================================================
- Coverage 75.17% 74.88% -0.29%
========================================================================
Files 517 527 +10
Lines 20067 20696 +629
Branches 1723 1723
========================================================================
+ Hits 15085 15499 +414
- Misses 4973 5188 +215
Partials 9 9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
jareth-whitney
left a comment
jareth-whitney
left a comment
There was a problem hiding this comment.
Great job implementing a complex piece! Nothing blocking, just check out the following:
- Translation of email and phone_number, is it necessary?
- Definition of default language code, should it be defined in env variables?
- Questions regarding image translations, is it just for the alt tag?
| batch_op.drop_column('subscribe_section_description') | ||
| batch_op.drop_column('subscribe_section_heading') | ||
| batch_op.drop_column('feedback_body') | ||
| batch_op.drop_column('feedback_heading') |
There was a problem hiding this comment.
Wow that is a very complex migration. It looks like we're decoupling some of the english strings from the engagement, which I guess makes sense. English is a language like any other, so we can treat the english strings with equal importance and structure.
| batch_op.drop_column('subscribe_consent_message') | ||
| batch_op.drop_column('subscribe_section_description') | ||
| batch_op.drop_column('subscribe_section_heading') | ||
| batch_op.drop_column('feedback_body') |
There was a problem hiding this comment.
Thanks for creating tables for other translations other than the engagement.
|
|
||
| WHO_IS_LISTENING = 1 | ||
| DOCUMENTS = 2 | ||
| SUBSCRIBE = 4 |
There was a problem hiding this comment.
Oops I must have missed that one, thanks for removing it.
| ) | ||
| name = db.Column(db.String(50), nullable=True) | ||
| title = db.Column(db.String(50), nullable=True) | ||
| email = db.Column(db.String(50), nullable=True) |
There was a problem hiding this comment.
It seems like we shouldn't need to translate email or phone_number but perhaps this is just an organization thing.
| 'widget_documents_id', 'language_id', | ||
| unique=True, | ||
| postgresql_where=text('widget_documents_id IS NOT NULL'), | ||
| ), |
There was a problem hiding this comment.
The indexes are a nice touch and should save us from conflicting data.
|
|
||
| @api.route('/') | ||
| class WidgetImageTranslationResource(Resource): | ||
| """Resource for creating widget image translations.""" |
There was a problem hiding this comment.
What do we need to change about the widget image? Is it the alt tag?
There was a problem hiding this comment.
yes; the alt text should be translatable for accessibility in other languages
| subscribe_consent_message = fields.Str(data_key='subscribe_consent_message') | ||
| sponsor_name = fields.Str(data_key='sponsor_name') | ||
| more_engagements_heading = fields.Str(data_key='more_engagements_heading') | ||
| languages = fields.List(fields.Str(), data_key='languages', load_only=True) |
There was a problem hiding this comment.
Nice, so the languages are stored within the engagement now. That was one issue before, I believe they were being stored by tenant.
| """Engagement management service.""" | ||
|
|
||
| otherdateformat = '%Y-%m-%d' | ||
| default_language_code = 'en' |
There was a problem hiding this comment.
Don't we have an env variable for this?
|
bce589f
into
DEP-234-authoring-language-switching
3 participants
Issue #: 🎟️ DEP-234
Description of changes:
Updated the DEP API to add support for:
in anticipation of frontend changes for DEP-234
User Guide update ticket (if applicable):
Common component changes: