Skip to content

Conversation

@gavalur-sfdc
Copy link
Collaborator

  • create_composite_alert (creates empty composite alert)
  • update_composite_alert (updates composite alert with updated terms, termconditions etc)
  • add/delete child alert from composite alert
  • add/delete trigger from child alert of a composite alert
  • add/delete notification from a composite alert
  • get_children_info from a composite alert

@salesforce-cla
Copy link

Thanks for the contribution! It looks like @gavalur-sfdc is an internal user so signing the CLA is not required. However, we need to confirm this.

Copy link
Contributor

@hdara-sfdc hdara-sfdc left a comment

Choose a reason for hiding this comment

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

@gavalur-sfdc You seem to have a lot autoformatting diff, please revert the diff that is unrelated to your changes (and keep the corresponding IDE settings disabled for future) and update the PR.

Copy link
Contributor

@hdara-sfdc hdara-sfdc left a comment

Choose a reason for hiding this comment

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

Could you please add pydoc for all the new functions? Is payload another alert ID? What about create?

I would suggest a few consistency changes:

  • Wherever an alert ID refers to the composite alert ID, we should call it composite_alert_id and then refer to it as such in the error messages instead of "compose alert_id".
  • payload can be better named based on whether it is an ID or a data object.

Since there are a functions that I thought are duplicates, I didn't do a full review, especially the tests.

@hdara-sfdc
Copy link
Contributor

@gavalur-sfdc I resolved most of the threads, but there are a few past threads that haven't received any response from you so I am not even sure if you ever saw them. Please look for all unresolved items.

Also, it looks like you haven't signed salesforce-cla and the merge will be blocked until you resolve it.

@gavalur-sfdc
Copy link
Collaborator Author

@gavalur-sfdc I resolved most of the threads, but there are a few past threads that haven't received any response from you so I am not even sure if you ever saw them. Please look for all unresolved items.

Also, it looks like you haven't signed salesforce-cla and the merge will be blocked until you resolve it.

Fixed the code and clarified couple of comments you had on a bug.

@gavalur-sfdc
Copy link
Collaborator Author

@gavalur-sfdc I resolved most of the threads, but there are a few past threads that haven't received any response from you so I am not even sure if you ever saw them. Please look for all unresolved items.
Also, it looks like you haven't signed salesforce-cla and the merge will be blocked until you resolve it.

Fixed the code and clarified couple of comments you had on a bug.

I resolved a comment saying that create_composite_alert is deleted. Since Creating a composite alerts is a multi-step process, we need to first create a dummy composite alert with proper alert type and then start adding child alerts one after another. In the end we need do an update with composite alert definition.

@gavalur-sfdc
Copy link
Collaborator Author

Will add more test coverage for the bug you've pointed out.

@hdara-sfdc
Copy link
Contributor

@gavalur-sfdc Please don't explicitly resolve threads started by me, I will resolve them when they have been addressed to my satisfaction.

Also, it looks like you haven't signed salesforce-cla and the merge will be blocked until you resolve it.

This is still pending.

@gavalur-sfdc
Copy link
Collaborator Author

I have updated the testDeleteChildAlert to retrieve the deleted. I tried signing up the CLA long time back and I did it again, couple of days back. But for reason its not showing up on the git. For security reasons I am sending you the screenshot through email.

Copy link
Contributor

@hdara-sfdc hdara-sfdc left a comment

Choose a reason for hiding this comment

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

It looks like create_composite_alert is either redundant (in favor of add_child_alert_to_composite_alert) and so can be removed? If not there is test coverage missing for it along with get_composite_alert_children.

@gavalur-sfdc
Copy link
Collaborator Author

It looks like create_composite_alert is either redundant (in favor of add_child_alert_to_composite_alert) and so can be removed? If not there is test coverage missing for it along with get_composite_alert_children.

create_composite_alert is redundant, which I removed from the moncfg side, but forgot to comment it out in client.py. I will remove it. I am adding test coverage for get_composite_alert_children

Copy link
Contributor

@hdara-sfdc hdara-sfdc left a comment

Choose a reason for hiding this comment

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

lgtm now

You may also want to bump up the version.

@j-ma-sf
Copy link
Contributor

j-ma-sf commented Jun 2, 2021

Thanks for the contribution! It looks like @gavalur-sfdc is an internal user so signing the CLA is not required. However, we need to confirm this.

@gavalur-sfdc You just need to fill out the form in the link

@j-ma-sf j-ma-sf merged commit 01b1bbd into master Jun 2, 2021
@j-ma-sf j-ma-sf deleted the composite_alert_support branch June 2, 2021 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants