Aaron/issues/translation #409

aaronskiba · 2023-08-01T19:08:16Z

Enable missing French translations: Missing front end French translations #404
Remove duplicate "Sign Up" and "Help" messages in "About Us": Repeated text in Getting started section of /about_us #407
Fix broken "Help" link: Broken link to /help page #412

This addresses "About Us" bug mentioned in issue #404

Enables proper export to translation.io

Need this for translation "Research domain" dropdown elements in "Project Details" panel of Plan

This addresses issue #407

github-actions · 2023-08-01T19:13:32Z

	1 Warning
⚠️	There are code changes, but no corresponding tests. Please include tests if this PR introduces any modifications in behavior.

Generated by 🚫 Danger

aaronskiba · 2023-08-01T19:32:13Z

app/presenters/research_output_presenter.rb

Maybe refactoring is needed for the array in def self.selectable_subjects? If the elements didn't contain commas and other symbols, I would've created enum selectable_subject in class ResearchOutput. Also, I'm not sure if the prepended XX- can be removed from each of the array elements.

aaronskiba · 2023-08-01T19:40:19Z

app/views/static_pages/about_us.html.erb

This remedies issue #407. No translations are currently available for the "Sign up" and "Help" messages I commented out here. Otherwise, I probably would've kept them and commented out the other "Sign up" and "Help" messages.

lagoan

The changes are looking good! I just have a few questions about the approach for some minor parts of the code.

Looking forward to reading your responses!

app/presenters/research_output_presenter.rb

lagoan · 2023-08-01T20:29:30Z

app/presenters/research_output_presenter.rb

+      "23-#{_('Agriculture, Forestry, Horticulture and Veterinary Medicine')}",
+      "21-#{_('Biology')}",
+      "31-#{_('Chemistry')}",
+      "44-#{_('Computer Science, Electrical and System Engineering')}",
+      "45-#{_('Construction Engineering and Architecture')}",
+      "34-#{_('Geosciences (including Geography)')}",
+      "11-#{_('Humanities')}",
+      "43-#{_('Materials Science and Engineering')}",
+      "33-#{_('Mathematics')}",
+      "41-#{_('Mechanical and industrial Engineering')}",
+      "22-#{_('Medicine')}",
+      "32-#{_('Physics')}",
+      "12-#{_('Social and Behavioural Sciences')}",
+      "42-#{_('Thermal Engineering/Process Engineering')}"


I dont think the hashtag notation will work for this. In previous times we have used the format method so that gettext can identify the keys. Translation gem is using gettext which uses the full string as a key in a hash object to look up the translations. so if a key is missing it wont translate.

So it should be something like _('23-Agriculture, Forestry, Horticulture and Veterinary Medicine').

