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

Initial Bonusly Package Content #7198

Merged
merged 37 commits into from
Sep 1, 2020

Conversation

trorabaugh
Copy link
Contributor

Status

  • [] In Progress
  • Ready
  • In Hold - (Reason for hold)

Related Issues

fixes: link to the issue

Description

This is an initial bonusly package content.

Bonus.ly is an employee recognition platform which enterprises use to for employee recognition. We're building tools to help people feel a sense of purpose and progress at work. The platform which also has an API enables employees to recognize each other by providing a point based bonus system. Bonus.ly helps your employees feel connected, engaged, and aligned is mission critical right now. Bonusly makes employee recognition easy and fun, fostering community and creating company-wide alignment. It also provides employees with positive feedback in the work that they are doing.

Screenshots

Screen Shot 2020-05-23 at 3 44 45 AM

Screen Shot 2020-05-23 at 3 45 23 AM

Screen Shot 2020-05-23 at 3 45 58 AM

Minimum version of Demisto

  • 4.5.0
  • 5.0.0
  • 5.5.0
  • ...

Does it break backward compatibility?

  • Yes
    • Further details:
  • No

Must have

  • [x ] Tests
  • Documentation

Demisto Partner?

  • The title must be in the following format: [YOUR_PARTNER_ID] short description

@CLAassistant
Copy link

CLAassistant commented May 25, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
5 out of 6 committers have signed the CLA.

✅ trorabaugh
✅ idovandijk
✅ anara123
✅ roysagi
✅ kirbles19
❌ Tyler Rorabaugh


Tyler Rorabaugh seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@trorabaugh trorabaugh requested a review from ronykoz May 25, 2020 08:36
@ronykoz ronykoz requested review from roysagi and removed request for ronykoz May 25, 2020 11:44
@ronykoz
Copy link
Contributor

ronykoz commented May 25, 2020

@roysagi let's change the base too

…m for all of the nice work on the updated dev docs. I also wish I could provide AutoGratitude but I dont have admin level points
@trorabaugh
Copy link
Contributor Author

Hi Everyone, I fixed the commit by amending my email. I don't quite follow the CircleCI failures.

Packs/Bonusly/Integrations/Bonusly/README.md Show resolved Hide resolved
Packs/Bonusly/pack_metadata.json Outdated Show resolved Hide resolved
@@ -0,0 +1,52 @@
commonfields:
Copy link
Contributor

Choose a reason for hiding this comment

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

please split this file to python and yml files. You can use demisto-sdk split-yml -i Packs/Bonusly/Scripts/IncOwnerToBonuslyUser.yml -o Packs/Bonusly/Scripts/IncOwnerToBonuslyUser

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is the output command missing a file extension? I don't see what file extension it should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i assume this will create a directory with the split files?

Copy link
Contributor Author

@trorabaugh trorabaugh Jun 5, 2020

Choose a reason for hiding this comment

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

That command produced a python 3.7 error...but Im running 3.8...

Detected python version: [3.8] for docker image: demisto/python3:3.8.2.8159 Copying pipenv files from: /Users/******/IdeaProjects/demisto_content/venv/lib/python3.8/site-packages/demisto_sdk/commands/lint/resources/pipfile_python3 Warning: the environment variable LANG is not set! We recommend setting this in ~/.profile (or equivalent) for proper expected behavior. Warning: Python 3.7 was not found on your system… Would you like us to install CPython 3.7.7 with pyenv? [Y/n]:

Copy link
Contributor

Choose a reason for hiding this comment

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

this seems to be pipenv related - I'll send over a possible solution by slack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be fixed

Packs/Bonusly/Scripts/GetOwnerEmail.yml Outdated Show resolved Hide resolved
Packs/Bonusly/Integrations/Bonusly/Bonusly.yml Outdated Show resolved Hide resolved
Packs/Bonusly/Integrations/Bonusly/Bonusly.yml Outdated Show resolved Hide resolved
Packs/Bonusly/Integrations/Bonusly/Bonusly.yml Outdated Show resolved Hide resolved
Packs/Bonusly/Scripts/GetOwnerEmail.yml Outdated Show resolved Hide resolved


def inc_owner_bonusly_user():
owner_username = demisto.incidents()[0].get("owner")
Copy link
Contributor

Choose a reason for hiding this comment

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

same issue as above - I am not sure this is what you wanted to achieve - please explain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both of these are used to get the owner of the incident's email address and utilized in the playbook as the owner in the product is only the username and not their email. For their playbook to work, you need the incident owners email.

Copy link
Contributor

Choose a reason for hiding this comment

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

same as above - please create an argument for owner_username and remove this line. Use the incident context to fill this argument in the playbook.

Packs/Bonusly/Scripts/GetOwnerEmail.yml Outdated Show resolved Hide resolved


