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

CPS-432: Fix ts function usage #796

Merged
merged 4 commits into from
Aug 19, 2021
Merged

CPS-432: Fix ts function usage #796

merged 4 commits into from
Aug 19, 2021

Conversation

deb1990
Copy link

@deb1990 deb1990 commented May 28, 2021 •
edited
Loading

Overview

The PR implements proper usage of the ts or translation function.

Technical Details

  1. Used E::ts instead of ts.
  2. ts function should only be used with string values, so applied the same.
  3. A new Gulp task, generate-translations has been added, which will generate the translation file(.pot).

New function created for translation
Under CRM_Civicase_Hook_Helper_CaseTypeCategory class a new function translate has been created.
This is wrapper for "E::ts" function.
CiviCRM does not recommend to use this to translate variables. But in CiviCase, we have used this function in few places with variables to achieve certain results. Hence this new function has been created, so that it can be only used in the places where it is absolutely necessary.

The Backstop Tests were run in https://github.com/compucorp/cases-product-suite-backstopjs/actions/runs/1081871388.

@deb1990 deb1990 force-pushed the CPS-432-translation-fix branch 6 times, most recently from 2cc2059 to 800095c Compare May 31, 2021 07:26
@deb1990 deb1990 force-pushed the CPS-432-translation-fix branch 3 times, most recently from de1651f to 2cd5055 Compare June 15, 2021 08:12
@deb1990 deb1990 added bug Something isn't working enhancement New feature or request labels Jun 15, 2021
@deb1990
Copy link
Author

deb1990 commented Jun 15, 2021 •
edited
Loading

@jamienovick @james-compucorp @tunbola This PR is in response to #580. I have fixed the the translation issues, but to to make civicase compatible with transifex, we need to make civicase ready for automatic distribution from civicrm extensions page. I dont see any reason about why we should not add civicase for automatic distribution. But I want to confirm this with you guys.

Link: https://docs.civicrm.org/dev/en/latest/extensions/translation/#for-developers-generate-po-and-mo-files-for-your-extension

tunbola
tunbola suggested changes Jun 16, 2021
View reviewed changes
CRM/Civicase/Hook/BuildForm/CaseCategoryFormLabelTranslationForChangeCase.php Outdated
@@ -57,7 +57,7 @@ private function translateFormLabels(CRM_Core_Form $form) {
* For Elements array.
*/
private function translateLabel($element) {
$label = ts($element->getLabel());
$label = $element->getLabel();
Copy link

Choose a reason for hiding this comment

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

This function is used to change relevant form labels for the Change Case Type , Change Case Status and Link Cases to the respective case category equivalent.
I know ts function should only be used with string values but in this case, existing functionality will be broken as this serves a purpose.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed, created a new function as discussed.

CRM/Civicase/Hook/BuildForm/CaseCategoryFormLabelTranslationForNewCase.php Outdated
@@ -52,7 +52,7 @@ private function translateFormLabels(CRM_Core_Form $form) {
*/
private function translateLabel(array $elements) {
foreach ($elements as $element) {
$label = ts($element->getLabel());
$label = $element->getLabel();
Copy link

Choose a reason for hiding this comment

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

Same here. Please check my comment on existing functionality being broken.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed, created a new function as discussed.

CRM/Civicase/Hook/PreProcess/CaseCategoryWordReplacementsForChangeCase.php Outdated
@@ -50,7 +50,7 @@ private function addWordReplacements(CRM_Core_Form $form) {
* Form Object.
*/
private function setPageTitle(CRM_Core_Form $form) {
$pageTitle = ts($form->get_template_vars('activityTypeName'));
$pageTitle = $form->get_template_vars('activityTypeName');
Copy link

Choose a reason for hiding this comment

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

Please note that this also serves a purpose. Without the translation here. Existing functionality will be broken and page titles will not be translated for the respective case category.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed, created a new function as discussed.

CRM/Civicase/Hook/PreProcess/CaseCategoryWordReplacementsForNewCase.php Outdated
@@ -47,7 +47,7 @@ private function addWordReplacements(CRM_Core_Form $form, $caseCategoryId) {
// We need to translate this manually as Civi does not the page title
// through the ts function.
$pageTitle = $form->get_template_vars('activityType');
CRM_Utils_System::setTitle(ts($pageTitle));
CRM_Utils_System::setTitle($pageTitle);
Copy link

Choose a reason for hiding this comment

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

Removing the translation function here will also break existing functionality.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed, created a new function as discussed.

CRM/Civicase/Hook/Tokens/AddContactTokens.php Outdated
@@ -50,11 +52,11 @@ public function __construct(
public function run(array &$tokens) {
foreach ($this->contactFieldsService->get() as $field) {
$tokens[self::TOKEN_KEY]['current_user.contact_' . $field] =
ts('Current User ' . ucwords(str_replace("_", " ", $field)));
E::ts('Current User ') . ucwords(str_replace("_", " ", $field));
Copy link

Choose a reason for hiding this comment

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

You can also do

 E::ts('Current User %1', array(
      1 => ucwords(str_replace("_", " ", $field),
    ));

Copy link
Author

Choose a reason for hiding this comment

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

Kept it like it was before, and just applied the newly created function.

@deb1990 deb1990 force-pushed the CPS-432-translation-fix branch 3 times, most recently from bf9591b to 7b47a62 Compare July 30, 2021 09:54
@deb1990 deb1990 force-pushed the CPS-432-translation-fix branch from 7b47a62 to cbb1bb2 Compare July 30, 2021 11:14
erawat
erawat previously approved these changes Jul 30, 2021
View reviewed changes
@deb1990 deb1990 merged commit b7dd5ea into master Aug 19, 2021
@deb1990 deb1990 deleted the CPS-432-translation-fix branch August 19, 2021 08:43
This was referenced Sep 10, 2021
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tunbola tunbola tunbola requested changes

@lisandro-compucorp lisandro-compucorp lisandro-compucorp approved these changes

@erawat erawat erawat left review comments

Assignees
No one assigned
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@deb1990 @erawat @tunbola @lisandro-compucorp