Or you can try something like format("23-%{subject}", subject : _(Agriculture, Forestry, Horticulture and Veterinary Medicine')) which could make it more general. I am not 100% sure it works though.

Give it a try on the test project and talk with @200455939-yashu .

Cool, good to double check. I just tried creating a couple translations in dmp-assistant-test. I guess the current code format is ok for translation.io to pick up.

Actually, although it is getting picked up, this will still probably need to be fixed. When .map do |subject| [subject.split('-').last, subject.gsub('-', ' ')] is applied to the array, both elements in the array receive the translation. However, subject.gsub('-', ' ') should not be receiving the translation.

app/views/research_outputs/metadata_standards/_search.html.erb

app/views/static_pages/about_us.html.erb

lagoan · 2023-08-01T20:35:58Z

app/views/static_pages/about_us.html.erb

@@ -34,10 +34,11 @@

      <h2 class="fontsize-h3"><%= _('Getting started') %></h2>

-        <p><%= _("If you have an account please sign in and start creating or editing your DMP.") %></p>
+        <p><%= _("If you have an account, please sign in and start creating or editing your DMP.") %></p>
+        <!--


Sorry, why are we commenting out this next part? If we don't need it we should remove it, but it looks like this code is something we do want here.

All good. This related to #407. Currently, the "Sign Up" and "Help" messages are repeated in the "Getting Started" section of the "About Us" page https://assistant.portagenetwork.ca/about_us.

I'm thinking we'll need to remove one of the "Sign Up" messages and one of the "Help" messages. But I actually prefer the code I commented out in comparison to what I kept. However, translations aren't currently available for the code that I do prefer.

We can double-check with @isawil to see which copy we like best. We can leave just one instead of waiting for the translation for the missing piece.

Does this make sense?

Cool, yes, that makes sense. I'll verify which one we should keep.

aaronskiba · 2023-08-01T22:22:21Z

The changes are looking good! I just have a few questions about the approach for some minor parts of the code.

Looking forward to reading your responses!

Awesome, thank you! Translation.io is a little peculiar so I appreciate any questions/concerns.

It looks like all of the proposed changes are currently working for the translations, though. Maybe you'd still prefer for something to be refactored, though? Also, I don't know if you have a preference as to how we should proceed with this #409 (comment)? Thanks!

Perhaps the best solution would be to add 'enum selectable_subject' to ResearchOutput. That way we could implement the same logic we used for ResearchOuput.output_types and ResearchOutput.accesses.

200455939-yashu · 2023-08-04T15:57:22Z

config/locale/app.pot

Do we need these files ?

Good question. I want to test the translations in UAT, but there are a few things I'm unsure about:

Does UAT use the test key or the prod key for translation.io?

Do I need to provide the po files for UAT deployment?

When translations are provided to translation.io, are those changes automatically sync'd back to UAT? And if not, should we deploy another UAT instance, or should we execute the translation:sync task within the existing UAT instance?

As per my knowledge,

UAT uses the prod key.

No, PO files are not needed for the UAT deployment.

Everything available in Translation.io are directly available to the UAT.

aaronskiba · 2023-08-04T17:09:52Z

It looks like all of the proposed changes are currently working for the translations, though. Maybe you'd still prefer for something to be refactored, though? Also, I don't know if you have a preference as to how we should proceed with this #409 (comment)? Thanks!

Or so I thought, haha. There was an issue that was addressed with the following commit: 07741d2

Also, sorry, I have a question here: #409 (comment)

200455939-yashu

Good work

Please remove the PO files

This reverts commit 1a00675.

lagoan

Great work @aaronskiba , I found some very minor changes that we can make and then it will be good to go.

lagoan · 2023-08-14T20:52:11Z

app/presenters/research_output_presenter.rb

@@ -11,13 +11,13 @@ def initialize(research_output:)
  # Returns the output_type list for a select_tag
  def selectable_output_types
    ResearchOutput.output_types
-                  .map { |k, _v| [k.humanize, k] }
+                  .map { |k, _v| [_(k.humanize), k] } # k.humanize is sync'd to translation.io via _research_output.erb


I like how you are referencing where we need to look for the translations in case we ever need to update them.

lagoan · 2023-08-14T20:59:46Z

CHANGELOG.md

+
+ - Removed repeated "Sign-up" and "Help" text in "About Us" [#407](https://github.com/portagenetwork/roadmap/issues/407)
+
+ - Fixed broken "Help" link [402](https://github.com/portagenetwork/roadmap/issues/412)


Minor change, add the # simbol to the link.

lagoan · 2023-08-14T21:03:35Z

app/views/static_pages/about_us.html.erb

@@ -34,10 +34,11 @@

      <h2 class="fontsize-h3"><%= _('Getting started') %></h2>

-        <p><%= _("If you have an account please sign in and start creating or editing your DMP.") %></p>
+        <p><%= _("If you have an account, please sign in and start creating or editing your DMP.") %></p>
+        <!--


We can double-check with @isawil to see which copy we like best. We can leave just one instead of waiting for the translation for the missing piece.

Does this make sense?

app/views/static_pages/about_us.html.erb

The commented out code is related to issue #407. It was decided that the commented code is what will be removed.

aaronskiba · 2023-08-15T17:56:46Z

Hi @lagoan, I did the changelog cleanup. Also, after discussing the repeated About Us text with @isawil, the commented-out code is what we will not be keeping. As a result, it has been removed.

lagoan

aaronskiba added 7 commits July 31, 2023 15:02

Edit string to match translation.io entry

e6f83a3

This addresses "About Us" bug mentioned in issue #404

Change "_" to " " for output_types

c72eb12

Enables proper export to translation.io

Add access enums for translation.io sync

14a7afe

Format strings for translation.io sync

13933f4

Add translation of db field

a0d8a44

Need this for translation "Research domain" dropdown elements in "Project Details" panel of Plan

Comment out repeated Sign-Up & Help text

1a62806

This addresses issue #407

Update CHANGELOG.md

633d8a0

Address rubocop offense

115e0ac

aaronskiba requested review from lagoan and 200455939-yashu August 1, 2023 19:26

aaronskiba commented Aug 1, 2023

View reviewed changes

lagoan reviewed Aug 1, 2023

View reviewed changes

aaronskiba added 3 commits August 2, 2023 11:58

Enable translation for 'Research domain' dropdown

4fe1918

Add comments to clarify translation.io sync

cd8ab7f

Prevent translation of subject.gsub('-', ' ')

07741d2

Perhaps the best solution would be to add 'enum selectable_subject' to ResearchOutput. That way we could implement the same logic we used for ResearchOuput.output_types and ResearchOutput.accesses.

aaronskiba force-pushed the aaron/issues/translation branch from e42ce36 to 07741d2 Compare August 2, 2023 19:21

aaronskiba added 2 commits August 2, 2023 15:35

Fix broken help link & add to changelog

57f5d25

run rake translation:sync with prod key

1a00675

200455939-yashu reviewed Aug 4, 2023

View reviewed changes

200455939-yashu previously approved these changes Aug 8, 2023

View reviewed changes

Revert "run rake translation:sync with prod key"

383accf

This reverts commit 1a00675.

lagoan requested changes Aug 14, 2023

View reviewed changes

aaronskiba added 2 commits August 15, 2023 11:41

Remove commented out code

523bac1

The commented out code is related to issue #407. It was decided that the commented code is what will be removed.

Add # symbol to link

41a8140

lagoan approved these changes Aug 16, 2023

View reviewed changes

aaronskiba merged commit cf914f7 into deployment-portage Aug 16, 2023

aaronskiba deleted the aaron/issues/translation branch August 16, 2023 15:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aaron/issues/translation #409

Aaron/issues/translation #409

aaronskiba commented Aug 1, 2023 •

edited

Loading

github-actions bot commented Aug 1, 2023 •

edited

Loading

aaronskiba Aug 1, 2023

aaronskiba Aug 1, 2023

lagoan left a comment

lagoan Aug 1, 2023

aaronskiba Aug 1, 2023

aaronskiba Aug 2, 2023

lagoan Aug 1, 2023

aaronskiba Aug 1, 2023

lagoan Aug 14, 2023

aaronskiba Aug 15, 2023

aaronskiba commented Aug 1, 2023

200455939-yashu Aug 4, 2023

aaronskiba Aug 4, 2023

200455939-yashu Aug 4, 2023

aaronskiba commented Aug 4, 2023

200455939-yashu left a comment

lagoan left a comment

lagoan Aug 14, 2023

lagoan Aug 14, 2023

lagoan Aug 14, 2023

aaronskiba commented Aug 15, 2023

lagoan left a comment


		- Removed repeated "Sign-up" and "Help" text in "About Us" [#407](https://github.com/portagenetwork/roadmap/issues/407)

		- Fixed broken "Help" link [402](https://github.com/portagenetwork/roadmap/issues/412)

Aaron/issues/translation #409

Aaron/issues/translation #409

Conversation

aaronskiba commented Aug 1, 2023 • edited Loading

github-actions bot commented Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lagoan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronskiba commented Aug 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronskiba commented Aug 4, 2023

200455939-yashu left a comment

Choose a reason for hiding this comment

lagoan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronskiba commented Aug 15, 2023

lagoan left a comment

Choose a reason for hiding this comment

aaronskiba commented Aug 1, 2023 •

edited

Loading

github-actions bot commented Aug 1, 2023 •

edited

Loading