Skip to content

Conversation

@cwant
Copy link
Contributor

@cwant cwant commented Jun 25, 2024

Summary of changes

This adds a "Language of Instruction" feature to events, as per #994
The languages supported are the 24 official languages of the EU:
https://european-union.europa.eu/principles-countries-history/languages_en

Motivation and context

We (Canada) need this feature for the bilingual instance of TeSS we are working on, so thought it might be better to push this upstream too.

We'll see how this goes, and then add a similar feature for learning materials in the future.

Checklist

  • [*] I have read and followed the CONTRIBUTING guide.
  • [*] I confirm that I have the authority necessary to make this contribution on behalf of its copyright owner and agree
    to license it to the TeSS codebase under the
    BSD license.

@cwant
Copy link
Contributor Author

cwant commented Jun 26, 2024

Oops, looks like I have config.i18n.raise_on_missing_translations = true turned off here. Will have to either rework the failing test or do something else with the helper function.

@fbacall
Copy link
Member

fbacall commented Jun 28, 2024

Thanks for this! Not had a chance to have a detailed review yet, but I was wondering did you consider using a gem to provide the list of language codes/names? e.g. https://github.com/xwmx/iso-639

@cwant
Copy link
Contributor Author

cwant commented Jun 28, 2024

I had not considered getting it from a gem. Some thoughts on that specific gem:

  • Seems to only support French/English language names, which is fine for my specific use case, but lacks some generalization (it would be nice if the gem used I18n for the language names)
  • I'm new to this, so I don't really know what the alpha2/alpha3 stuff is, or how (or if) it'll get used.
  • For our cases, we will want to consider a subset of the languages anyways, so it's just as easy to list them. In my case two languages, in the EU case maybe 24? Having a menu with 487 languages (most of which are obscure) might be a bit much. I would assume that at some point this goes through a dictionary (app/dictionaries), so I don't know. I guess the gem could be used, then languages restricted in tess.yml.
    That said, I'd be happy to look into it deeper in the context of a review.

Copy link
Member

@fbacall fbacall left a comment

Choose a reason for hiding this comment

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

Thanks for this great PR - super clean implementation.

Sorry for not mentioning before (because I'd forgotten about it), but we do actually have some existing code for handling languages that was used in the "Trainer" profile feature:

def language_options_for_select(priority = priority_languages)
priors = []
others = []
I18nData.languages.each do |lang|
next unless lang and !lang.empty?
value = lang[1]
key = lang[0]
# Rails.logger.debug "language: key[#{key}] value[#{value}]"
if priority and !priority.empty? and priority.include?(key)
priors << [value, key]
else
others << [value, key]
end
end
priors + others
end
def language_label_by_key(key)
return unless key and !key.nil?
I18nData.languages.each do |lang|
return lang[1] if lang[0] == key
end
end

This was added by the guys working on the DReSA instance, but we haven't yet enabled it on our ELIXIR deployment.

Perhaps you could re-use some of that for handling the language keys and names?

(The language selection form on the trainer profile does currently display a huge list of languages like you were worried about, but could be filtered down)

Comment on lines 14 to 19

# TODO: is there a better way?
if name == 'language'
title = render_language_name(value)
end
title ||= (html_options.delete(:title) || truncate(value.to_s, length: 50))
Copy link
Member

Choose a reason for hiding this comment

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

Not currently, but I would move this logic into a method that takes the facet name and value and handles the special case for language, otherwise falls back to truncate(value.to_s, length: 50) as below.

extend ActiveSupport::Concern

included do
validates :language, controlled_vocabulary_or_nil: { dictionary: 'LanguageDictionary' }
Copy link
Member

Choose a reason for hiding this comment

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

You can pass a allow_nil option to the existing vocab validator to get this behaviour:

controlled_vocabulary: { dictionary: 'LanguageDictionary', allow_nil: true }

@cwant
Copy link
Contributor Author

cwant commented Jul 5, 2024

Thanks for the review!

  • I've gotten rid of the controlled_vocabulary_or_nil business to use the allow_nil: true option of controlled_vocabulary
  • I've made a method to handle the Event title facet logic
  • I've used the I18nData for the localized language names. Some details and comments/questions about that:
    • I've flip-flopped on this decision, but I still keep a dictionary to handle the allowed language names (a list with hundreds of names isn't great UX). I can change this if need be. Another option I considered is listing some codes in tess.yml as being the allowable ones. The default I went with is the 24 official EU languages.
    • I'm not super happy with some of the design of I18nData. They go with iso-639-1 (two letter codes instead of three), but for some reason they decided to upper case the codes (e,g., 'EN' instead of 'en'). I decided to go with the lower case version in the standard, but this will be a deviation from the language field in the Trainer model. So yeah, this isn't great. What direction would you prefer to go? (consistency with the other table, or consistency with the standard?)
    • I capitalize language names in the form/facets, although this might be a deviation from what is done in some locales
    • Some of the language names from I18nData aren't great. Most noticeable is the Greek one:

lang-menu

@fbacall
Copy link
Member

fbacall commented Jul 9, 2024

  • I've flip-flopped on this decision, but I still keep a dictionary to handle the allowed language names (a list with hundreds of names isn't great UX). I can change this if need be. Another option I considered is listing some codes in tess.yml as being the allowable ones. The default I went with is the 24 official EU languages.

I think I like the idea of them being listed in an array in tess.yml, that way the order that the languages appear can also be changed. We can provide a default of the 24 EU languages in tess.example.yml.

  • I'm not super happy with some of the design of I18nData. They go with iso-639-1 (two letter codes instead of three), but for some reason they decided to upper case the codes (e,g., 'EN' instead of 'en'). I decided to go with the lower case version in the standard, but this will be a deviation from the language field in the Trainer model. So yeah, this isn't great. What direction would you prefer to go? (consistency with the other table, or consistency with the standard?)

I agree it's not very nice, although the spec says they are case-insensitive. I think we should go with lower case. We can just down-case the keys from the trainers table when they are read to be consistent (or write a migration to convert the stored data).

  • I capitalize language names in the form/facets, although this might be a deviation from what is done in some locales
  • Some of the language names from I18nData aren't great. Most noticeable is the Greek one

How about we list the EU language names under a language key in en.yml, then when rendering the language name it checks there first and falls back to the I18nData name if it is missing?

* remove languages.yml
* override language names from I18nData with `I18n.t("languages.#{code}")`
@cwant
Copy link
Contributor Author

cwant commented Jul 9, 2024

This all sounds reasonable. I've integrated all of those suggestions in the latest push.

Copy link
Member

@fbacall fbacall left a comment

Choose a reason for hiding this comment

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

Just a small simplification, otherwise looks good

@fbacall fbacall merged commit ddccfde into ElixirTeSS:master Jul 10, 2024
@cwant cwant mentioned this pull request Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants