chamilo / chamilo-lms

Chamilo is a learning management system focused on ease of use and accessibility
https://chamilo.org
GNU General Public License v3.0
798 stars 480 forks source link

C2: Settings current titles, comments and migrations #5313

Closed ywarnier closed 3 months ago

ywarnier commented 6 months ago

In the current dev version of Chamilo 2 (i.e. master branch), there are issues with the "title" and "comment" columns of the settings_current table.

In Chamilo 1., the institution variable defined title InstitutionTitle and comment InstitutionComment as names for language variables. However, these variables have now been converted in Chamilo 2 to the values* of the translation in English. So InstitutionTitle has become Organization name, and InstitutionComment has become The name of the organization (appears in the header on the right).

The old translation files have now been removed from C2's repository (due to he fact that we didn't catch the missing comments) and thus the relationship between the InstitutionTitle variable name and the Organization name gettext ID is now lost unless we re-use them to create the missing variables. We would only need the old main/lang/english/trad4all.inc.php to get the link back, though, and only to prepare the migration/install (so it doesn't need to be permanently added back to Git).

So now we have 2 "situations" to clean up:

  1. the settings_current table should change the definition of the title and comment fields to "text" (instead of varchar(255))
  2. fresh installs of Chamilo 2 create settings_current where title = variable. This is not correct. 2.1. When the variable already existed in Chamilo 1, it should use the English translation of the language variable for that settings_current.title in the title column, and the English translation of the language variable for that settings_current.comment in the comment column. For example, variable "administrator_email" should use the English translation of the old "title" column (emailAdministratorTitle), which is "Portal Administrator: e-mail" (this variable already exists in Gettext, so just use get_lang() to translate it on screen), and the "comment" was "emailAdministratorComment" and should now be "The e-mail address of the Platform Administrator (appears in the footer on the left)" (already exists in Gettext). 2.2. When the variable did not exist in Chamilo 1 (in settings_current) but it was already present in configuration.dist.php, then it is more complicated (you can use the list I will create below for that). 2.3. If the variable was added in C2 (did not exist anywhere previously), then please report it so I can provide a title and comment for them.
  3. migrations from Chamilo 1 maintain the "title" and "comment" fields as they were in Chamilo 1, so these have to be converted to gettext format. An update table should be setup in the migration to convert "InstitutionTitle" to "Organization name" and "InstitutionComment" to "The name of the organization (appears in the header on the right)". This can probably be done by scanning main/lang/english/trad4all.inc.php real quick and building a table then copy-pasting only the ones used in settings_current.title or .comment it in the code of the migration.

There's also a bit of fun with variables having been renamed in the transition from C1 to C2 in src/CoreBundle/Settings/SettingsManager.php::renameVariable(), but there's only about 22 of those, so not a huge deal to check "by hand".

ywarnier commented 6 months ago

Doing and reviewing the list takes a lot of time, but I'm adding new tasks progressively to deal with a few issues.

christianbeeznest commented 6 months ago

the settings_current table should change the definition of the title and comment fields to "text" (instead of varchar(255))

It is done in this commit https://github.com/chamilo/chamilo-lms/pull/5333/commits/f1b25a9dd18e994b18e6f661c31c8c59558798f4

christianbeeznest commented 6 months ago
2. fresh installs of Chamilo 2 create settings_current where title = variable. This is not correct.
2.1. When the variable already existed in Chamilo 1, it should use the English translation of the language variable for that settings_current.title in the title column, and the English translation of the language variable for that settings_current.comment in the comment column. For example, variable "administrator_email" should use the English translation of the old "title" column (emailAdministratorTitle), which is "Portal Administrator: e-mail" (this variable already exists in Gettext, so just use get_lang() to translate it on screen), and the "comment" was "emailAdministratorComment" and should now be "The e-mail address of the Platform Administrator (appears in the footer on the left)" (already exists in Gettext).

It is checked in this commit https://github.com/chamilo/chamilo-lms/pull/5338/commits/52b407ae57c75370685865c0e80ad5cef66bb14c

christianbeeznest commented 6 months ago

3. migrations from Chamilo 1 maintain the "title" and "comment" fields as they were in Chamilo 1, so these have to be converted to gettext format. An update table should be setup in the migration to convert "InstitutionTitle" to "Organization name" and "InstitutionComment" to "The name of the organization (appears in the header on the right)". This can probably be done by scanning main/lang/english/trad4all.inc.php real quick and building a table then copy-pasting only the ones used in settings_current.title or .comment it in the code of the migration.

It is created a migration file to update titles and comments for settings from configuration.php , it is partial in this PR https://github.com/chamilo/chamilo-lms/pull/5345/commits/76b530f897e2e838c67bb83e4de7a579b5a53000 , it will be merged when all variables are checked.

ywarnier commented 5 months ago

As stated privately, there are 4 issues remaining. The first 2 can be solved at a later phase but the last 2 probably need to be solved before alpha: