Tryton - Issues

 

Issue7904

Title sao's Iconfactory.get_icon_url does not return a promise
Priority feature Status resolved
Superseder Nosy List ced, nicoe, reviewbot, roundup-bot
Type behavior Components sao
Assigned To nicoe Keywords review
Reviews 70461002
View: 70461002

Created on 2018-12-05.18:32:11 by nicoe, last changed by roundup-bot.

Messages
New changeset 95dbcdfd3b53 by Nicolas ?vrard in branch 'default':
Return an empty URL when the icon name is invalid
https://hg.tryton.org/tryton-env/rev/95dbcdfd3b53
New changeset 7d0808912c70 by Nicolas ?vrard in branch 'default':
Return an empty URL when the icon name is invalid
https://hg.tryton.org/sao/rev/7d0808912c70
review70461002 updated at https://codereview.tryton.org/70461002/#ps267401006
review70461002 updated at https://codereview.tryton.org/70461002/#ps277371002
review70461002 updated at https://codereview.tryton.org/70461002/#ps20001
msg45197 (view) Author: [hidden] (nicoe) (Tryton committer) Date: 2018-12-07.18:14:00
* Cédric Krier [2018-12-05 20:15:51]:
> Where is such call made?

I updated the review to fix the calls that are responsible for this
and to throw an exception instead of returning nothing.
msg45173 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2018-12-05.20:15:51
Where is such call made?
msg45172 (view) Author: [hidden] (nicoe) (Tryton committer) Date: 2018-12-05.20:01:49
* Cédric Krier [2018-12-05 19:32:19]:

> I do not understand how forgetting an attribute can lead to using
> this attribute.

If the icon is a field and the value of the field is the empty string
for example.

> Also I prefer crash when something is wrong than silently ignoring.

It could be considered that an empty icon name is not a mistake thus
returning a rejected promise would just prevent displaying any icon.
msg45170 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2018-12-05.19:32:18
I do not understand how forgetting an attribute can lead to using this attribute.
Also I prefer crash when something is wrong than silently ignoring.
msg45169 (view) Author: [hidden] (nicoe) (Tryton committer) Date: 2018-12-05.19:30:15
It results in a traceback in the browser that won't continue the processing in some case, it could result in a weird behaviour when the reason for this is the fact that the dev has forgotten to fill the icon attribute of a view.

So I think that rejecting the promise will allow to display nothing while continuing the processing.
msg45165 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2018-12-05.19:03:56
I'm not sure that a reject is the right answer neither. I think raising an exception is better as it is a programmatic error.
msg45164 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2018-12-05.18:58:05
So there is no bug.
msg45163 (view) Author: [hidden] (nicoe) (Tryton committer) Date: 2018-12-05.18:56:03
Calling .done() on undefined won't work.
It's a matter of defensive programming.
msg45162 (view) Author: [hidden] (ced) (Tryton committer) (Tryton translator) Date: 2018-12-05.18:44:37
What is the problem?
New review70461002 at https://codereview.tryton.org/70461002/#ps1
msg45160 (view) Author: [hidden] (nicoe) (Tryton committer) Date: 2018-12-05.18:32:10
get_icon_url can be called with an empty name in some cases I think it should return a rejected promise in this case rather than returning undefined.
History
Date User Action Args
2019-06-04 17:38:33roundup-botsetmessages: + msg49991
2019-06-04 17:38:31roundup-botsetstatus: need-eg -> resolved
nosy: + roundup-bot
messages: + msg49990
2019-06-04 17:16:11reviewbotsetmessages: + msg49986
2019-06-04 16:14:43reviewbotsetmessages: + msg49983
2018-12-07 18:42:52reviewbotsetmessages: + msg45200
2018-12-07 18:14:01nicoesetmessages: + msg45197
2018-12-05 20:15:51cedsetmessages: + msg45173
2018-12-05 20:01:49nicoesetmessages: + msg45172
2018-12-05 19:32:19cedsetmessages: + msg45170
2018-12-05 19:30:15nicoesetmessages: + msg45169
2018-12-05 19:03:56cedsetmessages: + msg45165
2018-12-05 18:58:05cedsetpriority: bug -> feature
type: crash -> behavior
messages: + msg45164
2018-12-05 18:56:04nicoesetmessages: + msg45163
2018-12-05 18:44:37cedsetstatus: chatting -> need-eg
nosy: + ced
messages: + msg45162
2018-12-05 18:37:35reviewbotsetstatus: unread -> chatting
nosy: + reviewbot
messages: + msg45161
2018-12-05 18:37:34reviewbotsetreviews: 70461002
keyword: + review
2018-12-05 18:32:11nicoecreate