feat: added trigger methods on toolset #1225

Devanshusisodiya · 2025-01-25T14:28:33Z

Added some trigger methods on toolset to view config, list active triggers and delete triggers.

Important

Add methods to ComposioToolSet for managing triggers, including viewing configurations, listing active triggers, and deleting triggers, with corresponding tests.

Behavior:
- Add get_trigger_config_scheme() to ComposioToolSet for retrieving trigger configuration.
- Add get_active_triggers() to ComposioToolSet for listing active triggers.
- Add delete_trigger() to ComposioToolSet for deleting triggers.
Collections:
- Add delete() method to Triggers class in collections.py for deleting a trigger.
Tests:
- Add test_get_trigger_config_scheme() in test_toolset.py to test trigger config retrieval.
- Add test_delete_trigger() in test_toolset.py to test trigger deletion.

^{This description was created by}^{for b204879. It will automatically update as commits are pushed.}

vercel · 2025-01-25T14:28:38Z

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

Name	Status	Preview	Comments	Updated (UTC)
composio	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 28, 2025 1:25pm

entelligence-ai-pr-reviews · 2025-01-25T14:29:09Z

LGTM 👍

ellipsis-dev

👍 Looks good to me! Reviewed everything up to 1e45b3d in 59 seconds

More details

Looked at 120 lines of code in 3 files
Skipped 0 files when reviewing.
Skipped posting 1 drafted comments based on config settings.

1. python/composio/client/collections.py:937

Draft comment:
The endpoint for deleting a trigger seems incorrect. It should be url=str(self.endpoint / "instance" / id) instead of including "delete" in the path.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests changing the URL structure, but I don't have strong evidence that the suggested change is correct. While other endpoints like disable() use a different URL pattern, that's not conclusive proof that delete() should match. The actual API endpoint structure would be determined by the backend, and without access to backend API docs or specs, I can't be certain which URL is correct.
I might be missing some implicit API design pattern that would make this obvious to someone more familiar with the codebase. The fact that other endpoints use a different pattern could be meaningful.
While there may be patterns in the API design, without clear documentation or specs, I can't be confident enough that the comment is correct. The current URL could be intentionally different for delete operations.
Delete the comment since we don't have strong evidence that the suggested URL change is correct. Making incorrect suggestions about API endpoints could cause confusion.

Workflow ID: wflow_1sQeMbtw6JyPoCig

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

shreysingla11 · 2025-01-25T14:31:09Z

python/tests/test_tools/test_toolset.py

+def test_delete_trigger() -> None:
+    """Test `ComposioToolSet.delete_trigger` method."""
+    toolset = ComposioToolSet(
+        api_key="s7cccojimwxj9od0344an" # [email protected] account key


Security concern: Hardcoded API key and account ID in test file. Consider using environment variables or test fixtures instead:

toolset = ComposioToolSet( api_key=os.getenv("COMPOSIO_TEST_API_KEY") ) enabled_trigger = toolset.client.triggers.enable( name="GMAIL_NEW_GMAIL_MESSAGE", connected_account_id=os.getenv("COMPOSIO_TEST_ACCOUNT_ID"), ... )

shreysingla11 · 2025-01-25T14:31:26Z

python/composio/tools/toolset.py

+            trigger_names=trigger_names
+        )
+
+    def delete_trigger(self, id: str):


Missing docstring and return type hint. Consider adding:

def delete_trigger(self, id: str) -> t.Dict[str, str]: """Delete a trigger by its ID. Args: id (str): The ID of the trigger to delete Returns: Dict[str, str]: Response containing the status of deletion Raises: ComposioClientError: If trigger deletion fails """ return self.client.triggers.delete(id=id)

shreysingla11 · 2025-01-25T14:31:44Z

python/composio/tools/toolset.py

@@ -1495,6 +1497,25 @@ def get_action(self, action: ActionType) -> ActionModel:

    def get_trigger(self, trigger: TriggerType) -> TriggerModel:
        return self.client.triggers.get(trigger_names=[trigger]).pop()
