Skip to content

Zoom new action #14157

New issue

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

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

Already on GitHub? # to your account

Merged
merged 4 commits into from
Oct 3, 2024
Merged

Zoom new action #14157

merged 4 commits into from
Oct 3, 2024

Conversation

GTFalcao
Copy link
Collaborator

@GTFalcao GTFalcao commented Oct 2, 2024

Closes #13056

I've validated that the request is being sent in accordance with the API docs, with and without optional props, but I cannot successfully execute it due to these requirements:

  • A Pro or higher account plan
  • A Zoom Phone license

Summary by CodeRabbit

Release Notes

  • New Features
    • Introduced a new method for retrieving call recordings from Zoom.
  • Version Updates
    • Incremented version numbers for multiple components, enhancing overall stability and performance.

This release includes a significant addition to the Zoom integration, allowing users to access call recordings, alongside various version updates to ensure compatibility and reliability across components.

Copy link

vercel bot commented Oct 2, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
docs-v2 ⬜️ Ignored (Inspect) Visit Preview Oct 2, 2024 3:04am
pipedream-docs ⬜️ Ignored (Inspect) Oct 2, 2024 3:04am
pipedream-docs-redirect-do-not-edit ⬜️ Ignored (Inspect) Oct 2, 2024 3:04am

Copy link
Contributor

coderabbitai bot commented Oct 2, 2024

Walkthrough

This pull request includes updates to multiple files within the Zoom component, primarily focusing on version increments for various action and source modules. The changes involve updating version numbers across several files, with one new method introduced for retrieving call recordings. No alterations to the logic or functionality of existing methods are present, ensuring that the overall structure remains consistent while enhancing the capabilities of the Zoom integration.

Changes

File Change Summary
components/zoom/actions/add-meeting-registrant/add-meeting-registrant.mjs Version updated from 0.3.2 to 0.3.3
components/zoom/actions/add-webinar-registrant/add-webinar-registrant.mjs Version updated from 0.3.2 to 0.3.3
components/zoom/actions/create-meeting/create-meeting.mjs Version updated from 0.1.3 to 0.1.4
components/zoom/actions/create-user/create-user.mjs Version updated from 0.2.3 to 0.2.4
components/zoom/actions/delete-user/delete-user.mjs Version updated from 0.2.3 to 0.2.4
components/zoom/actions/get-meeting-details/get-meeting-details.mjs Version updated from 0.3.3 to 0.3.4
components/zoom/actions/get-webinar-details/get-webinar-details.mjs Version updated from 0.3.2 to 0.3.3
components/zoom/actions/list-call-recordings/list-call-recordings.mjs New action added for retrieving call recordings
components/zoom/actions/list-channels/list-channels.mjs Version updated from 0.1.3 to 0.1.4
components/zoom/actions/list-past-meeting-participants/list-past-meeting-participants.mjs Version updated from 0.2.2 to 0.2.3
components/zoom/actions/list-past-webinar-qa/list-past-webinar-qa.mjs Version updated from 0.1.3 to 0.1.4
components/zoom/actions/list-user-call-logs/list-user-call-logs.mjs Version updated from 0.0.2 to 0.0.3
components/zoom/actions/list-webinar-participants-report/list-webinar-participants-report.mjs Version updated from 0.0.3 to 0.0.4
components/zoom/actions/send-chat-message/send-chat-message.mjs Version updated from 0.1.3 to 0.1.4
components/zoom/actions/update-meeting/update-meeting.mjs Version updated from 0.1.3 to 0.1.4
components/zoom/actions/update-webinar/update-webinar.mjs Version updated from 0.1.3 to 0.1.4
components/zoom/actions/view-user/view-user.mjs Version updated from 0.1.3 to 0.1.4
components/zoom/package.json Version updated from 0.5.1 to 0.6.0
components/zoom/sources/custom-event/custom-event.mjs Version updated from 0.1.2 to 0.1.3
components/zoom/sources/meeting-created/meeting-created.mjs Version updated from 0.1.2 to 0.1.3
components/zoom/sources/meeting-deleted/meeting-deleted.mjs Version updated from 0.1.2 to 0.1.3
components/zoom/sources/meeting-ended/meeting-ended.mjs Version updated from 0.1.2 to 0.1.3
components/zoom/sources/meeting-started/meeting-started.mjs Version updated from 0.1.3 to 0.1.4
components/zoom/sources/meeting-updated/meeting-updated.mjs Version updated from 0.1.2 to 0.1.3
components/zoom/sources/phone-event/phone-event.mjs Version updated from 0.1.2 to 0.1.3
components/zoom/sources/recording-completed/recording-completed.mjs Version updated from 0.1.2 to 0.1.3
components/zoom/sources/webinar-created/webinar-created.mjs Version updated from 0.1.2 to 0.1.3
components/zoom/sources/webinar-deleted/webinar-deleted.mjs Version updated from 0.1.2 to 0.1.3
components/zoom/sources/webinar-ended/webinar-ended.mjs Version updated from 0.1.2 to 0.1.3
components/zoom/sources/webinar-started/webinar-started.mjs Version updated from 0.1.2 to 0.1.3
components/zoom/sources/webinar-updated/webinar-updated.mjs Version updated from 0.1.2 to 0.1.3
components/zoom/zoom.app.mjs Method listCallRecordings added

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant ZoomAPI
    participant ListCallRecordings

    User->>ListCallRecordings: Request call recordings
    ListCallRecordings->>ZoomAPI: Call /phone/recordings endpoint
    ZoomAPI-->>ListCallRecordings: Return call recordings data
    ListCallRecordings-->>User: Provide call recordings
