fix(cli): Remove logic converting empty array to undefined in changelog metadata

[kreddlear]

Currently, if either an App type update or an Issue type update is present, the promotion process expects both to be present and errors if not.

If only one is present, the other is converted to undefined instead of an empty array. However, in the promote command, there is a join attempt on both values (code here) -- so if one is undefined, the code returns something like TypeError: issueFeatureUpdates is not iterable.

This error blocks the developer from promoting and doesn't clearly state what they need to do to fix it, which is not ideal.

After some discussion about this, I updated the join so that it accounts for the undefined condition.

Jira ticket with link to report from customer

[eliangcs]

Good catch!

Since appMetadata and issueMetadata can no longer be undefined, the places that make such assumption should be changed too.

Here's one I found: Line 120-129 in promote.js:

      if (
        !appFeatureUpdates &&
        !issueFeatureUpdates &&
        !appBugfixes &&
        !issueBugfixes
      ) {
        this.log(
          `No metadata was found in the changelog. Remember, you can associate the changelog with issues or triggers/actions.\n\n${metadataPromptHelper}`
        );
      }

should be changed to the following (or something equivalent). Otherwise, the this.log in the if statement will be unreachable because appFeatureUpdates and its friends are always truthy (an empty arrays is truthy).

      if (
        _.isEmpty(appFeatureUpdates) &&
        _.isEmpty(issueFeatureUpdates) &&
        _.isEmpty(appBugfixes) &&
        _.isEmpty(issueBugfixes)
      ) {
        this.log(
          `No metadata was found in the changelog. Remember, you can associate the changelog with issues or triggers/actions.\n\n${metadataPromptHelper}`
        );
      }

There are others in promote.js like line 93 and 116.

[kreddlear]

Thank you for the feedback @eliangcs ! I updated those lines so that they'll check if the array is empty, instead of if the value is truthy or not. I checked around and don't see other problematic checks on that, so hopefully we are all good here 🤞

Question for you -- do you think this is the best way of fixing this? Or, would changing the check and the join be better here? I went with this way because I could see others expecting this to be an array and creating similar bugs in the future, but open to alternatives, although I know this is a simple situation!

[standielpls]

Alternatively, does something like this fix the issue? @kreddlear

if (appBugfixes || issueBugfixes) {
  this.log(`Bug fixes: ${[...(appBugfixes ?? []), ...(issueBugfixes ?? [])].join(', ')}`);
}

[kreddlear]

@standielpls yes I think so! I was just playing with something like that, I just remembered the ?? operator in JS exists 😅 I asked ChangHung something similar above, but do you think that would be a cleaner solution? I'm leaning towards yes since it's a one-line change and would mean all those isEmpty checks don't have to apply.

[standielpls]

I think you're right! The isEmpty check is mostly important if we change from a undefined to [] so we should be able to omit those

[standielpls]

I didn't test this locally but I think this solution should work!

[eliangcs]

Assuming you tested it, this is cleaner. Thanks!