Tryton - Issues

 

Issue7904

Title sao's Iconfactory.get_icon_url does not return a promise
Priority feature Status need-eg
Superseder Nosy List ced, nicoe, reviewbot
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 reviewbot.

Messages
review70461002 updated at https://codereview.tryton.org/70461002/#ps20001
msg45197 (view) Author: [hidden] (nicoe) (Tryton committer) (Tryton translator) 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) (Tryton translator) 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) (Tryton translator) 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) (Tryton translator) 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) (Tryton translator) 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
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

Showing 10 items. Show all history (warning: this could be VERY long)