def inc_owner_bonusly_user():
owner_username = demisto.incidents()[0].get("owner")
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above - please create an argument for owner_username and remove this line. Use the incident context to fill this argument in the playbook.

@@ -0,0 +1,52 @@
commonfields:
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems to be pipenv related - I'll send over a possible solution by slack.

@idovandijk
Copy link
Contributor

@roysagi can I begin playbook review here (is the code finalized)?

@roysagi
Copy link
Contributor

roysagi commented Jun 30, 2020

@idovandijk - I talked to @trorabaugh today, he needs to make some changes in his scripts that will affect the playbooks. I think it is best if I ping you once I see the changes have been made.

Copy link
Contributor

@idovandijk idovandijk left a comment

Choose a reason for hiding this comment

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

I like the idea, the playbooks look good. I have a few comments for changes though.
I know you are working on a change for the script so the way the playbook looks may change a bit.
Please reach out if you have any questions or need assistance with making the changes.
Once the changes are done we can probably close this out with a short meeting to see an incident running smoothly.

Bonusly - AutoGratitude

  • I like the use of Lists. In the playbook input BonuslyLookupList I suggest starting the description with a short explanation of what this input is. Then proceed with the explanation you already added which explains how to do it.

  • Task Added scripts (with scripts.json), playbooks and reports. #6 starts a timer but it is never stopped. I think timers are automatically when incidents close, but since this playbook doesn't contain a task that closes the investigation, I recommend using the Done task to stop the time to assignment
    image

  • Why do we want an input checking whether SLA should be checked? The description of the playbook implies that that is the purpose of it, so it would make sense to remove that input and always check SLA, or to change it to "true" by default.
    But removing the SLA part would leave us with a 2-task playbook, one is the script and the other is the create bonus command, so I recommend keeping that - it's a good use case.

  • To make the playbook even more customizable and generic, you could specify the SLAs as playbook inputs, and then look at those inputs. This would make it very easy for other users to simply use this playbook in their incidents and specify in the inputs which SLAs matter for them
    image

  • Please add descriptions to all tasks

Bonusly - Create Bonus

Let's remove this playbook and use the task directly in the other one. If the task can be used just like that then we should avoid complicating with extra playbooks.

@trorabaugh
Copy link
Contributor Author

I like the idea, the playbooks look good. I have a few comments for changes though.
I know you are working on a change for the script so the way the playbook looks may change a bit.
Please reach out if you have any questions or need assistance with making the changes.

Yeah I will have to get to this hopefully either later this week or on the weekend as its a challenge to do during the work week.

Once the changes are done we can probably close this out with a short meeting to see an incident running smoothly.

ok

Bonusly - AutoGratitude

  • I like the use of Lists. In the playbook input BonuslyLookupList I suggest starting the description with a short explanation of what this input is. Then proceed with the explanation you already added which explains how to do it.
    Yeah any suggestions on this or better way would be great.

Ok great ill update that

image

  • Why do we want an input checking whether SLA should be checked? The description of the playbook implies that that is the purpose of it, so it would make sense to remove that input and always check SLA, or to change it to "true" by default.
    But removing the SLA part would leave us with a 2-task playbook, one is the script and the other is the create bonus command, so I recommend keeping that - it's a good use case.

Not sure I quite follow

  • To make the playbook even more customizable and generic, you could specify the SLAs as playbook inputs, and then look at those inputs. This would make it very easy for other users to simply use this playbook in their incidents and specify in the inputs which SLAs matter for them

Yeah the thing I was trying to figure out is how you can use SLA's as inputs as they are generally set by the incident I think. Any thoughts on how to make this customization?

image

  • Please add descriptions to all tasks

Thx for pointing this out ill check them all again

Bonusly - Create Bonus

Let's remove this playbook and use the task directly in the other one. If the task can be used just like that then we should avoid complicating with extra playbooks.

This part I might disagree on. The challenge I've seen with customers is that sometimes they just want to be able to drop a create task in without having to think about how the command works or learn the command. That was why I created a separate playbook so that it could just be included without needing to know how it works. I can remove it though if people don't think its useful. For me I like when I have example CRUD playbooks as it allows me to just drop them into larger playbooks.

@idovandijk
Copy link
Contributor

Not sure I quite follow

I was wondering about this task which determines whether to check the SLA before granting the Bonusly. My thought was that this playbook is there to only provide bonusly of SLA was met, so I can't see the reason in making this check
image

I thought that if we are not checking the SLA then the playbook kinda loses its purpose.
however, on second thought I see that even if not using this playbook only in conjunction with SLAs, it does convert the email to bonusly user, so actually this is up to you whether to leave this as is, or decide that SLA will always be checked :)

Yeah the thing I was trying to figure out is how you can use SLA's as inputs as they are generally set by the incident I think. Any thoughts on how to make this customization?

