watson-developer-cloud / node-red-node-watson

A collection of nodes for the IBM Watson services
Apache License 2.0
82 stars 86 forks source link

Translator ignoring msg. override settings. #224

Closed chughts closed 7 years ago

chughts commented 7 years ago

Bug report for Translator node: If either msg.srclang & msg.destlang are set then these should override any config settings.

chughts commented 7 years ago

This is the code in error

      // checkbox option
      if (config.lgparams2 === false) {
        if (tmpmodel_id.length > 1) {
          result = tmpmodel_id.split('-');
          msg.model_id = tmpmodel_id;
          msg.srclang = result[0];
          msg.destlang = result[1];
        } else {
          msg.model_id = config.domain;
          msg.srclang = config.srclang;
          msg.destlang = config.destlang;
        }
      } else {
        msg.model_id = config.domain;
        msg.srclang = config.srclang;
        msg.destlang = config.destlang;
      }

This code should not be using msg.srclang and / or msg.destlang the way it is, and msg.model_id isn't used anywhere else, so it isn't clear what it is meant to be doing. It should be trying to set msg.domain

Instead this code should be

      // checkbox option
      if (config.lgparams2 === false) {
        if (tmpmodel_id.length > 1) {
          result = tmpmodel_id.split('-');
          //msg.model_id = tmpmodel_id;
          msg.srclang = result[0];
          msg.destlang = result[1];
        } 
      }

Current line 346 (etc. ) will then work as it should.

var srclang = msg.srclang || config.srclang;

I think the line

msg.model_id = tmpmodel_id;

should be

msg.domain = result[2];

The change will require a thorough regression test, through all the configuration options, to verify nothing is broken, and in conjunction with the dashboard translator to verify what msg.model_id is trying to achieve.

The error also applies to the old deprecated translation node.

chughts commented 7 years ago

Fixed in #230