Loading

Assessment against linked issues

Objective Addressed Explanation
Get a call recording from Zoom (Issue #13056)

Possibly related PRs

Suggested labels

action, trigger / source

Suggested reviewers

  • jcortes

Poem

In the land of Zoom, where meetings abound,
A new way to list calls has been found.
With versions that rise, like the sun in the sky,
We gather our recordings, oh me, oh my!
So hop on, dear friends, let's celebrate this cheer,
For every new change brings us closer, my dear! 🐇✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (8)
components/zoom/sources/meeting-updated/meeting-updated.mjs (3)

Line range hint 28-40: Consider improving the deploy hook implementation.

  1. The current implementation fetches only 25 meetings. Consider adding pagination to retrieve all meetings or allow users to configure the number of meetings to fetch.
  2. Add error handling to the deploy hook to gracefully handle API failures.

Here's a suggested improvement:

hooks: {
  async deploy() {
    try {
      let meetings = [];
      let page = 1;
      const pageSize = 100; // Increased page size
      let hasMore = true;

      while (hasMore) {
        const response = await this.app.listMeetings({
          params: {
            page_size: pageSize,
            page_number: page,
          },
        });

        if (response.meetings && response.meetings.length > 0) {
          meetings = meetings.concat(response.meetings);
          page++;
        } else {
          hasMore = false;
        }
      }

      if (meetings.length === 0) {
        console.log("No meetings found");
        return;
      }

      const objects = this.sortByDate(meetings, "created_at");
      for (const object of objects) {
        this.emitEvent({
          object,
          time_stamp: +new Date(object.start_time),
        }, object);
      }
    } catch (error) {
      console.error("Error in deploy hook:", error);
      // Consider how you want to handle this error (e.g., emit an error event)
    }
  },
},

Line range hint 52-58: Enhance the generateMeta method for more informative metadata.

The current implementation provides basic information. Consider including more details about the updated meeting to make the metadata more useful.

Here's a suggested improvement:

generateMeta(payload, object) {
  return {
    id: `${object.id}-${payload.time_stamp}`,
    summary: `Meeting "${object.topic}" (ID: ${object.id}) updated`,
    ts: +new Date(object.start_time),
    details: {
      host_id: object.host_id,
      type: object.type,
      duration: object.duration,
      timezone: object.timezone,
    },
  };
},

Line range hint 1-62: Summary of review findings

  1. The version update from 0.1.2 to 0.1.3 is appropriate.
  2. Several improvements have been suggested for the existing code:
    • Enhancing the deploy hook with pagination and error handling
    • Improving the generateMeta method for more informative metadata
  3. The current implementation doesn't fully address the PR objective of retrieving call recordings. Consider extending the component to include this functionality.

Overall, while the current changes are minimal and correct, there's an opportunity to significantly improve the component's functionality and align it more closely with the PR objectives.

components/zoom/sources/webinar-ended/webinar-ended.mjs (1)

Line range hint 1-71: Potential missing implementation for new Zoom action.

While the version update is appropriate, I noticed that this file doesn't contain any changes related to the new action for retrieving call recordings from Zoom, as mentioned in the PR objectives. The current file only handles webinar ended events.

To address the PR objectives:

  1. Consider creating a new file for the call recording retrieval action.
  2. Implement the necessary API calls to fetch call recordings as per the Zoom API documentation.
  3. Add appropriate error handling and data processing for the retrieved recordings.

Would you like assistance in creating a template for the new action file or guidance on implementing the Zoom API call for retrieving recordings?

components/zoom/sources/recording-completed/recording-completed.mjs (3)

Line range hint 39-63: Consider pagination in the deploy hook.

The deploy hook retrieves recordings from the last month with a page_size of 25. For users with many recordings, this might not capture all relevant data. Consider implementing pagination to ensure all recordings within the time range are processed.

Here's a suggested implementation using pagination:

async deploy() {
  const startDate = this.monthAgo();
  const endDate = new Date().toISOString().slice(0, 10);
  let nextPageToken = '';
  
  do {
    const { meetings, next_page_token } = await this.app.listRecordings({
      params: {
        from: startDate,
        to: endDate,
        page_size: 25,
        next_page_token: nextPageToken,
      },
    });
    
    if (!meetings || meetings.length === 0) break;
    
    for (const meeting of meetings) {
      if (this.isMeetingRelevant(meeting)) {
        for (const file of meeting.recording_files) {
          if (this.isFileRelevant(file)) {
            this.emitEvent(file, meeting);
          }
        }
      }
    }
    
    nextPageToken = next_page_token;
  } while (nextPageToken);
}

Line range hint 66-84: Simplify isMeetingRelevant and isFileRelevant methods.

The isMeetingRelevant and isFileRelevant methods could be simplified for better readability. Consider using early returns and combining conditions.

Here are suggested implementations:

isMeetingRelevant(meeting) {
  if (!meeting.recording_files?.length) {
    console.log("No files in recording. Exiting");
    return false;
  }

  if (this.meetingIds?.length && !this.meetingIds.includes(meeting.id)) {
    console.log("Meeting ID does not match the filter rules.");
    return false;
  }

  return true;
}

isFileRelevant(file) {
  if (file.status !== "completed") return false;
  if (file.file_type === "M4A" && !this.includeAudioRecordings) return false;
  if (file.file_type === "CHAT" && !this.includeChatTranscripts) return false;
  return true;
}

Line range hint 85-105: Consistent use of spread operator in emitEvent.

The emitEvent method mixes the spread operator with individual property assignments. Consider using the spread operator consistently for better readability and maintainability.

Here's a suggested implementation:

emitEvent(file, meeting, downloadToken = null) {
  this.$emit(
    {
      ...file,
      download_url_with_token: `${file.download_url}?access_token=${downloadToken}`,
      download_token: downloadToken,
      meeting_id_long: meeting.id,
      meeting_topic: meeting.topic,
      host_id: meeting.host_id,
      host_email: meeting.host_email,
    },
    {
      id: file.id,
      summary: `${meeting.topic}${file.file_type}`,
      ts: +new Date(file.recording_end),
    },
  );
}
components/zoom/zoom.app.mjs (1)

293-298: LGTM! Consider adding a brief JSDoc comment.

The implementation of listCallRecordings is correct and aligns well with the existing code structure and the PR objectives. It properly utilizes the _makeRequest method to interact with the Zoom API endpoint for retrieving call recordings.

To improve code documentation, consider adding a brief JSDoc comment above the method. For example:

/**
 * Retrieves a list of call recordings from Zoom.
 * @param {Object} args - Optional arguments to pass to the request.
 * @returns {Promise<Object>} The API response containing call recordings.
 */
listCallRecordings(args = {}) {
  // ... (existing implementation)
}

This addition would enhance code readability and provide quick insights into the method's purpose and parameters.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 1261174 and 2f932b1.

📒 Files selected for processing (32)
  • components/zoom/actions/add-meeting-registrant/add-meeting-registrant.mjs (1 hunks)
  • components/zoom/actions/add-webinar-registrant/add-webinar-registrant.mjs (1 hunks)
  • components/zoom/actions/create-meeting/create-meeting.mjs (1 hunks)
  • components/zoom/actions/create-user/create-user.mjs (1 hunks)
  • components/zoom/actions/delete-user/delete-user.mjs (1 hunks)
  • components/zoom/actions/get-meeting-details/get-meeting-details.mjs (1 hunks)
  • components/zoom/actions/get-webinar-details/get-webinar-details.mjs (1 hunks)
  • components/zoom/actions/list-call-recordings/list-call-recordings.mjs (1 hunks)
  • components/zoom/actions/list-channels/list-channels.mjs (1 hunks)
  • components/zoom/actions/list-past-meeting-participants/list-past-meeting-participants.mjs (1 hunks)
  • components/zoom/actions/list-past-webinar-qa/list-past-webinar-qa.mjs (1 hunks)
  • components/zoom/actions/list-user-call-logs/list-user-call-logs.mjs (1 hunks)
  • components/zoom/actions/list-webinar-participants-report/list-webinar-participants-report.mjs (1 hunks)
  • components/zoom/actions/send-chat-message/send-chat-message.mjs (1 hunks)
  • components/zoom/actions/update-meeting/update-meeting.mjs (1 hunks)
  • components/zoom/actions/update-webinar/update-webinar.mjs (1 hunks)
  • components/zoom/actions/view-user/view-user.mjs (1 hunks)
  • components/zoom/package.json (1 hunks)
  • components/zoom/sources/custom-event/custom-event.mjs (1 hunks)
  • components/zoom/sources/meeting-created/meeting-created.mjs (1 hunks)
  • components/zoom/sources/meeting-deleted/meeting-deleted.mjs (1 hunks)
  • components/zoom/sources/meeting-ended/meeting-ended.mjs (1 hunks)
  • components/zoom/sources/meeting-started/meeting-started.mjs (1 hunks)
  • components/zoom/sources/meeting-updated/meeting-updated.mjs (1 hunks)
  • components/zoom/sources/phone-event/phone-event.mjs (1 hunks)
  • components/zoom/sources/recording-completed/recording-completed.mjs (1 hunks)
  • components/zoom/sources/webinar-created/webinar-created.mjs (1 hunks)
  • components/zoom/sources/webinar-deleted/webinar-deleted.mjs (1 hunks)
  • components/zoom/sources/webinar-ended/webinar-ended.mjs (1 hunks)
  • components/zoom/sources/webinar-started/webinar-started.mjs (1 hunks)
  • components/zoom/sources/webinar-updated/webinar-updated.mjs (1 hunks)
  • components/zoom/zoom.app.mjs (1 hunks)
✅ Files skipped from review due to trivial changes (25)
  • components/zoom/actions/add-meeting-registrant/add-meeting-registrant.mjs
  • components/zoom/actions/add-webinar-registrant/add-webinar-registrant.mjs
  • components/zoom/actions/create-meeting/create-meeting.mjs
  • components/zoom/actions/create-user/create-user.mjs
  • components/zoom/actions/delete-user/delete-user.mjs
  • components/zoom/actions/get-meeting-details/get-meeting-details.mjs
  • components/zoom/actions/get-webinar-details/get-webinar-details.mjs
  • components/zoom/actions/list-channels/list-channels.mjs
  • components/zoom/actions/list-past-meeting-participants/list-past-meeting-participants.mjs
  • components/zoom/actions/list-past-webinar-qa/list-past-webinar-qa.mjs
  • components/zoom/actions/list-user-call-logs/list-user-call-logs.mjs
  • components/zoom/actions/list-webinar-participants-report/list-webinar-participants-report.mjs
  • components/zoom/actions/send-chat-message/send-chat-message.mjs
  • components/zoom/actions/update-meeting/update-meeting.mjs
  • components/zoom/actions/update-webinar/update-webinar.mjs
  • components/zoom/actions/view-user/view-user.mjs
  • components/zoom/package.json
  • components/zoom/sources/custom-event/custom-event.mjs
  • components/zoom/sources/meeting-deleted/meeting-deleted.mjs
  • components/zoom/sources/meeting-ended/meeting-ended.mjs
  • components/zoom/sources/meeting-started/meeting-started.mjs
  • components/zoom/sources/phone-event/phone-event.mjs
  • components/zoom/sources/webinar-created/webinar-created.mjs
  • components/zoom/sources/webinar-deleted/webinar-deleted.mjs
  • components/zoom/sources/webinar-started/webinar-started.mjs
🧰 Additional context used
🪛 Biome
components/zoom/actions/list-call-recordings/list-call-recordings.mjs

[error] 45-45: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)

