Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

Notify the user that a commit has finished. #578

Merged
merged 5 commits into from Sep 24, 2014
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions app/utils/environment-change-set.js
Expand Up @@ -398,6 +398,7 @@ YUI.add('environment-change-set', function(Y) {
} else {
this.currentLevel = -1;
delete this.currentCommit;
this.fire('currentCommitFinished');
}
}
},
Expand Down
15 changes: 15 additions & 0 deletions app/widgets/deployer-bar.js
Expand Up @@ -136,6 +136,7 @@ YUI.add('deployer-bar', function(Y) {
container.addClass('deployer-bar');
this._toggleDeployButtonStatus(changes > 0);
ecs.on('changeSetModified', Y.bind(this.update, this));
ecs.on('currentCommitFinished', Y.bind(this.notifyCommitFinished, this));
return this;
},

Expand Down Expand Up @@ -367,6 +368,20 @@ YUI.add('deployer-bar', function(Y) {
}
},

/**
Notify the user that a change-set commit has completed.

@method notifyCommitFinished
*/
notifyCommitFinished: function() {
var db = this.get('db');
db.notifications.add(new Y.juju.models.Notification({
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just pass the object here you don't need to create a new notification instance first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, right. I was trying to get the notification level set up, so I was shotgunning it, turned out to be the level needed to be important or higher.

title: 'All changes committed',
Copy link
Contributor

Choose a reason for hiding this comment

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

can we id this with the changeset number? Changes completed commit: #3

message: 'All requested changes have been sent to Juju and committed.',
Copy link
Contributor

Choose a reason for hiding this comment

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

the idea is that each time a changeset completes we'd note it with a message. In the future the UI would show progress and as each one is done we note it.

level: 'important'
}));
},

/**
Hide the summary panel.

Expand Down
11 changes: 11 additions & 0 deletions test/test_deployer_bar.js
Expand Up @@ -223,6 +223,17 @@ describe('deployer bar view', function() {
assert.equal(called, true);
});

it('notifies the user when a change set is completed', function(done) {
var called = false;
view.notifyCommitFinished = function(evt) {
called = true;
done();
};
view.render();
view.get('ecs').fire('currentCommitFinished');
assert.equal(called, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

This strikes me as a race condition issue in the making and is unnecessary for the test. If it's called, it'll done() else it'll hang and fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as the test below, will re-evaluate both.

});

it('displays the most recent change as a notification on update', function() {
var stubGet = utils.makeStubMethod(view, '_getChangeNotification');
view.render();
Expand Down