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

Replace "name" with "title" fields in tables #3581

Closed ywarnier closed 9 months ago

ywarnier commented 3 years ago

Chamilo 2.0 is a great opportunity to clean the mess up a little bit in our database structure. At the moment, there is a mix between fields called "name" and fields called "title", where they all represent the textual name of the item represented by the table row.

Move all "name" fields to "title", to sanitize this.

Making this a standard with help with the capability to predict the field name and, as such, will improve development speed in the long run.

Beware that some of these fields are also indexed. Indexes should be modified too.

Tables with "name":

So that's 48 changes, vs 39 'title' fields (and 38 pure "name" fields).

christianbeeznest commented 1 year ago

Request is done in this PR https://github.com/chamilo/chamilo-lms/pull/4706

NicoDucou commented 1 year ago

I switch to branch GH-3581-2 and update the table schema :

root@testing:/var/www/testing24.beeznest.com/www# php8.1 bin/console doctrine:schema:update --force

 [WARNING] Not passing the "--complete" option to "doctrine:schema:update" is deprecated and will not be supported when 
           using doctrine/dbal 4                                                                                        

 Updating database schema...

     89 queries were executed

 [OK] Database schema updated successfully!                                                                             

After this I checked the table and column names in the database :

SELECT DISTINCT TABLE_NAME, COLUMN_NAME FROM INFORMATION_SCHEMA.COLUMNS WHERE column_name LIKE '%name%'; It seems all good, @ywarnier I just wonder if those fields should stay with "name" or if it should be changed to "title" in thos tables :

Then I did a quick review of code and see that the migrations are present. Appart from the remarks above to define if we should include those extra tables, changes in the PR seems correct and we will see once accepted if there was a missing change that generate error when usgin the funcionality since I can not test all the plateform to validate all functionalities.

So @ywarnier I let you confirm for the tables indicated above and reassign to @christianbeeznest to do those changes and to adapt the PR because there are conflicts now so it can not be merged.

ywarnier commented 9 months ago

Retaking your list here:

ywarnier commented 9 months ago

Fixed through https://github.com/chamilo/chamilo-lms/commit/faeca062624e4fa0c0eba86e54c25e490c000f49