SLAs can be changed throughout the incident, yes. I believe you can still pass their values as inputs to the playbook. So if incident.detectionsla.breachedTriggered is false, you can pass that "false" value into your playbook as I showed in the screenshot. Then you would check that value under say inputs.DetectionSLABreached... but honestly, this would require customization of task #5 according to their inputs anyway, so if you want you may also leave it like that :)

This part I might disagree on. The challenge I've seen with customers is that sometimes they just want to be able to drop a create task in without having to think about how the command works or learn the command. That was why I created a separate playbook so that it could just be included without needing to know how it works. I can remove it though if people don't think its useful. For me I like when I have example CRUD playbooks as it allows me to just drop them into larger playbooks.

Can you please elaborate? I understand your position but since this playbook only contains 1 task:
image

I can't see the reason for a customer to prefer to edit the playbook inputs instead of task inputs.

Thanks again, please tell me if there are any problems or if something is still unclear.

adding line at the end of py file

def bonusly_test(self):
suffix = 'bonuses'
self._http_request('GET', suffix)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self._http_request('GET', suffix)
return self._http_request('GET', suffix)

I see your test_module is failing, I believe this is the reason

@Itay4 Itay4 mentioned this pull request Aug 4, 2020
8 tasks
@Itay4 Itay4 closed this Aug 12, 2020
@Itay4 Itay4 reopened this Aug 12, 2020
@roysagi
Copy link
Contributor

roysagi commented Aug 12, 2020

Me and @trorabaugh are waiting on a bonusly dummy user for testing - then we proceed with this PR

@trorabaugh
Copy link
Contributor Author

@roysagi - intro was sent if you need anything else just ping them

@trorabaugh
Copy link
Contributor Author

Do I need to do anything for the conflicting files?

@trorabaugh
Copy link
Contributor Author

Anything else I need to do here?

@idovandijk
Copy link
Contributor

Anything else I need to do here?

Looks like just fixing conflicts.. @roysagi and I both approved

@anara123
Copy link
Contributor

@roysagi please the validation and lets merge this

@anara123
Copy link
Contributor

@roysagi please fix the validation error and make sure this PR is merged

@anara123 anara123 merged commit 10557be into demisto:contrib/trorabaugh_bonusly Sep 1, 2020
roysagi added a commit that referenced this pull request Sep 17, 2020
* Initial Bonusly Package Content (#7198)

* Initial Add

* Initial Add of Bonusly. I want to thank francesco and the content team for all of the nice work on the updated dev docs. I also wish I could provide AutoGratitude but I dont have admin level points

* Added additional readme content based on generate docs request

* Updated Ignore

* fixed validation outputs

* fixed validation outputs

* updating validation checks

* initial commit

* added again and also added new comments and fixed incorrect logic

* Fixed commit issues, removed duplicate files, removed individual create bonus playbook

* Added new adjustments based on feedback thanks a bunch Roy

* Added new adjustments based on feedback thanks a bunch Roy

* Update IncOwnerToBonuslyUser.py

adding line at the end of py file

* added pb readme, img

* updated img link in pb readme

* Updated secrets ignore file to ignore the email addresses

* Updated descriptions

* Updated

* Updated descriptions

* Update Bonusly.py

fixing test_module

* Update IncOwnerToBonuslyUser.py

* Update IncOwnerToBonuslyUser.yml

* Update IncOwnerToBonuslyUser.yml

* Update IncOwnerToBonuslyUser.py

* Update IncOwnerToBonuslyUser.py

* Update Bonusly_-_AutoGratitude.yml

* Update Bonusly_-_AutoGratitude.yml

* Update Bonusly_-_AutoGratitude.yml

* Update Bonusly_-_AutoGratitude.yml

* Update IncOwnerToBonuslyUser.yml

* Update IncOwnerToBonuslyUser.yml

* Update Packs/Bonusly/pack_metadata.json

* Update Bonusly.yml

* Update IncOwnerToBonuslyUser.yml

Co-authored-by: Tyler Rorabaugh <[email protected]>
Co-authored-by: roysagi <[email protected]>
Co-authored-by: idovandijk <[email protected]>
Co-authored-by: Alex Fiedler <[email protected]>
Co-authored-by: Anar Azadaliyev <[email protected]>

* remove TPB

* Update Bonusly.yml (#9002)

Co-authored-by: Tyler Rorabaugh <[email protected]>
Co-authored-by: Tyler Rorabaugh <[email protected]>
Co-authored-by: idovandijk <[email protected]>
Co-authored-by: Alex Fiedler <[email protected]>
Co-authored-by: Anar Azadaliyev <[email protected]>
Co-authored-by: ronykoz <[email protected]>
Co-authored-by: syaakovi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contribution Thank you! Contributions are always welcome! docs-approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants