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.

Sign up for GitHub

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

Jump to bottom

Fix issue with webex adapter #200

Merged
merged 9 commits into from
Apr 10, 2020
Merged

Fix issue with webex adapter #200

merged 9 commits into from
Apr 10, 2020

Conversation

viveksyngh
Copy link
Contributor

@viveksyngh viveksyngh commented Feb 7, 2020 •
edited by arm4b

Fix issue with webex adapter

Closes #198

Also, created this PR with hubot-spark adapter.

tonybaloney/hubot-spark#23

Vivek_Singh and others added 7 commits January 24, 2020 13:14
@viveksyngh
Fix issue with spark bot room Id
4a4b4f8 
Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>
@viveksyngh
Update recipient to be consistent with other adapter
55db764 
Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>
@viveksyngh
Move log statment below for error
56a5b53 
Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>
@viveksyngh
Update recipient structure for rooms
a2c9bcd 
Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>
@viveksyngh
Remove user from envelope
08ace71 
Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>
@viveksyngh
Update the room key to roomId
83e6474 
Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>
@viveksyngh
Remove added logging instructions
2e1c23d 
Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>
@viveksyngh viveksyngh requested a review from blag February 7, 2020 11:09
blag
blag suggested changes Feb 7, 2020
View reviewed changes
Copy link
Contributor

@blag blag left a comment

Choose a reason for hiding this comment

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

An excellent start, just needs a tiny bit of linting.

src/lib/adapters/spark.js Outdated Show resolved Hide resolved
src/lib/adapters/spark.js Outdated Show resolved Hide resolved
src/lib/stackstorm_api.js Show resolved Hide resolved
src/lib/stackstorm_api.js Outdated Show resolved Hide resolved
@blag
Copy link
Contributor

blag commented Feb 7, 2020

Just FYI: I refactored this project and split up the adapter modules specifically so people wouldn't have to touch stackstorm_api.js when they were fixing bugs in the adapter modules. It's fine that you did - the only changes you made were some formatting and linting fixups - but it was just mildly surprising to see.

@blag blag mentioned this pull request Feb 7, 2020
Closed
@viveksyngh
Fix formatting issue
04ed86f 
Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>
@viveksyngh
Copy link
Contributor Author

@blag I have addressed the review comments related to formatting.

@viveksyngh viveksyngh requested a review from blag April 7, 2020 05:18
blag
blag suggested changes Apr 7, 2020
View reviewed changes
Copy link
Contributor

@blag blag left a comment

Choose a reason for hiding this comment

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

Looking better but one remaining lint issue and one feature you may still want to support.

src/lib/adapters/spark.js Show resolved Hide resolved
src/lib/adapters/spark.js Outdated Show resolved Hide resolved
src/lib/adapters/spark.js Outdated Show resolved Hide resolved
src/lib/adapters/spark.js Outdated Show resolved Hide resolved
@viveksyngh
Remove unused recipient variable
6962084 
Signed-off-by: Vivek Singh <vivekkmr45@yahoo.in>
blag
blag approved these changes Apr 7, 2020
View reviewed changes
Copy link
Contributor

@blag blag left a comment

Choose a reason for hiding this comment

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

Thanks!

@blag blag merged commit 5a75bef into StackStorm:master Apr 10, 2020
@blag blag mentioned this pull request Apr 10, 2020
Merged
@blag
Copy link
Contributor

blag commented Apr 11, 2020

A note to future people: the build looks like it passed, but the jobs didn't actually pass. There was an issue with mocha where it exited with status 0 even though the tests failed.

@blag blag mentioned this pull request Apr 11, 2020
Merged
4 tasks
This was referenced Apr 15, 2020
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blag blag blag approved these changes

Assignees
No one assigned
Labels
adapter: Cisco Spark bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Chatops does not seem to be working for cisco spark [webex teams]
3 participants
@viveksyngh @blag @arm4b