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

feat: Execute macro actions, for the conversation #5066

Merged
merged 6 commits into from
Jul 26, 2022

Conversation

tejaswinichile
Copy link
Contributor

@tejaswinichile tejaswinichile commented Jul 19, 2022

Pull Request Template

Description

Execute macros actions for the conversation.

Fixes #5013

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Tested on local and with test cases.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented on my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
Screen.Recording.2022-07-20.at.1.17.00.PM.mov

@pr-triage pr-triage bot added the PR: unreviewed This pull request is yet to be reviewed. label Jul 19, 2022
@tejaswinichile tejaswinichile force-pushed the feat/5013-execute-macro branch 5 times, most recently from b019893 to 4f9e285 Compare July 20, 2022 06:50
@sojan-official
Copy link
Member

@tejaswinichile, unlike automation performed by the system, for macros, the actions should come under the agent who performed the action. Think of it as a sequence of actions that you perform as an agent everyday, so that you want that to be done in one click.

Also, for the macros endpoint, we will need to accept an array of conversation ids if we are thinking about having it in bulk actions.

cc: @pranavrajs

@pranavrajs
Copy link
Member

@sojan-official Yes, this is correct. The user context should be used. Unlike automation, the action is performed by the agent, so every action should consider the user context (e.g., A message should be sent in the name of the agent, or labels should have the agent name, etc.). Also, we need support for multiple conversation ids in the API.

@tejaswinichile Noticed that the add_label action didn't work in the screencast.

@tejaswinichile
Copy link
Contributor Author

tejaswinichile commented Jul 20, 2022

@tejaswinichile Noticed that the add_label action didn't work in the screencast.

@pranavrajs Yeah because I didn't have those labels added to my account, it works, let me add the updated video.

Also, for the macros endpoint, we will need to accept an array of conversation ids if we are thinking about having it in bulk actions.

@pranavrajs @sojan-official Should I update the PR for the bulk actions?

The user context should be used. Unlike automation, the action is performed by the agent, so every action should consider the user context

Updating it. I added the Macro as a performer.

@tejaswinichile
Copy link
Contributor Author

Screen.Recording.2022-07-20.at.10.39.12.PM.mov

Hey, @pranavrajs @sojan-official Please have a look at this updated video.

@pranavrajs
Copy link
Member

@pranavrajs @sojan-official Should I update the PR for the bulk actions?

I guess you can update the conversation_id parameter in the API to use conversation_ids.

Copy link
Member

@sojan-official sojan-official left a comment

Choose a reason for hiding this comment

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

Added a couple of comments. Looks good overall.

@@ -34,6 +34,12 @@ def update
end
end

def execute
::MacrosExecutionJob.perform_later(@macro, conversation_ids: params[:conversation_ids], user: current_user)
Copy link
Member

Choose a reason for hiding this comment

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

let's use Current.user instead. We are trying to remove the usage current_user in favor of this.


def send_email_to_team(_params); end

def agent_belongs_to_account?(agent_ids)
Copy link
Member

Choose a reason for hiding this comment

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

some of these methods seem repetitive to what we have in automation service

def agent_belongs_to_account?(agent_ids)

can we extract them to a common base class or a helper and avoid the repetition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we can have a common action service may be. The issue was just with the activity execution, let me check.

@tejaswinichile tejaswinichile force-pushed the feat/5013-execute-macro branch 2 times, most recently from 81a8374 to 7855fb9 Compare July 21, 2022 13:57
@tejaswinichile tejaswinichile force-pushed the feat/5013-execute-macro branch 2 times, most recently from a380f78 to a60b740 Compare July 22, 2022 11:08
@tejaswinichile
Copy link
Contributor Author

@sojan-official This has been fixed please have a look.

Copy link
Member

@sojan-official sojan-official left a comment

Choose a reason for hiding this comment

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

LGTM

@pr-triage pr-triage bot added PR: partially-approved Not all reviewers have approved the PR and removed PR: unreviewed This pull request is yet to be reviewed. labels Jul 25, 2022
@pr-triage pr-triage bot added PR: unreviewed This pull request is yet to be reviewed. and removed PR: partially-approved Not all reviewers have approved the PR labels Jul 26, 2022
@tejaswinichile tejaswinichile merged commit 6a4c0a1 into develop Jul 26, 2022
@tejaswinichile tejaswinichile deleted the feat/5013-execute-macro branch July 26, 2022 07:11
@pr-triage pr-triage bot added PR: merged The pull request is merged to another branch and removed PR: unreviewed This pull request is yet to be reviewed. labels Jul 26, 2022
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR: merged The pull request is merged to another branch
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add an API to execute macros
3 participants