Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

Add name column for webhook and change dashboard messages for webhook #1581

Merged
merged 1 commit into from
Jan 24, 2018
Merged

Add name column for webhook and change dashboard messages for webhook #1581

merged 1 commit into from
Jan 24, 2018

Conversation

termdew
Copy link
Contributor

@termdew termdew commented Jan 10, 2018

Add a name column for webhook and change dashboard messages for webhook activities (e.g updating a webhook or creating a webhook).

Fix #1578
Fix #1340

@termdew
Copy link
Contributor Author

termdew commented Jan 10, 2018

@mssola @vitoravelino

vitoravelino
vitoravelino reviewed Jan 10, 2018
View reviewed changes
spec/controllers/webhooks_controller_spec.rb Outdated
@@ -1,5 +1,4 @@
# frozen_string_literal: true

Copy link
Contributor

Choose a reason for hiding this comment

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

Revert

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

vitoravelino
vitoravelino reviewed Jan 10, 2018
View reviewed changes
spec/factories/webhooks.rb Outdated
@@ -1,5 +1,4 @@
# frozen_string_literal: true

Copy link
Contributor

Choose a reason for hiding this comment

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

Revert

vitoravelino
vitoravelino reviewed Jan 10, 2018
View reviewed changes
spec/models/webhook_spec.rb Outdated
@@ -1,5 +1,4 @@
# frozen_string_literal: true

Copy link
Contributor

Choose a reason for hiding this comment

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

Revert

vitoravelino
vitoravelino reviewed Jan 10, 2018
View reviewed changes
db/migrate/20180109114124_add_name_to_webhooks.rb~ Outdated
@@ -0,0 +1,4 @@
class AddNameToWebhooks < ActiveRecord::Migration
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove file? Leftover?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to create a migration which adds the name. Moreover, this column should never be empty (hence I'd make it mandatory)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a backup file ending with ~. Please remove it.

@termdew termdew force-pushed the webhookName branch 2 times, most recently from 18773be to e71c62c Compare January 10, 2018 14:26
mssola
mssola suggested changes Jan 10, 2018
View reviewed changes
Copy link
Collaborator

@mssola mssola left a comment

Choose a reason for hiding this comment

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

Tests are missing. You should update tests so you also check that:

  • Creating a webhook gives you the right name.
  • You can rename a webhook

db/migrate/20180109114124_add_name_to_webhooks.rb~ Outdated
@@ -0,0 +1,4 @@
class AddNameToWebhooks < ActiveRecord::Migration
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to create a migration which adds the name. Moreover, this column should never be empty (hence I'd make it mandatory)

@termdew termdew force-pushed the webhookName branch 2 times, most recently from 2f17655 to 92be8f6 Compare January 15, 2018 08:34
@termdew termdew force-pushed the webhookName branch 13 times, most recently from a3f814d to af377af Compare January 23, 2018 12:16
vitoravelino
vitoravelino reviewed Jan 23, 2018
View reviewed changes
db/migrate/20160411150441_create_webhooks.rb Outdated
@@ -8,7 +8,7 @@ def change
t.integer :request_method
t.integer :content_type
t.boolean :enabled, default: false

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove whitespace.

Copy link
Contributor

@vitoravelino vitoravelino Jan 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

File still here...

@SUSE SUSE deleted a comment from termdew Jan 23, 2018
vitoravelino
vitoravelino reviewed Jan 23, 2018
View reviewed changes
db/migrate/20160411150441_create_webhooks.rb Outdated
@@ -8,7 +8,6 @@ def change
t.integer :request_method
t.integer :content_type
t.boolean :enabled, default: false

t.timestamps null: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing changed in this file. You can simply revert the file or create an empty line here to restore the previous state of the file and avoid this change.

mssola
mssola previously approved these changes Jan 23, 2018
View reviewed changes
@mssola
Copy link
Collaborator

mssola commented Jan 23, 2018

The failure on travis does not look related to this PR (flaky test ? 😨)

@vitoravelino
Copy link
Contributor

The failure on travis does not look related to this PR (flaky test ? 😨)

I don't think so... Let's hope not.

vitoravelino
vitoravelino approved these changes Jan 23, 2018
View reviewed changes
Copy link
Contributor

@vitoravelino vitoravelino left a comment

Choose a reason for hiding this comment

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

👍

@mssola mssola merged commit 72dd43e into SUSE:master Jan 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mssola mssola mssola approved these changes

@vitoravelino vitoravelino vitoravelino 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.
3 participants
@termdew @mssola @vitoravelino