New feature #14782: Possibility to override single translations by database #2213

Shnoulle · 2022-01-19T16:49:34Z

Dev: working solution for user with DB access, no GUI

…tabase Dev: working solution for user with DB access, no GUI

application/core/LSMessageSource.php

c-schmitz · 2022-01-19T17:04:23Z

I am very hesitant to have this as a core feature, because I am afraid that this will diminish contributions to translate.limesurvey.org, because why correct a wrong translation (and contribute back) if you can just quickly fix it in your own instance, only.

Shnoulle · 2022-01-19T17:22:02Z

See your comment here : https://bugs.limesurvey.org/view.php?id=17163

That would be nice.

seems i made an error in bug report, i update the title

Shnoulle · 2022-01-19T17:36:53Z

@olleharstedt if it('s OK to be included in core, can you help me on the DB update issue ? I update myself (from 479) : it's OK …

Dev: merge issue with other DB update

Shnoulle · 2022-01-20T09:37:57Z

@olleharstedt if it('s OK to be included in core, can you help me on the DB update issue ? I update myself (from 479) : it's OK …

OK, seems i have it : 6a87152

application/core/LSMessageSource.php

Shnoulle · 2022-01-20T09:47:40Z

I am very hesitant to have this as a core feature, because I am afraid that this will diminish contributions to translate.limesurvey.org, because why correct a wrong translation (and contribute back) if you can just quickly fix it in your own instance, only.

Then , for information :. To have own translation, not translated currently.

With DB (this commit)

Search the english string
Create the string via DB in sourcemessage table
Check the id
Create the translated string in message.

With po (no commit)

Download po file in https://translate.limesurvey.org/projects/
Edit via poedit.
Create ownlocale directory, update

LimeSurvey/application/config/internal.php

Line 210 in fca5dc1

'basePath' => __DIR__.DIRECTORY_SEPARATOR.'..'.DIRECTORY_SEPARATOR.'..'.DIRECTORY_SEPARATOR.'locale'
Put your updated version in this directory.

Shnoulle · 2022-01-20T10:07:20Z

The issue in «Compare upgrade and fresh install» is not related to MYISAM against InnoDB ? Since only InnoDB accept foreignKey.

I can not force foreign key and create model, but since we don't use this model …

@olleharstedt an idea ?

olleharstedt · 2022-01-20T13:40:12Z

Hm

installer/create-database.php

application/helpers/update/updates/Update_480.php

olleharstedt · 2022-01-20T13:44:54Z

I'd like a unit-test of any kind to test this feature, too. If Carsten is willing to merge it, that is.

application/core/LSMessageSource.php

Shnoulle · 2022-01-20T17:45:24Z

I'd like a unit-test of any kind to test this feature, too. If Carsten is willing to merge it, that is.

DB update allowed ?

I'm think of a system :

Create a random string
Check translation : return same string ( gT($string) )
Create another random string
Add the DB translation for string1 to string2 (without model)
Check gT($string) again.
Remove the translation

Allowed ?

Else : need to replace LSMessageSource->loadMessages function.

Dev: remove uneeded update of db version

Dev: fixed, not updatabale by config

…rvey-Shnoulle into develop_translation # Conflicts: # application/core/LSMessageSource.php

olleharstedt · 2022-01-20T19:50:34Z

If you mock getDbConnection, you won't need to touch the db at all. I can show you how. Alternatively, you can do anything in the db as long as you restore the previous state. The problem is different tests affecting each other, as we've seen before.

Shnoulle · 2022-01-21T07:50:34Z

function is now this :

    public function getDbConnection()
    {
        return App()->getDb();
    }

I need to mock->createCommand() and queryAll() right. It's already exist in another test ? More quick to copy/paste ;)

olleharstedt · 2022-01-21T10:12:44Z

I need to mock->createCommand() and queryAll() right. It's already exist in another test ? More quick to copy/paste ;)

Something like that. But what you need to mock differs depending on which function (unit) you test. Let me check...

olleharstedt · 2022-01-21T10:21:41Z

Hm, maybe it's easier to mock getMessagesFromDb() instead.

And then test getMessagesFromDb() with real db data (which is deleted after test is run, so called fixture).

olleharstedt · 2022-01-21T10:27:59Z

Mock docs for PHPUnit: https://phpunit.readthedocs.io/en/9.5/test-doubles.html?highlight=mock

I don't think we're using version 9.5 tho, you'd have to double check.

olleharstedt · 2022-02-04T10:41:05Z

public function testLsMessageSource()
{
    $messageSource = new LSMessageSource();
    $messageSource->basePath = YII_PATH . '/../locale/';
    $ref = new ReflectionClass('LSMessageSource');
    $met = $ref->getMethod('loadMessages');
    $met->setAccessible(true);
    $messages = $met->invokeArgs($messageSource, ['', 'en']);
    $this->assertNotEmpty($messages);
}

One example of calling loadMessages (even tho it's protected) and test the result.

Shnoulle · 2022-02-04T10:43:28Z

One example of calling loadMessages (even tho it's protected) and test the result.

Interest ? Testing a 2 functions via phpunit …

I can add if you want , but didn't see the interest.

The current test check gT function.

olleharstedt · 2022-02-04T10:45:21Z

Well, optimally, you're supposed to test each possible path in the function - with cache, without cache, with db, without db, etc. That will be hard only using gT(). Just using gT(), you might not get 100% or 50% coverage.

You're also supposed to do negative tests - testing that the function fails correctly.

olleharstedt · 2022-02-04T10:49:06Z

Hm, on another note, I'm not able to repeat your test multiple times locally. It works once, but then I get this:

1) ls\tests\LSMessageSourceTest::testUpdatedString
Message translation by DB is broken
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'translatedmessageP8YlHKPocMV98wFEZeolVgWhTYm5FbadrKkzYM~7UX'
+'sourcemessageCgfg5jHVZ5MhS_F_pQ4D7LFi~QRv7SY0s9gL9dqjmn'

Related to random string?

Shnoulle · 2022-02-04T10:51:19Z

Well, optimally, you're supposed to test each possible path in the function - with cache, without cache, with db, without db, etc. That will be hard only using gT(). Just using gT(), you might not get 100% or 50% coverage.

And with your test : what is the coverage ? What do you test ?

I can not create test for all previous feature each time : it's not part of the coverage… i have to test only the new feature.

What is the coverage here : «Possibility to override single translation by db» OK to add caching (translation not updated with DB update only), but don't understand witout db …

Shnoulle · 2022-02-04T10:52:43Z

Hm, on another note, I'm not able to repeat your test multiple times locally. It works once, but then I get this:
1) ls\tests\LSMessageSourceTest::testUpdatedString
Message translation by DB is broken
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'translatedmessageP8YlHKPocMV98wFEZeolVgWhTYm5FbadrKkzYM~7UX'
+'sourcemessageCgfg5jHVZ5MhS_F_pQ4D7LFi~QRv7SY0s9gL9dqjmn'
Related to random string?

Related to caching … cache for 1 hour by default.

With caching : we don't get the new translation : we return the cache.

olleharstedt · 2022-02-04T11:05:13Z

Related to caching … cache for 1 hour by default.

OK, so I need to disable caching? You see one benefit at once with my approach: just set $messageSource->cachingDuration = 0 in the test code. :)

Shnoulle · 2022-02-04T11:15:59Z

Related to caching … cache for 1 hour by default.

OK, so I need to disable caching? You see one benefit at once with my approach: just set $messageSource->cachingDuration = 0 in the test code. :)

