search

nrkno / sofie-mos-connection

Sofie MOS Connection: A Part of the Sofie TV Studio Automation System
https://github.com/nrkno/Sofie-TV-automation/
MIT License
22 stars 14 forks source link

chore: fix storyID parameter on roStoryMove #91

Closed JuanPabloLoCoco closed 7 months ago

JuanPabloLoCoco commented 7 months ago

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

codecov-commenter commented 7 months ago

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 :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

nytamin commented 7 months ago

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 commented 7 months ago

@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.