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

Allow case_search_display and case_search_hint values to be translated in Transifex #34794

Merged
merged 5 commits into from
Jun 26, 2024

Conversation

minhaminha
Copy link
Contributor

Product Description

This PR fixes a bug that was preventing case property display text under the Case Search and Claim section from being pushed to Transifex and from being uploaded back into the app.

Technical Summary

Jira Ticket

These values weren't being pushed simply their type (case_search_display and case_search_hint) wasn't matching the regex. There was also an old conditional where if a bulk translations upload had any of its case property text and their corresponding hints missing, it wouldn't update any of either field, even if there was partial data. I've made this more lenient for hints because empty hints won't be pushed to Transifex and so won't be included as a row in the pulled file that you upload back into the app. The bulk application translation uploader will not be blocked by partially complete hint data.

Feature Flag

Feature flags necessary to use Case Search and Claim (see here)

Safety Assurance

Safety story

Tested locally and on staging.

Minimal impact to existing data. Transifex pushing and pulling are now more inclusive, as is the bulk translations uploader. If it sees that it's missing a certain hint field, it'll skip updating it.

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@minhaminha minhaminha added the product/feature-flag Change will only affect users who have a specific feature flag enabled label Jun 19, 2024
@minhaminha minhaminha requested a review from orangejenny as a code owner June 19, 2024 20:04
Copy link
Contributor

@Jtang-1 Jtang-1 left a comment

Choose a reason for hiding this comment

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

changes look good, just some nonblocking comments

self.msgs.append((messages.warning, message))
partially_update_translations(displays, is_display=True)

if len(hints) == len(properties):
Copy link
Contributor

Choose a reason for hiding this comment

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

Recognizing this wasn't part of your changes, but this message could be consolidated with the almost exact same one for display

'A hint row for menu {index} has an unexpected case search property "{field}". '
                                'Case properties must appear in the same order as they do in the bulk '
                                'app translation download. No translations updated for this row.'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my response to your other comment also applies to this one!

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm seeing that string is already used in two places, but one says "A hint row" while the other is "A display row"

I was thinking we can do something like

msg = 'A {row_property} row for menu {index} has an unexpected case search property "{field}". '
                                'Case properties must appear in the same order as they do in the bulk '
                                'app translation download. No translations updated for this row.

and we can do msg.format('hint'...) and msg.format('display'...). But nbd, certainly not blocking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah you're right I misread the part of the code this comment was attached to - made the error message reusable!

@minhaminha minhaminha merged commit c2663e2 into master Jun 26, 2024
13 checks passed
@minhaminha minhaminha deleted the ml/missing-display-translations branch June 26, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/feature-flag Change will only affect users who have a specific feature flag enabled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants