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

Create plain-text transform to cover the HipChat and Flowdock transform #125

Merged
merged 3 commits into from
Oct 19, 2017
Merged

Create plain-text transform to cover the HipChat and Flowdock transform #125

merged 3 commits into from
Oct 19, 2017

Conversation

llvieira
Copy link
Member

Closes #124.

  • Created a plain-text-transform to cover the HipChat and Flowdock transform.
  • Fixed some bugs: res.count changed to res.members.length and fixed send() method in the plain-text.

danielreed
danielreed reviewed Oct 18, 2017
View reviewed changes
Copy link
Member

@danielreed danielreed left a comment

Choose a reason for hiding this comment

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

These changes are a big improvement! I mostly have questions on the changes.

src/listener/alerts-listener.js
@@ -54,7 +54,7 @@ export default class AlertsListener extends Listener {
let time = msg.time;

this.client.Alerts.getFilteredAlerts(status, state, time).then((res) => {
if (res.count === 0) {
if (res.members.length === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Why change this here? Using the res.count is probably fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Basically because the response does not have the count attribute, it has only a member array.

src/listener/ov-listener.js Outdated
@@ -62,7 +62,15 @@ export default function(robot, client) {

let checkedMessage = filter.check(resource);
if (typeof checkedMessage !== 'undefined' && checkedMessage.length > 0) {
transform.messageRoom(client.notificationsRoom, resource.resource);
let room = '';
Copy link
Member

Choose a reason for hiding this comment

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

Can you shorten this to:

let room = '#' + client.notificationsRoom;
if (robot.adapterName === 'flowdock') {
room = client.notificationsRoom;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Great, fixed.

src/listener/server-hardware.js
if (res.count === 0) {
return this.transform.text(msg, msg.message.user.name + ", I didn't find any blades with a " + msg.status.toLowerCase() + " status.");

if (res.members.length === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to change line 183?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we do. See the comment above.

src/listener/server-hardware.js Outdated
if (msg.status.toLowerCase() === "ok") {
return this.transform.send(msg, res, "Okay " + msg.message.user.name + ", the following blades have an " + msg.status.toUpperCase() + " status.");
if (status.toLowerCase() === "ok") {
return this.transform.send(msg, res, "Okay " + msg.message.user.name + ", the following blades have an " + status.toLowerCase() + " status.");
Copy link
Member

Choose a reason for hiding this comment

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

Why lower case on the status in the message to the user?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

src/listener/server-hardware.js
@@ -198,7 +199,7 @@ export default class ServerHardwareListener extends Listener {
let status = msg.powerState.substring(8, msg.powerState.length);
status = status.charAt(0).toUpperCase() + status.slice(1);
this.client.ServerHardware.getHardwareByPowerState(status).then((res) => {
if (res.count === 0) {
if (res.members.length === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, see the comment above.

src/listener/server-profiles.js
@@ -81,7 +81,7 @@ export default class ServerProfilesListener extends Listener {
let status = msg.status.toLowerCase();
status = status.charAt(0).toUpperCase() + status.slice(1);
this.client.ServerProfiles.getProfilesByStatus(status).then((res) => {
if (res.count === 0) {
if (res.members.length === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, see the comment above.

src/listener/utils/transforms/plain-text-transformer.js Outdated
return 'Flowdock';
} else {
return 'HipChat';
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we name this file just plain-text.js? No need to include transformer in the name of the file, this will match the slack and shell transformers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, fixed!

src/listener/utils/transforms/plain-text-transformer.js Outdated
@@ -77,7 +77,10 @@ function output(resource) {
return [];
}

export default class FlowdockTransform {
/**
Copy link
Member

Choose a reason for hiding this comment

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

I think JS multi-line comments are just /* */

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

src/listener/utils/transforms/plain-text-transformer.js Outdated
@@ -100,8 +103,8 @@ export default class FlowdockTransform {

const out = output(resource).join('\n');
if (text) {
msg.send(out);
}
msg.send(text);
Copy link
Member

Choose a reason for hiding this comment

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

Aren't we supposed to use out in the msg.send()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we are using yet.

Before:

if (text) {
      msg.send(out);
}

msg.send(out);

After:

if (text) {
      msg.send(text);
}

msg.send(out);

The bug was fixed, because before the output was being shown 2 times and the text never.

@llvieira
Updating with Daniel's review: plain-text transform was renamed, comm…
02fe2ae 
…ent fixed, and improving the if statement to get the right room
@llvieira llvieira changed the title Create plain-text-transform to cover the HipChat and Flowdock transform Create plain-text transform to cover the HipChat and Flowdock transform Oct 19, 2017
@danielreed danielreed merged commit 47cc129 into master Oct 19, 2017
@llvieira llvieira deleted the refactoring_transform branch October 19, 2017 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielreed danielreed danielreed approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor transform to cover all adapters that use plain-text responses
2 participants
@llvieira @danielreed