+
+    def get_trigger_config_scheme(self, trigger: TriggerType) -> TriggerConfigModel:


Missing docstring and type hints could be improved. Consider:

def get_trigger_config_scheme(self, trigger: TriggerType) -> TriggerConfigModel: """Get the configuration schema for a specific trigger. Args: trigger (TriggerType): The type of trigger to get config scheme for Returns: TriggerConfigModel: The configuration schema model Raises: ComposioClientError: If trigger type is invalid or not found """ return self.client.triggers.get(trigger_names=[trigger]).pop().config

shreysingla11 · 2025-01-25T14:32:02Z

python/composio/tools/toolset.py

+    def get_trigger_config_scheme(self, trigger: TriggerType) -> TriggerConfigModel:
+        return self.client.triggers.get(trigger_names=[trigger]).pop().config
+
+    def get_active_triggers(self, 


Consider improving type hints and adding docstring:

def get_active_triggers( self, trigger_ids: t.Optional[t.List[str]] = None, connected_account_ids: t.Optional[t.List[str]] = None, integration_ids: t.Optional[t.List[str]] = None, trigger_names: t.Optional[t.List[t.Union[str, Trigger]]] = None ) -> t.List[ActiveTriggerModel]: """Retrieve active triggers based on various filter criteria. Args: trigger_ids: Optional list of trigger IDs to filter by connected_account_ids: Optional list of connected account IDs integration_ids: Optional list of integration IDs trigger_names: Optional list of trigger names or Trigger enum values Returns: List[ActiveTriggerModel]: List of active triggers matching the criteria Raises: ComposioClientError: If retrieval fails """ return self.client.active_triggers.get( trigger_ids=trigger_ids, connected_account_ids=connected_account_ids, integration_ids=integration_ids, trigger_names=trigger_names )

shreysingla11 · 2025-01-25T14:32:21Z

Code Review Summary

Overall, the changes look good but there are some areas that need improvement:

Strengths:

Clean implementation of new trigger management methods
Good test coverage
Proper model inheritance structure

Areas for Improvement:

Security:
- Remove hardcoded credentials from test file
- Use environment variables for sensitive data
Documentation:
- Add comprehensive docstrings to new methods
- Include error handling documentation
- Document return types and possible exceptions
Type Hints:
- Use Optional[T] for nullable parameters
- Improve type hints consistency across methods
- Add proper return type hints
Testing:
- Consider adding error case tests
- Use test fixtures for credentials

The code is functionally sound but would benefit from these improvements for better maintainability and security.

Rating: 7/10 - Good functionality but needs documentation and security improvements.

ellipsis-dev

👍 Looks good to me! Incremental review on b304fcf in 28 seconds

More details

Looked at 114 lines of code in 3 files
Skipped 0 files when reviewing.
Skipped posting 1 drafted comments based on config settings.

1. python/tests/test_tools/test_toolset.py:52

Draft comment:
Avoid using hardcoded API keys in tests for security reasons. Consider using a mock or a fixture instead.
Reason this comment was not posted:
Comment was on unchanged code.

Workflow ID: wflow_iN1nLZ2MuE2AwkGY

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev

👍 Looks good to me! Incremental review on 95f9ed0 in 12 seconds

More details

Looked at 19 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 1 drafted comments based on config settings.

1. python/composio/tools/toolset.py:905

Draft comment:
Use t.Optional for type hinting optional parameters for consistency and readability. This change is correctly applied in the get_active_triggers method.
Reason this comment was not posted:
Confidence changes required: 0%
The PR changes the type hinting for optional parameters in the get_active_triggers method. This is a good practice as it makes the code more readable and consistent with the rest of the codebase.

Workflow ID: wflow_iZQFMqp7NQpXhShw

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev

👍 Looks good to me! Incremental review on e069ccf in 12 seconds

More details

Looked at 13 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 1 drafted comments based on config settings.

1. python/composio/tools/toolset.py:908

Draft comment:
The change from Trigger to TriggerType in the type hint is correct and aligns with the imported TriggerType. Ensure that all usages of trigger_names are compatible with this type change.
Reason this comment was not posted:
Confidence changes required: 0%
The change from Trigger to TriggerType in the type hint is correct based on the imports and usage context.

Workflow ID: wflow_XcnwmIQCcJKfxTUC

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev

👍 Looks good to me! Incremental review on 55d6730 in 11 seconds

More details

Looked at 22 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 1 drafted comments based on config settings.

1. python/composio/tools/toolset.py:47

Draft comment:
The removal of the Trigger import is correct as it is not used in the file.
Reason this comment was not posted:
Confidence changes required: 0%
The import statement for Trigger is removed, but it is not used anywhere in the file. This change is correct and does not affect the functionality.

Workflow ID: wflow_fM5LnzeqvbGDQp1f

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev

👍 Looks good to me! Incremental review on 56751b7 in 33 seconds

More details

Looked at 47 lines of code in 2 files
Skipped 0 files when reviewing.
Skipped posting 1 drafted comments based on config settings.

1. python/tests/test_tools/test_toolset.py:52

Draft comment:
Consider adding a check to ensure COMPOSIO_API_KEY is set before using it to initialize ComposioToolSet. This will prevent potential NoneType errors if the environment variable is not set.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:

The comment is about a real potential issue - null API keys could cause problems. 2. However, looking at the full file, this is already well-handled by the codebase. 3. There is a specific test case test_api_key_missing that verifies the error handling. 4. The code already has proper error handling via ApiKeyNotProvidedError. 5. The comment is suggesting something that is already implemented.
I could be wrong if the test case doesn't actually cover all code paths, or if there are edge cases not handled by the existing error handling.
The test case explicitly tests the error handling by setting an empty API key and verifying the error message. The error handling appears comprehensive.
Delete the comment because the suggested check is already implemented and tested via test_api_key_missing.

Workflow ID: wflow_Fjb4V7mfUEFt5req

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

angrybayblade · 2025-01-27T09:56:17Z

python/composio/tools/toolset.py

@@ -895,6 +897,26 @@ def get_action(self, action: ActionType) -> ActionModel:
    def get_trigger(self, trigger: TriggerType) -> TriggerModel:
        return self.client.triggers.get(trigger_names=[trigger]).pop()

+    def get_trigger_config_scheme(self, trigger: TriggerType) -> TriggerConfigModel:
+        return self.client.triggers.get(trigger_names=[trigger]).pop().config


Suggested change

return self.client.triggers.get(trigger_names=[trigger]).pop().config

return self.get_trigger(trigger=trigger).config

ellipsis-dev

👍 Looks good to me! Incremental review on b204879 in 16 seconds

More details

Looked at 42 lines of code in 2 files
Skipped 0 files when reviewing.
Skipped posting 2 drafted comments based on config settings.

1. python/composio/tools/toolset.py:901

Draft comment:
Refactored get_trigger_config_scheme to use get_trigger method for better code reuse. No issues found.
Reason this comment was not posted:
Confidence changes required: 0%
The method get_trigger_config_scheme was updated to use get_trigger method, which is a good change for code reuse. No issues here.

2. python/composio/tools/toolset.py:918

Draft comment:
Updated delete_trigger method to include return type t.Dict. This is a good practice for type hinting. No issues found.
Reason this comment was not posted:
Confidence changes required: 0%
The return type of delete_trigger method was updated to t.Dict, which is a good practice for type hinting. No issues here.

Workflow ID: wflow_yat4WWlCbyjvzt9n

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

feat: added trigger methods on toolset

1e45b3d

Devanshusisodiya requested a review from angrybayblade as a code owner January 25, 2025 14:28

Devanshusisodiya changed the title ~~feat: added trigger methods on toolset~~ feat/added trigger methods on toolset Jan 25, 2025

ellipsis-dev bot reviewed Jan 25, 2025

View reviewed changes

shreysingla11 reviewed Jan 25, 2025

View reviewed changes

fmt chk

b304fcf

vercel bot deployed to Preview January 25, 2025 14:33 View deployment

ellipsis-dev bot reviewed Jan 25, 2025

View reviewed changes

Devanshusisodiya changed the title ~~feat/added trigger methods on toolset~~ feat: added trigger methods on toolset Jan 26, 2025

Merge branch 'master' into feat/trigger_features

0a95bec

vercel bot deployed to Preview January 26, 2025 16:42 View deployment

fix: tests

95f9ed0

vercel bot deployed to Preview January 26, 2025 17:49 View deployment

ellipsis-dev bot reviewed Jan 26, 2025

View reviewed changes

fix: tests

e069ccf

vercel bot deployed to Preview January 26, 2025 18:16 View deployment

ellipsis-dev bot reviewed Jan 26, 2025

View reviewed changes

fix: tests

55d6730

ellipsis-dev bot reviewed Jan 26, 2025

View reviewed changes

vercel bot deployed to Preview January 26, 2025 18:23 View deployment

test: fix tests

56751b7

vercel bot deployed to Preview January 26, 2025 19:10 View deployment

ellipsis-dev bot reviewed Jan 26, 2025

View reviewed changes

angrybayblade approved these changes Jan 27, 2025

View reviewed changes

fix: review fixes

b204879

vercel bot deployed to Preview January 27, 2025 10:27 View deployment

ellipsis-dev bot reviewed Jan 27, 2025

View reviewed changes

Devanshusisodiya enabled auto-merge (squash) January 27, 2025 10:35

Devanshusisodiya disabled auto-merge January 27, 2025 10:48

Merge branch 'master' into feat/trigger_features

e6ade0c

vercel bot deployed to Preview January 28, 2025 13:25 View deployment

Devanshusisodiya enabled auto-merge (squash) January 28, 2025 13:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added trigger methods on toolset #1225

feat: added trigger methods on toolset #1225

Devanshusisodiya commented Jan 25, 2025 •

edited by ellipsis-dev bot

Loading

vercel bot commented Jan 25, 2025 •

edited

Loading

entelligence-ai-pr-reviews bot commented Jan 25, 2025

ellipsis-dev bot left a comment

shreysingla11 Jan 25, 2025

shreysingla11 Jan 25, 2025

shreysingla11 Jan 25, 2025

shreysingla11 Jan 25, 2025

shreysingla11 commented Jan 25, 2025

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

ellipsis-dev bot left a comment

angrybayblade Jan 27, 2025

ellipsis-dev bot left a comment

	return self.client.triggers.get(trigger_names=[trigger]).pop().config
	return self.get_trigger(trigger=trigger).config

feat: added trigger methods on toolset #1225

Are you sure you want to change the base?

feat: added trigger methods on toolset #1225

Conversation

Devanshusisodiya commented Jan 25, 2025 • edited by ellipsis-dev bot Loading

vercel bot commented Jan 25, 2025 • edited Loading

entelligence-ai-pr-reviews bot commented Jan 25, 2025

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

shreysingla11 Jan 25, 2025

Choose a reason for hiding this comment

shreysingla11 Jan 25, 2025

Choose a reason for hiding this comment

shreysingla11 Jan 25, 2025

Choose a reason for hiding this comment

shreysingla11 Jan 25, 2025

Choose a reason for hiding this comment

shreysingla11 commented Jan 25, 2025

Code Review Summary

Strengths:

Areas for Improvement:

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

angrybayblade Jan 27, 2025

Choose a reason for hiding this comment

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Devanshusisodiya commented Jan 25, 2025 •

edited by ellipsis-dev bot

Loading

vercel bot commented Jan 25, 2025 •

edited

Loading