I already think we need to check if caching work .

Best seems to

Reset caching at start
Create the new translation in DB
Call existing po DB string : (Submit ? Yes ? anything can be updated at a time
Call a random string : must return new translation
Update the random string in DB : must retuen previous one

The lack is update core string , then if i can add a translation for Submit : it's OK.

But in this condition : if user have updated his own language : didn't work …

I create all of this

olleharstedt · 2022-02-04T11:20:02Z

I already think we need to check if caching work .

Maaaybe. Caching is part of Yii, and we should assume it's working (Yii has its own test suite).

Shnoulle · 2022-02-04T11:34:17Z

Well, optimally, you're supposed to test each possible path in the function - with cache, without cache, with db, without db, etc. That will be hard only using gT(). Just using gT(), you might not get 100% or 50% coverage.

You're also supposed to do negative tests - testing that the function fails correctly.

I already think we need to check if caching work .

Maaaybe. Caching is part of Yii, and we should assume it's working (Yii has its own test suite).

????

olleharstedt · 2022-02-04T11:47:44Z

Oh, I'm contradicting myself. 🤦

Shnoulle · 2022-02-04T12:31:36Z

we should assume

Like : we should assume po/mo file work (or not ?)

The «we should assume» the default way to get translation use LSMessageSource can not be assumed.

gT whole system can be updated too easily …

olleharstedt · 2022-02-04T13:00:09Z

Like : we should assume po/mo file work (or not ?)

I think, assuming po/mo files are correct, is the same as mocking the access to mo/po files in the unit test. Which is one way to do it. But it's OK to just access those files, too.

Shnoulle · 2022-02-04T13:52:00Z

Like : we should assume po/mo file work (or not ?)

I think, assuming po/mo files are correct, is the same as mocking the access to mo/po files in the unit test. Which is one way to do it. But it's OK to just access those files, too.

Again : mocking a 2 function call in same class … i don't really understand the point here …

I don't understand what you want …

Shnoulle · 2022-02-06T16:38:43Z

One example of calling loadMessages (even tho it's protected) and test the result.

Any idea to mock loadMessagesFromDb here ? Because without this : i didn't test this feature …

Shnoulle · 2022-02-06T17:24:51Z

Check if po translation work + check if file cache (by default) work + DB new source + DB replace source.

I don't see what i can do now …

olleharstedt · 2022-02-06T20:14:32Z

Great! Is it now possible to run the test multiple times locally?

Shnoulle · 2022-02-07T07:17:28Z

Great! Is it now possible to run the test multiple times locally?

I add a resetCache function.
Default behaviour is to reset DB cache only :

LimeSurvey/tests/TestBaseClass.php

Line 40 in 30d8f21

\Yii::app()->db->schema->refresh();

I add https://github.com/LimeSurvey/LimeSurvey/pull/2213/files#diff-3cc131b4d608fd992d775790fbc8c5f5b04a49347a85a54390af3291c6b504e0R44
(PS : maybe we need an API function for this)

olleharstedt · 2022-02-07T11:10:35Z

OK, gonna test locally again...

Shnoulle · 2022-02-07T11:21:11Z

LSMessage Source (ls\tests\LSMessageSource)
✔ Updated string

olleharstedt · 2022-02-07T11:29:44Z

Works multiple times for me locally 👍 🌟

…into develop_translation

New feature #14782: Possibility to override single translations by da…

3bef3d0

…tabase Dev: working solution for user with DB access, no GUI

Shnoulle commented Jan 19, 2022

View reviewed changes

application/core/LSMessageSource.php Outdated Show resolved Hide resolved

Shnoulle commented Jan 19, 2022

View reviewed changes

application/core/LSMessageSource.php Outdated Show resolved Hide resolved

Shnoulle commented Jan 19, 2022

View reviewed changes

application/core/LSMessageSource.php Outdated Show resolved Hide resolved

Dev: fix installer

07446c7

Dev: Fixed issue end by bad DB version in some case.

6a87152

Dev: merge issue with other DB update

Shnoulle commented Jan 20, 2022

View reviewed changes

application/core/LSMessageSource.php Show resolved Hide resolved

olleharstedt reviewed Jan 20, 2022

View reviewed changes

installer/create-database.php Outdated Show resolved Hide resolved

olleharstedt reviewed Jan 20, 2022

View reviewed changes

application/helpers/update/updates/Update_480.php Outdated Show resolved Hide resolved

olleharstedt reviewed Jan 20, 2022

View reviewed changes

application/helpers/update/updates/Update_480.php Outdated Show resolved Hide resolved

olleharstedt reviewed Jan 20, 2022

View reviewed changes

application/core/LSMessageSource.php Outdated Show resolved Hide resolved

Shnoulle added 5 commits January 20, 2022 19:14

Dev: remove constraint (use at your own risk)

3831bb6

Dev: remove uneeded update of db version

Dev: remove public database name and all connection part

3f9b69a

Dev: fixed, not updatabale by config

Dev: remove public database name and all connection part

800a764

Dev: fixed, not updatabale by config

Dev: oups, lost {{ }}

b829f37

Dev: Merge branch 'develop_translation' of github.com:Shnoulle/LimeSu…

5d663b1

…rvey-Shnoulle into develop_translation # Conflicts: # application/core/LSMessageSource.php

Shnoulle marked this pull request as draft February 4, 2022 11:18

Shnoulle marked this pull request as ready for review February 4, 2022 14:06

Shnoulle marked this pull request as draft February 5, 2022 18:39

Dev: add po checker + cache checker + replace

086b7a5

Shnoulle marked this pull request as ready for review February 6, 2022 17:22

Shnoulle requested a review from olleharstedt February 6, 2022 17:22

olleharstedt approved these changes Feb 7, 2022

View reviewed changes

Merge branch 'develop' of https://github.com/LimeSurvey/LimeSurvey.git …

8f53fdc

…into develop_translation

c-schmitz merged commit 1ed3005 into LimeSurvey:develop Feb 8, 2022

New feature #14782: Possibility to override single translations by database #2213

New feature #14782: Possibility to override single translations by database #2213

Conversation

Shnoulle commented Jan 19, 2022

c-schmitz commented Jan 19, 2022

Shnoulle commented Jan 19, 2022

Shnoulle commented Jan 19, 2022

Shnoulle commented Jan 20, 2022

Shnoulle commented Jan 20, 2022 • edited Loading

Shnoulle commented Jan 20, 2022 • edited Loading

olleharstedt commented Jan 20, 2022

olleharstedt commented Jan 20, 2022 • edited Loading

Shnoulle commented Jan 20, 2022 • edited Loading

olleharstedt commented Jan 20, 2022

Shnoulle commented Jan 21, 2022

olleharstedt commented Jan 21, 2022

olleharstedt commented Jan 21, 2022 • edited Loading

olleharstedt commented Jan 21, 2022

olleharstedt commented Feb 4, 2022

Shnoulle commented Feb 4, 2022

olleharstedt commented Feb 4, 2022 • edited Loading

olleharstedt commented Feb 4, 2022

Shnoulle commented Feb 4, 2022

Shnoulle commented Feb 4, 2022 • edited Loading

olleharstedt commented Feb 4, 2022

Shnoulle commented Feb 4, 2022

olleharstedt commented Feb 4, 2022

Shnoulle commented Feb 4, 2022

olleharstedt commented Feb 4, 2022

Shnoulle commented Feb 4, 2022

olleharstedt commented Feb 4, 2022

Shnoulle commented Feb 4, 2022

Shnoulle commented Feb 6, 2022

Shnoulle commented Feb 6, 2022

olleharstedt commented Feb 6, 2022

Shnoulle commented Feb 7, 2022

olleharstedt commented Feb 7, 2022

Shnoulle commented Feb 7, 2022

olleharstedt commented Feb 7, 2022

Shnoulle commented Jan 20, 2022 •

edited

Loading

Shnoulle commented Jan 20, 2022 •

edited

Loading

olleharstedt commented Jan 20, 2022 •

edited

Loading

Shnoulle commented Jan 20, 2022 •

edited

Loading

olleharstedt commented Jan 21, 2022 •

edited

Loading

olleharstedt commented Feb 4, 2022 •

edited

Loading

Shnoulle commented Feb 4, 2022 •

edited

Loading