New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IMP]web: translation issue fixed #26952

Closed
wants to merge 1 commit into
base: saas-11.5
from

Conversation

Projects
None yet
5 participants
@tsh-odoo
Copy link
Contributor

tsh-odoo commented Sep 13, 2018

Description of the issue/feature this PR addresses:

Issue-Link: https://www.odoo.com/web#id=1877480&action=333&active_id=1278&model=project.task&view_type=form&menu_id=4720
Pad-Link: https://pad.odoo.com/p/r.59c91d547543c99f3470911805394ea3

Before this commit:
After translating any field, translation warning could not be opened for translation.

After this commit:
Translation can be done of any field from translation warning form view. History of multiple translation pages has also been maintained until page reload.
Pass alertFields as an object so where the key will be the field name, this will allow us to set name attribute on translation alert for the specific field so when that field is clicked translation list is opened in that field's context.

Issue-ID: 1877480

Co-authored-by: Mohammed Shekha msh@openerp.com

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@C3POdoo C3POdoo added the RD label Sep 13, 2018

@tsh-odoo tsh-odoo force-pushed the odoo-dev:saas-11.5-translation-issue-tsh branch from e13d47f to ca15f6d Dec 10, 2018

@Polymorphe57

This comment has been minimized.

Copy link
Contributor

Polymorphe57 commented Jan 10, 2019

@tsh-odoo @msh-odoo Hello guys, I have looked at your PR. I think we are on the right track but I have find a bug in the
onTranslate method of BasicController:
on_reverse_breadcrumb: function () {
if (!
.isEmpty(self.renderer.alertFields)) {
self.renderer.displayTranslationAlert();
}
return false;
},
We should take care of the new nature of alertFields (and use appropriate res_id (event.data.id I think)).
The bug is reproducible in the following way. (In a multi language setting) Suppose you have two products. Start from the kanban view of products. Click on a product and change its name in the form view. Navigate via the notification to the translations and come back via the breadcrumb, then using the pager go to the second product and come back via the breadcrumb to the kanban view, you will get a traceback.

Another problem (less important) is that the rendering of the notification is done more often than necessary (for instance going from a product with an active notification to the kanban view via the breadcrumb). If possible, could you change that?

Last thing, the commit message need to be rewritten. There are several typos and some grammatical errors.

Thank you for your work.
Mathieu

@Polymorphe57

This comment has been minimized.

Copy link
Contributor

Polymorphe57 commented Jan 10, 2019

(DAM)

* @param {Object} alertFields
*/
updateAlertFields: function (alertFields) {
this.alertFields[this.state.res_id] = _.extend(this.alertFields[this.state.res_id] || {}, alertFields);

This comment has been minimized.

@Polymorphe57

Polymorphe57 Jan 10, 2019

Contributor

no need for this.alertFields[this.state.res_id] =

The object this.alertFields[this.state.res_id] is already modified by _.extend

This comment has been minimized.

@msh-odoo

msh-odoo Jan 10, 2019

Contributor

Well, I'll keep as it is, it is quite possible that this.alertFields[this.state.res_id] is undefined in that case _.extend will extend data in blank object i.e. in {}, so in that case it is useful to have:
this.alertFields[this.state.res_id] = _.extend(this.alertFields[this.state.res_id] || {}, alertFields);

This comment has been minimized.

@Polymorphe57

Polymorphe57 Jan 11, 2019

Contributor

you are right of course.

@msh-odoo

This comment has been minimized.

Copy link
Contributor

msh-odoo commented Jan 11, 2019

@tsh-odoo @msh-odoo Hello guys, I have looked at your PR. I think we are on the right track but I have find a bug in the
onTranslate method of BasicController: on_reverse_breadcrumb: function () { if (!.isEmpty(self.renderer.alertFields)) {
self.renderer.displayTranslationAlert();
}
return false;
},
We should take care of the new nature of alertFields (and use appropriate res_id (event.data.id I think)).
The bug is reproducible in the following way. (In a multi language setting) Suppose you have two products. Start from the kanban view of products. Click on a product and change its name in the form view. Navigate via the notification to the translations and come back via the breadcrumb, then using the pager go to the second product and come back via the breadcrumb to the kanban view, you will get a traceback.

Another problem (less important) is that the rendering of the notification is done more often than necessary (for instance going from a product with an active notification to the kanban view via the breadcrumb). If possible, could you change that?

Last thing, the commit message need to be rewritten. There are several typos and some grammatical errors.

Thank you for your work.
Mathieu

@Polymorphe57 It seems that issue is from displayTranslationAlert because this.alertFields will have data for form record but when we do reverse breadcrumb it tries to call this.renderer.displayTranslationAlert where this.alertFields[this.state.res_id] will return undefined, I fixed it, you can have a look now

[IMP]web: translation issue fixed
Before this commit:
After transating any field -> clicking on transalation warning -> come back through breadcrumb do not preserve translation warning.

After this commit:
After transating any field -> clicking on transalation warning -> come back through breadcrumb preserves warning, warning alert remains there until view or record is changed.

alertFields is now object which stores tranlsation alerts per record, so changing record and come back to previous record for which alert fields are available then form renderer display those alert fields until form renderer is destroyed.

Issue-ID: 1877480

Co-authored-by: Mohammed Shekha <msh@openerp.com>

@msh-odoo msh-odoo force-pushed the odoo-dev:saas-11.5-translation-issue-tsh branch from 6069017 to 7979a52 Jan 11, 2019

@Polymorphe57
Copy link
Contributor

Polymorphe57 left a comment

I would rewrite the commit message as follows:

[FIX] web: translation issue fixed

Before this commit:
Editing a translatable field -> clicking on a translation warning ->
come back through breadcrumb do not preserve translation warning.

After this commit:
Editing a translatable field -> clicking on a translation warning ->
come back through breadcrumb preserves translation warning.
Technical note: alertFields is now an object and an attribute of the form renderer
that stores translation alerts per record. The translation alerts
created for a given record are maintained unless
they are closed manually or the renderer is destroyed.

Issue-ID: 1877480

Co-authored-by: Mohammed Shekha msh@openerp.com

* @param {Object} alertFields
*/
updateAlertFields: function (alertFields) {
this.alertFields[this.state.res_id] = _.extend(this.alertFields[this.state.res_id] || {}, alertFields);

This comment has been minimized.

@Polymorphe57

Polymorphe57 Jan 11, 2019

Contributor

you are right of course.

form.destroy();
_t.database.multi_lang = multi_lang;
});

This comment has been minimized.

@Polymorphe57

Polymorphe57 Jan 11, 2019

Contributor

I think it would be good to add a test that controls that the bug you have just fixed does not longer occur.

This comment has been minimized.

@msh-odoo

msh-odoo Jan 11, 2019

Contributor

@Polymorphe57 Checking next/previous pager and then alert is about what we fixed, still, if you want then I will try to open translation listview and then come back using reverse breadcrumb and check whether translation alerts preserved or not

@msh-odoo

This comment has been minimized.

Copy link
Contributor

msh-odoo commented Jan 15, 2019

Work continue at #30223

@hmo-odoo hmo-odoo closed this Jan 15, 2019

@hmo-odoo hmo-odoo deleted the odoo-dev:saas-11.5-translation-issue-tsh branch Jan 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment