Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added multiArchiveTag to startArchive #280

Merged
merged 3 commits into from
Nov 7, 2022
Merged

Added multiArchiveTag to startArchive #280

merged 3 commits into from
Nov 7, 2022

Conversation

manchuck
Copy link
Contributor

@manchuck manchuck commented Sep 9, 2022

What is this PR doing?

This updates startArchive to accept in the multiArchiveTag parameter.

How should this be manually tested?

opentok.startArchive(
    sessionId, 
    {
        name: 'My awesome session',
        multiArchiveTag: 'tagging this session',
    },
    (err, archive) => {
        if (err) {
            console.err(err);
            return;
        }

        console.log(archive);
);

What are the relevant tickets?

DEVX-6489

Copy link
Collaborator

@jeffswartz jeffswartz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some things are missing:

  • Add docs comments for the multiArchiveTag option of the OpenTok.startArchive() method.
  • Add mutliArchiveTag as a property of the Archive class (and add its docs).
  • Implement the multiBroadcastTag option of the OpenTok.startBroadcast() method and the multiArchiveTag property of the Broadcast object (and add docs for both). Or will this be a separate PR?
  • This PR has merge conflicts.
  • Should this PR target the dev branch instead of main. Also, we need to rev the version (to 2.15.0, I think).

@manchuck manchuck changed the base branch from main to dev October 3, 2022 15:37
@manchuck
Copy link
Contributor Author

manchuck commented Oct 3, 2022

@jeffswartz , I was unable to find the merge conflict. Could it be that changing the base resolved it?

multiBroadcastTag was not part of the ticket. @pardel is that part of a different ticket?

@jeffswartz I'm not sure where to add the property to the class. I did check for this first and did not see a need to add it since the code is composing the class props from the properties passed in:

function Archive(config, properties) {
  var hasProp = {}.hasOwnProperty;
  var id = properties.id;
  var key;

  for (key in properties) {
    if (hasProp.call(properties, key)) {
      this[key] = properties[key];
    }
  }
  // ... snip
}

I did add the property to the docs.

@manchuck
Copy link
Contributor Author

manchuck commented Oct 3, 2022

Also holding off on bumping the rev since I have another PR waiting to merge.

Copy link
Contributor

@hamzanasir hamzanasir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as the documentation for this tag is looked out for this PR looks good to me!

lib/archiving.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@jeffswartz jeffswartz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will we also be adding the multiBroadcastTag for start broadcast?

lib/opentok.js Outdated Show resolved Hide resolved
@manchuck
Copy link
Contributor Author

@jeffswartz I also added multiBroadcastTag as well

@manchuck manchuck merged commit c112dab into dev Nov 7, 2022
@manchuck manchuck deleted the archive-tags branch November 7, 2022 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants