chore: fix storyID parameter on roStoryMove

[JuanPabloLoCoco]

About the Contributor

Type of Contribution

This is a:

Bug fix

Current Behavior

When you send a roStoryMove with only one storyID the roElementAction message that is created is wrong. That is because it will create the element_source using the first letter of your data.roElementMove.storyID (aka storyID), and will create the element_target using the second letter of the storyID

New Behavior

The message will detect if data.roElementAction.storyID is an Array or a String. If it is an array, it will use the array. If it is a string, will convert it to an array.

Testing Instructions

Other Information

Status

• PR is ready to be reviewed.
• The functionality has been tested by the author.
• Relevant unit tests has been added / updated.
• Relevant documentation (code comments, system documentation) has been added / updated.

[codecov-commenter]

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (a87681f) 77.08% compared to head (5b997ef) 77.08%.

Files	Patch %	Lines
packages/connector/src/MosDevice.ts	85.71%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #91      +/-   ##
==========================================
- Coverage   77.08%   77.08%   -0.01%     
==========================================
  Files          65       65              
  Lines        3522     3535      +13     
  Branches      799      812      +13     
==========================================
+ Hits         2715     2725      +10     
- Misses        724      727       +3     
  Partials       83       83              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[nytamin]

Thanks for the PR @JuanPabloLoCoco !

We discussed this a little bit internally and have come up with the conclusions below, please give them a read and let us know your thoughts.

roStoryMove

A missing second <storyID> tag in roStoryMove is technically out of spec (according to the DTD <!ELEMENT roStoryMove (roID, storyID, storyID)>), so we'll only allow it if running in non-strict mode.

I've pushed a commit to this PR with this change.

roStoryMoveMultiple

This is a wierd one, where the docs are quite ambigous, I think.

From the docs:

This command allows one or more stories to be moved to a new location in the playlist. The last storyID is the ID of the story before which to insert the new stories. All remaining storyIDs identify stories to insert at that location. The resulting playlist has all the moved stories appearing in the order specified in the command before the reference story. If the last storyID is blank, the stories are moved to the end of the playlist.

A single <storyID> tag in roStoryMoveMultiple is technically within spec (<!ELEMENT roStoryMoveMultiple (roID, storyID+ )>).

If I where to ready the documentation LITERALY I come to the conclusion that this means that: "Zero stories should be moved to the location specified by the single tag" - which of course makes no sense since it is a no-op.

In non strict mode: I think that we should (as you propose) make the assumtion that the sender intended to send a second (blank) <storyID> tag, but just omitted it instead.
In strict mode: This will be a no-op.

---

Let me know what you think!
I've pushed some changes to account for the reasonings above (to be adjusted if needed).

[JuanPabloLoCoco]

@nytamin I proposed the changes just to have more defensive programming. We should indeed believe that the sender will not be malicious, but who knows?

I agree with your comments.