🔇 Additional comments (7)
components/zoom/sources/meeting-updated/meeting-updated.mjs (2)

9-9: Version increment looks good.

The version has been updated from "0.1.2" to "0.1.3", which is appropriate for minor changes or bug fixes. This change aligns with semantic versioning practices.


Line range hint 1-62: Consider addressing the PR objective of retrieving call recordings.

The current implementation focuses on meeting updates, but the PR objective mentions retrieving call recordings. Consider adding functionality to fetch and emit events for call recordings as well.

To address this:

  1. Add a new method to fetch call recordings using the Zoom API.
  2. Implement a new event type for call recordings.
  3. Update the deploy hook to fetch both meetings and call recordings.
  4. Ensure that the necessary permissions are in place to access call recordings.

Here's a high-level suggestion for fetching call recordings:

async fetchCallRecordings() {
  // Implement the API call to Zoom's Get Call Recordings endpoint
  // https://marketplace.zoom.us/docs/api-reference/zoom-api/phone-recordings/listcallrecordings
  // Emit events for each call recording
}

Remember to update the component's description and add any necessary props for call recording functionality.

To verify the current API capabilities, let's check the Zoom API client implementation:

components/zoom/sources/webinar-updated/webinar-updated.mjs (2)

9-9: Version update looks good.

