search

plone / plone.app.discussion

General commenting system for Plone content
https://pypi.org/project/plone.app.discussion
16 stars 47 forks source link

[WIP] - Fix title translation #167

Open cekk opened 4 years ago

mister-roboto commented 4 years ago

@cekk thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

cekk commented 4 years ago

Title were always create with the default label and not translated into current language.

Should i have to create an upgrade-step to update already created comments?

tisto commented 4 years ago

@cekk an upgrade step would be nice if you have the time for it.

mauritsvanrees commented 4 years ago

Is this still Work In Progress? Just from looking at the changes, it seems good and complete, and should not need an upgrade step. I have not tried it out yet.

mauritsvanrees commented 4 years ago

Actually, I am not sure how to test this. With plone.app.discussion master in a Dutch site I enabled comments on Page, added an anonymous comment without any name, and the name shown was "Anoniem", the Dutch translation of "Anonymous". So this part is working, but maybe the Title is something else. Or does this need a multilingual site to properly test?

Ah, I see: the way to test this, is to go to the search control panel and allow searching comments. Then I see in the search results a title like "Anonymous on Page Title", where "Anonymous on" would be translated in Dutch with your fix. And an upgrade step would then be needed to make this active. That is a corner case, but I guess some people do use it this way.

There are various spots in the same comment.py file where translate is called without context, so those should be fixed too, preferably. Specifically, a few lines down from your fix, the next translate call needs this, otherwise I see in Dutch "Anoniem on Page Title", where "on" is not translated into the Dutch "over".

Actually, when I add the context to this call, and rebuild the catalog, the search results show "${creator} over Page Title". So this literally contains ${creator}.

Okay, thanks for starting this, but this indeed needs more work, so the "WIP" in the title is correct. :-) BTW, you can revert the PR into draft status if you want. There is a link below the reviewers.