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.

Sign up for GitHub

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

Jump to bottom

[BUG]: CDATA causes issues with Translatewiki importing #5357

Closed
BenHenning opened this issue Mar 12, 2024 · 2 comments · Fixed by #5361 or #5524
Closed

[BUG]: CDATA causes issues with Translatewiki importing #5357

BenHenning opened this issue Mar 12, 2024 · 2 comments · Fixed by #5361 or #5524
Assignees
@adhiamboperes
Labels
bug End user-perceivable behaviors which are not desirable. Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet.
Milestone
1.0 Global availa...

Comments

@BenHenning
Copy link
Member

BenHenning commented Mar 12, 2024 •
edited
Loading

Describe the bug

As described in https://github.com/oppia/oppia-android/pull/5274/files#r1517940307 and observed in that PR, CDATA in strings causes one of three Translatewiki behaviors:

  1. The string can be correctly translated.
  2. The string is ignored (as is observed for privacy_policy_web_link, agree_to_terms, and terms_of_service_web_link.
  3. The translation string is empty (Localisation updates from https://translatewiki.net. #5274).

Steps To Reproduce

See above.

Expected Behavior

All strings should be translatable via Translatwiki.

Screenshots/Videos

No response

What device/emulator are you using?

No response

Which Android version is your device/emulator running?

No response

Which version of the Oppia Android app are you using?

No response

Additional Context

I think this means we cannot use CDATA anymore and instead should prohibit it via a regex check. Existing CDATA strings will need to be escaped, instead (e.g. HTML tag markers will need to be replaced with '>' and '<').
@BenHenning BenHenning added bug End user-perceivable behaviors which are not desirable. triage needed labels Mar 12, 2024
@BenHenning
Copy link
Member Author

@adhiamboperes I think you're looking into this.

@BenHenning BenHenning mentioned this issue Mar 12, 2024
Merged
@adhiamboperes adhiamboperes added Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. and removed triage needed labels Mar 12, 2024
@BenHenning BenHenning mentioned this issue Mar 14, 2024
Merged
6 tasks
BenHenning added a commit that referenced this issue Mar 15, 2024
@BenHenning
Fix #5357: Remove CDATA from translatable strings. (#5361)
0cc0237 
## Explanation
Fixes #5357

This PR removes all ``CDATA`` declarations in the translatable
``strings.xml`` file and instead escapes all necessary characters: < and
> (& didn't need to be escaped since no strings use that character at
the moment). This is needed because Translatewiki doesn't seem to
extract the HTML within the CDATA declaration correctly, so it may not
be translated (some existing strings were never translated, and per
#5274 the latest FAQ changes aren't being processed correctly (leading
to empty translation strings being submitted).

This PR also introduces a regex check + test to ensure that CDATA isn't
used anymore in strings.

## Essential Checklist
- [x] The PR title and explanation each start with "Fix #bugnum: " (If
this PR fixes part of an issue, prefix the title with "Fix part of
#bugnum: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## For UI-specific PRs only
See video for a demonstration on all of the affected strings still
rendering correctly (as far as I can tell) after this change:


https://github.com/oppia/oppia-android/assets/12983742/b1102d28-d3a5-4266-969c-d7d84f8d5d38
adhiamboperes added a commit that referenced this issue May 22, 2024
@adhiamboperes
Revert "Fix #5357: Remove CDATA from translatable strings. (#5361)"
c6c822c 
This reverts commit 0cc0237.
@BenHenning BenHenning reopened this May 26, 2024
@BenHenning BenHenning mentioned this issue May 26, 2024
Merged
@BenHenning BenHenning reopened this May 26, 2024
@BenHenning
Copy link
Member Author

The original PR fixing this has been reverted, so this work needs to happen again.

@adhiamboperes adhiamboperes mentioned this issue Aug 30, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@adhiamboperes adhiamboperes

Labels
bug End user-perceivable behaviors which are not desirable. Impact: Medium Moderate perceived user impact (non-blocking bugs and general improvements). Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet.
Projects
[Team] Developer Workflow & Infrastructure - Android
Archived in project
Milestone
1.0 Global availability
2 participants
@BenHenning @adhiamboperes