The version number has been incremented from 0.1.2 to 0.1.3, which is appropriate for minor changes or bug fixes.


Line range hint 1-67: Discrepancy between PR objectives and implemented changes.

The PR objectives mention implementing a new action for retrieving call recordings from Zoom (issue #13056). However, this file is related to webinar updates and doesn't contain any changes regarding call recordings. Please clarify if there are additional files that implement the call recording functionality or if the PR description needs to be updated.

To check for any related changes in other files, you can run the following script:

✅ Verification successful

Discrepancy Clarified: PR Implements Zoom Call Recordings

The PR includes the implementation of Zoom call recordings as intended:

  • Added File: components/zoom/actions/list-call-recordings/list-call-recordings.mjs

These changes confirm that the functionality for retrieving call recordings from Zoom is present within the Zoom component.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for files related to Zoom call recordings

# Test: Search for files containing "zoom" and "call recording" or "phone recording"
echo "Files potentially related to Zoom call recordings:"
rg -i -l 'zoom.*(?:call|phone)\s+recording' --type-add 'code:*.{js,mjs,ts,jsx,tsx}' -t code

# Test: Check for new or modified files in the zoom component directory
echo "New or modified files in the zoom component directory:"
git diff --name-status origin/main..HEAD -- components/zoom/

Length of output: 426


Script:

#!/bin/bash
# Description: Ensure origin/main is up-to-date and check for new or modified files in the zoom component directory

# Fetch the latest changes from the remote repository
echo "Fetching latest changes from origin..."
git fetch origin

# Verify that origin/main exists
if git show-ref --verify --quiet refs/remotes/origin/main; then
  echo "origin/main exists. Proceeding with git diff."
else
  echo "origin/main does not exist. Please check the remote repository and branch names."
  exit 1
fi

# Check for new or modified files in the zoom component directory
echo "New or modified files in the zoom component directory after fetching origin/main:"
git diff --name-status origin/main..HEAD -- components/zoom/

Length of output: 342


Script:

#!/bin/bash
# Description: Identify the default branch name and check for new or modified files in the zoom component directory

# List all remote branches to identify the default branch
echo "Listing all remote branches:"
git branch -r

# Set the default branch variable (update if default branch is different)
DEFAULT_BRANCH="origin/main"

# Check if origin/main exists; if not, try origin/master
if git show-ref --verify --quiet refs/remotes/origin/main; then
  DEFAULT_BRANCH="origin/main"
elif git show-ref --verify --quiet refs/remotes/origin/master; then
  DEFAULT_BRANCH="origin/master"
else
  echo "Neither origin/main nor origin/master exists. Please verify the default branch name."
  exit 1
fi

echo "Using default branch: $DEFAULT_BRANCH"

# Check for new or modified files in the zoom component directory against the identified default branch
echo "New or modified files in the zoom component directory compared to $DEFAULT_BRANCH:"
git diff --name-status "$DEFAULT_BRANCH..HEAD" -- components/zoom/

Length of output: 2632

components/zoom/sources/webinar-ended/webinar-ended.mjs (1)

9-9: Version update approved.

The version number has been incremented from "0.1.2" to "0.1.3". This change is appropriate for tracking minor updates to the component.

However, I'd like to verify if this change is related to the PR objectives or if additional changes are needed. Let's check the PR description and linked issues:

These checks will help us determine if the PR includes the necessary changes for the new Zoom action to retrieve call recordings, as mentioned in the PR objectives.

components/zoom/sources/recording-completed/recording-completed.mjs (1)

9-9: Verify the necessity of the version update.

The version has been incremented from "0.1.2" to "0.1.3", but there are no other visible changes in this file. Please confirm if there are any functional changes that justify this version update. If not, consider reverting the version change to maintain accurate versioning.

components/zoom/zoom.app.mjs (1)

293-298: Additional considerations for the new listCallRecordings method

The implementation looks good and aligns with the PR objectives. However, there are a few points to address:

  1. The PR description mentions that you couldn't execute the request due to specific account requirements. Could you provide more information on this? It might be helpful to add a comment in the code or update the documentation to mention these requirements.

  2. The Zoom API documentation states that a Pro or higher account plan and a Zoom Phone license are needed. Should we add any checks or error handling for these requirements?

  3. The linked issue mentions logging call recordings into HubSpot. Is there any additional integration or method needed to facilitate this, or will it be handled elsewhere in the application?

Could you provide more context on how this method will be used and if any additional error handling or documentation is needed?

@@ -6,7 +6,7 @@ export default {
key: "zoom-meeting-created",
name: "Meeting Created (Instant)",
description: "Emit new event each time a meeting is created where you're the host",
version: "0.1.2",
version: "0.1.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Version increment needs justification.

The version has been incremented from "0.1.2" to "0.1.3", which suggests a patch-level change according to semantic versioning. However, there are no visible functional changes in this file that would justify a version increment.

Please provide justification for this version change. If there are no functional changes, consider reverting the version update or including the intended changes that warrant this increment.

let to = endDate;
if (startDate && !endDate) {
const date = new Date(startDate);
if (isNaN(date.valueOf())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Use Number.isNaN() Instead of isNaN()

The isNaN() function is unsafe as it attempts type coercion. It's recommended to use Number.isNaN() which doesn't coerce the value, ensuring accurate validation.

Apply this diff to fix the issue:

-        if (isNaN(date.valueOf())) {
+        if (Number.isNaN(date.valueOf())) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (isNaN(date.valueOf())) {
if (Number.isNaN(date.valueOf())) {
🧰 Tools
🪛 Biome

[error] 45-45: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)

Comment on lines +43 to +53
if (startDate && !endDate) {
const date = new Date(startDate);
if (isNaN(date.valueOf())) {
throw new Error("Invalid format for `Start Date`. Please use `yyyy-mm-dd` or `yyyy-MM-ddTHH:mm:ssZ`.");
}
date.setDate(date.getDate() - 30);
to = date.toISOString();
if (!startDate.split("T")[1]) {
to = to.split("T")[0];
}
}
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 2, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix Logic for Calculating the to Date

There's a logical error when calculating the to date. Subtracting 30 days from startDate results in a to date earlier than startDate, which may lead to incorrect or empty results since from would be after to.

According to your description, if End Date is not specified, calls made within a 30-day period starting on Start Date should be retrieved. Therefore, you should add 30 days to startDate to set the to date.

Apply this diff to correct the logic:

        const date = new Date(startDate);
        if (Number.isNaN(date.valueOf())) {
          throw new Error("Invalid format for `Start Date`. Please use `yyyy-mm-dd` or `yyyy-MM-ddTHH:mm:ssZ`.");
        }
-        date.setDate(date.getDate() - 30);
+        date.setDate(date.getDate() + 30);
         to = date.toISOString();
         if (!startDate.split("T")[1]) {
           to = to.split("T")[0];
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (startDate && !endDate) {
const date = new Date(startDate);
if (isNaN(date.valueOf())) {
throw new Error("Invalid format for `Start Date`. Please use `yyyy-mm-dd` or `yyyy-MM-ddTHH:mm:ssZ`.");
}
date.setDate(date.getDate() - 30);
to = date.toISOString();
if (!startDate.split("T")[1]) {
to = to.split("T")[0];
}
}
if (startDate && !endDate) {
const date = new Date(startDate);
if (Number.isNaN(date.valueOf())) {
throw new Error("Invalid format for `Start Date`. Please use `yyyy-mm-dd` or `yyyy-MM-ddTHH:mm:ssZ`.");
}
date.setDate(date.getDate() + 30);
to = date.toISOString();
if (!startDate.split("T")[1]) {
to = to.split("T")[0];
}
}
🧰 Tools
🪛 Biome

[error] 45-45: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wondering if what coderabbitai says here is relevant, otherwise omit this comment!

Copy link
Contributor

Choose a reason for hiding this comment

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

@jcortes, the comment by coderabbitai is indeed relevant. There's a logical error in the current code where you're subtracting 30 days from startDate to compute to when endDate is not specified. This results in a to date that is earlier than startDate, which could lead to incorrect or empty results.

According to your description, if End Date is not specified, calls made within a 30-day period starting on Start Date should be retrieved. Therefore, you should add 30 days to startDate to set the to date.

Here's the corrected code:

if (startDate && !endDate) {
  const date = new Date(startDate);
  if (Number.isNaN(date.valueOf())) {
    throw new Error("Invalid format for `Start Date`. Please use `yyyy-mm-dd` or `yyyy-MM-ddTHH:mm:ssZ`.");
  }
  date.setDate(date.getDate() + 30);
  to = date.toISOString();
  if (!startDate.includes("T")) {
    to = to.split("T")[0];
  }
}

Additionally, regarding the Biome warning, it's recommended to use Number.isNaN instead of isNaN to avoid type coercion issues. Updating your code as shown above addresses this concern.

Let me know if you have any questions or need further assistance.

Comment on lines +42 to +60
let to = endDate;
if (startDate && !endDate) {
const date = new Date(startDate);
if (isNaN(date.valueOf())) {
throw new Error("Invalid format for `Start Date`. Please use `yyyy-mm-dd` or `yyyy-MM-ddTHH:mm:ssZ`.");
}
date.setDate(date.getDate() - 30);
to = date.toISOString();
if (!startDate.split("T")[1]) {
to = to.split("T")[0];
}
}
const { recordings } = await zoom.listCallRecordings({
step: $,
params: {
page_size: max,
from: startDate,
to,
},
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 2, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Validate Date Range Does Not Exceed 30 Days

The Zoom API requires the date range between from and to to not exceed 30 days. Currently, there's no validation to enforce this limit when both startDate and endDate are provided.

Consider adding a validation check to ensure the date range does not exceed 30 days and throw an error if it does.

Apply this diff to add date range validation:

      const { zoom, startDate, endDate, max } = this;
      let to = endDate;
      if (startDate && !endDate) {
        const date = new Date(startDate);
        if (Number.isNaN(date.valueOf())) {
          throw new Error("Invalid format for `Start Date`. Please use `yyyy-mm-dd` or `yyyy-MM-ddTHH:mm:ssZ`.");
        }
        date.setDate(date.getDate() + 30);
        to = date.toISOString();
        if (!startDate.split("T")[1]) {
          to = to.split("T")[0];
        }
      }
+     if (startDate && to) {
+       const start = new Date(startDate);
+       const end = new Date(to);
+       const diffTime = Math.abs(end - start);
+       const diffDays = Math.ceil(diffTime / (1000 * 60 * 60 * 24));
+       if (diffDays > 30) {
+         throw new Error("The date range between `Start Date` and `End Date` cannot exceed 30 days.");
+       }
+     }
      const { recordings } = await zoom.listCallRecordings({
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let to = endDate;
if (startDate && !endDate) {
const date = new Date(startDate);
if (isNaN(date.valueOf())) {
throw new Error("Invalid format for `Start Date`. Please use `yyyy-mm-dd` or `yyyy-MM-ddTHH:mm:ssZ`.");
}
date.setDate(date.getDate() - 30);
to = date.toISOString();
if (!startDate.split("T")[1]) {
to = to.split("T")[0];
}
}
const { recordings } = await zoom.listCallRecordings({
step: $,
params: {
page_size: max,
from: startDate,
to,
},
let to = endDate;
if (startDate && !endDate) {
const date = new Date(startDate);
if (Number.isNaN(date.valueOf())) {
throw new Error("Invalid format for `Start Date`. Please use `yyyy-mm-dd` or `yyyy-MM-ddTHH:mm:ssZ`.");
}
date.setDate(date.getDate() + 30);
to = date.toISOString();
if (!startDate.split("T")[1]) {
to = to.split("T")[0];
}
}
if (startDate && to) {
const start = new Date(startDate);
const end = new Date(to);
const diffTime = Math.abs(end - start);
const diffDays = Math.ceil(diffTime / (1000 * 60 * 60 * 24));
if (diffDays > 30) {
throw new Error("The date range between `Start Date` and `End Date` cannot exceed 30 days.");
}
}
const { recordings } = await zoom.listCallRecordings({
step: $,
params: {
page_size: max,
from: startDate,
to,
},
🧰 Tools
🪛 Biome

[error] 45-45: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jcortes I'm leaving this validation up to their API, since they do not mention it as a strictly enforced limit (i.e. that will throw an error) but rather state that the response is limited to up to 30 days of data.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Collaborator

@jcortes jcortes left a comment

Choose a reason for hiding this comment

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

Hi @GTFalcao I've just left one minor comment, other than that lgtm. Ready for QA!

Comment on lines +43 to +53
if (startDate && !endDate) {
const date = new Date(startDate);
if (isNaN(date.valueOf())) {
throw new Error("Invalid format for `Start Date`. Please use `yyyy-mm-dd` or `yyyy-MM-ddTHH:mm:ssZ`.");
}
date.setDate(date.getDate() - 30);
to = date.toISOString();
if (!startDate.split("T")[1]) {
to = to.split("T")[0];
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wondering if what coderabbitai says here is relevant, otherwise omit this comment!

@GTFalcao GTFalcao merged commit ce48071 into master Oct 3, 2024
12 checks passed
@GTFalcao GTFalcao deleted the 13056-zoom-new-action branch October 3, 2024 03:59
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ACTION] Zoom - Get Call Recordings
2 participants