Fixes #25047 - Notification link #233

johnsonm325 · 2018-10-02T19:13:46Z

This is my first attempt at fixing this code. I'm getting it to work correctly to fix the issue, but it's causing some weird problems with other success notification windows. It seems to stall the process of showing the notification window and any action after that.

Also, I'm getting hung up with the tests that I wrote. I'm unsure of how to pass the parameters correctly and use the default parameters when only a message is passed to the function. Failing tests look like so:

PhantomJS 2.1.1 (Linux 0.0.0) Factory: Nofification provides a way to set success messages FAILED
	TypeError: undefined is not an object (evaluating 'options.context') in /home/vagrant/bastion/app/assets/javascripts/bastion/components/notification.service.js (line 9)
	setSuccessMessage@/home/vagrant/bastion/app/assets/javascripts/bastion/components/notification.service.js:9:1073
	/home/vagrant/bastion/test/components/notification.service.test.js:35:39
PhantomJS 2.1.1 (Linux 0.0.0) Factory: Nofification allows message context to be specified for interpolation FAILED
	Expected spy notify to have been called with [ Object({ message: 'Everything ran correctly yay!', type: 'success', link: '' }) ] but actual calls were [ Object({ message: 'Everything ran correctly {{ ending }}!', type: 'success', link: '' }) ].
	/home/vagrant/bastion/test/components/notification.service.test.js:46:71

theforeman-bot · 2018-10-02T19:13:50Z

@johnsonm325, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #25047: Deleting repositories results in HTML output in the success message is the correct one
move ticket #25047 from Katello to the Bastion project
or file a new ticket in the Bastion project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

johnpmitsch · 2018-10-02T20:22:10Z

app/assets/javascripts/bastion/components/notification.service.js

+        if (options === angular.isUndefined) {
+            options = {link: ""};
+        }
+        foreman.toastNotifications.notify({message: interpolateIfNeeded(message, options.context), type: 'success', link: options.link});


I was more thinking of leaving the options open and just passing that along to the toast notification. This way whatever is in the object becomes a prop in the toast notification, leaving this flexible.

this.setSuccessMessage = function (message, options) { if (options === angular.isUndefined) { options = {}; } var baseOptions = { message: interpolateIfNeeded(message, options.context), type: 'success'}; // combine options and base options to one object var fullOptions = Object.assign(baseOptions, options); foreman.toastNotifications.notify(fullOptions);

Then you can pass in a link and it will make its way to the react component. i.e. setSuccessMessage("it worked", {link: "/path/to/thing"}) but you can also pass in things like title and any other props that may be adde din the future.

With this approach any existing call of setSuccessMessage will work with only one option i.e. setSuccessMessage("it worked!"), looks like this is the majority of calls katello uses.

If context was passed in i.e. setSuccessMessage("it worked", "some context"), that will have to be updated to setSuccessMessage("it worked", { context: "some context" }).

One potential caveat: If you run into any problems passing along context to the react component, that may have to be popped out of the object before it's sent in the notify method, let me know if you see anything like this (you will probably see an invalid propType error message if this is the case).

@johnpmitsch I'm getting the following error for it:

TypeError: Cannot read property 'context' of undefined at Object.setSuccessMessage (bastion-46c70c6bc8dbfb49bd30f57d8ec38039cf096443284e5bd4a0dac8a55ac8737d.js:87515) at success (bastion_katello-638b1c206284a094f9567ef353eed3374f77bbf130771fc42a829119c85fb8a5.js:14516) at bastion-46c70c6bc8dbfb49bd30f57d8ec38039cf096443284e5bd4a0dac8a55ac8737d.js:48330 at processQueue (bastion-46c70c6bc8dbfb49bd30f57d8ec38039cf096443284e5bd4a0dac8a55ac8737d.js:32696) at bastion-46c70c6bc8dbfb49bd30f57d8ec38039cf096443284e5bd4a0dac8a55ac8737d.js:32712 at Scope.$eval (bastion-46c70c6bc8dbfb49bd30f57d8ec38039cf096443284e5bd4a0dac8a55ac8737d.js:33964) at Scope.$digest (bastion-46c70c6bc8dbfb49bd30f57d8ec38039cf096443284e5bd4a0dac8a55ac8737d.js:33780) at Scope.$apply (bastion-46c70c6bc8dbfb49bd30f57d8ec38039cf096443284e5bd4a0dac8a55ac8737d.js:34072) at done (bastion-46c70c6bc8dbfb49bd30f57d8ec38039cf096443284e5bd4a0dac8a55ac8737d.js:28307) at completeRequest (bastion-46c70c6bc8dbfb49bd30f57d8ec38039cf096443284e5bd4a0dac8a55ac8737d.js:28513)

It's hitting it at the interpolateIfNeeded call. Since context isn't in options, it's undefined. I can jury-rig a fix using a messy if statement, but that doesn't seem to be the best approach. Let me know if that's correct.

Error above came while creating a new repository in the products. Deleting repositories in a product seems to work fine.

@johnsonm325 can you push up the code?

johnpmitsch · 2018-10-02T21:07:46Z

test/components/notification.service.test.js

@@ -38,10 +38,18 @@ describe('Factory: Nofification', function() {

    it("allows message context to be specified for interpolation", function () {
        var message = "Everything ran correctly {{ ending }}!",
+            options = {link: ""},
            $scope = {ending: 'yay'},


Its not obvious, but you'll have to put the context in the options
options = {link: "", context: {ending: yay}}
then call as Notification.setSuccessMessage(message, options)

theforeman-bot · 2018-10-03T15:26:43Z

@johnsonm325, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #25047: Deleting repositories results in HTML output in the success message is the correct one
move ticket #25047 from Katello to the Bastion project
or file a new ticket in the Bastion project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

johnsonm325 · 2018-10-03T15:29:32Z

@johnpmitsch new code pushed up.

johnpmitsch · 2018-10-03T17:24:29Z

app/assets/javascripts/bastion/components/notification.service.js

+    this.setSuccessMessage = function (message, options) {
+        if (options === angular.isUndefined) {
+            options = {};
+        }


Changing this to (options === undefined) && (options = {}); worked for me. This is probably due to the async nature of javascript, having a boolean expression forces each side to be evaluated and it declares the options variable before the rest of the code is run.

Did you change this from the example because of eslint?

@johnpmitsch yes, that is exactly why I changed it. It was saying it expected a conditional or command instead of an expression. I'll change it back, but what do we do with eslint? Ignore the error? If so, I'll just add an eslint-disable-next-line.

yes, just change it back and try ignoring it. You will need to use the older eslint syntax as shown here and use the proper rule name. Angular uses a different eslint version than react code.

theforeman-bot · 2018-10-04T17:22:55Z

@johnsonm325, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #25047: Deleting repositories results in HTML output in the success message is the correct one
move ticket #25047 from Katello to the Bastion project
or file a new ticket in the Bastion project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

theforeman-bot · 2018-10-04T20:28:22Z

@johnsonm325, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

check #25047: Deleting repositories results in HTML output in the success message is the correct one
move ticket #25047 from Katello to the Bastion project
or file a new ticket in the Bastion project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

This message was auto-generated by Foreman's prprocessor

johnsonm325 · 2018-10-04T20:32:28Z

@johnpmitsch pushed up new code.

johnpmitsch

some minor refactoring comments, but overall looks good!

johnpmitsch · 2018-10-04T20:38:01Z

app/assets/javascripts/bastion/components/notification.service.js

-    this.setSuccessMessage = function (message, context) {
-        foreman.toastNotifications.notify({message: interpolateIfNeeded(message, context), type: 'success'});
+    this.setSuccessMessage = function (message, options) {
+        /* eslint-disable no-unused-expressions, angular/ng_definedundefined*/


You can use angular.isUndefined, no need to disable it, it should function the same as undefined

johnpmitsch · 2018-10-04T20:42:45Z

app/assets/javascripts/bastion/components/notification.service.js

-        foreman.toastNotifications.notify({message: interpolateIfNeeded(message, context), type: 'success'});
+    this.setSuccessMessage = function (message, options) {
+        /* eslint-disable no-unused-expressions, angular/ng_definedundefined*/
+        var baseOptions = {};


You can initialize a variable and then assign a value to it later. This is a good practice in JS (well, for es5) because of hoisting, which is why eslint recommends it.

i.e.

var baseOptions, fullOptions; // other stuff baseOptions = { ham: "sandwich" }; fullOptions = { turkey: 'club' };

johnpmitsch · 2018-10-04T20:44:10Z

app/assets/javascripts/bastion/components/notification.service.js

+        /* eslint-disable no-unused-expressions, angular/ng_definedundefined*/
+        var baseOptions = {};
+        var fullOptions = {};
+        (options === undefined) && (options = {});


Since this is the only line we want to disable the no-unused-expressions rule on, you can move the eslint disable line above it and then re-enable the rule after it.

johnsonm325 · 2018-10-05T15:36:20Z

@johnpmitsch new code pushed up and tests have passed. I don't remember seeing anywhere else where we send links to setWarningMessage or setErrorMessage, but wouldn't we want to go ahead and modify the code in those functions to ensure that we don't run into this issue again? And to ensure that we're handling these in the same way?

johnpmitsch · 2018-10-05T16:27:29Z

@johnsonm325 Maybe grep around for the other functions in Katello and see if we are passing links to any of them.

If you do go that route, let me know as I have some ideas, but I think its fine to just update only success message if there aren't broken links in warning or error messages.

johnpmitsch · 2018-10-05T16:38:02Z

I think its fine to just update only success message if there aren't broken links in warning or error messages.

Of course, if you want to update them all no one will stop you 😏

Looks good, but I plan to test with the accompoying katello PR

johnsonm325 · 2018-10-08T14:03:28Z

Looks good, but I plan to test with the accompanying katello PR

@johnpmitsch We need to merge this before we create the Katello PR, if I understood correctly. Or am I missing something?

johnpmitsch · 2018-10-08T14:05:08Z

@johnsonm325 we need to merge this before the katello PR is merged, you can create the katello PR without issue right now, I'd like to test them together before merging the bastion one.

johnpmitsch · 2018-10-11T20:48:37Z

@ehelms @waldenraines Can we get a merge?

theforeman-bot added the Waiting on contributor label Oct 2, 2018

theforeman-bot added Needs testing Not yet reviewed labels Oct 2, 2018

johnpmitsch suggested changes Oct 2, 2018

View reviewed changes

johnpmitsch reviewed Oct 2, 2018

View reviewed changes

johnsonm325 force-pushed the notificationLink branch from db1f33a to 9f359e3 Compare October 3, 2018 15:26

theforeman-bot removed the Waiting on contributor label Oct 3, 2018

johnpmitsch reviewed Oct 3, 2018

View reviewed changes

johnsonm325 force-pushed the notificationLink branch from 9f359e3 to e6c4559 Compare October 4, 2018 17:22

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Oct 4, 2018

johnsonm325 force-pushed the notificationLink branch from e6c4559 to 206dcef Compare October 4, 2018 20:28

theforeman-bot added Waiting on contributor and removed Waiting on contributor labels Oct 4, 2018

johnsonm325 changed the title ~~WIP - Fixes #25047 - Notification link~~ Fixes #25047 - Notification link Oct 4, 2018

johnpmitsch suggested changes Oct 4, 2018

View reviewed changes

Fixes #25047 - Notification link

f979794

johnsonm325 force-pushed the notificationLink branch from 206dcef to f979794 Compare October 5, 2018 15:23

johnsonm325 mentioned this pull request Oct 8, 2018

Fixes #25146 - SuccessMessage call sends link Katello/katello#7743

Merged

johnpmitsch approved these changes Oct 11, 2018

View reviewed changes

waldenraines merged commit 41f775b into Katello:master Oct 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #25047 - Notification link #233

Fixes #25047 - Notification link #233

johnsonm325 commented Oct 2, 2018

theforeman-bot commented Oct 2, 2018

johnpmitsch Oct 2, 2018

johnpmitsch Oct 2, 2018

johnsonm325 Oct 3, 2018

johnsonm325 Oct 3, 2018

johnpmitsch Oct 3, 2018

johnpmitsch Oct 2, 2018

theforeman-bot commented Oct 3, 2018

johnsonm325 commented Oct 3, 2018

johnpmitsch Oct 3, 2018

johnsonm325 Oct 3, 2018

johnpmitsch Oct 3, 2018

theforeman-bot commented Oct 4, 2018

theforeman-bot commented Oct 4, 2018

johnsonm325 commented Oct 4, 2018

johnpmitsch left a comment

johnpmitsch Oct 4, 2018

johnpmitsch Oct 4, 2018

johnpmitsch Oct 4, 2018

johnsonm325 commented Oct 5, 2018

johnpmitsch commented Oct 5, 2018

johnpmitsch commented Oct 5, 2018

johnsonm325 commented Oct 8, 2018

johnpmitsch commented Oct 8, 2018

johnpmitsch commented Oct 11, 2018

Fixes #25047 - Notification link #233

Fixes #25047 - Notification link #233

Conversation

johnsonm325 commented Oct 2, 2018

theforeman-bot commented Oct 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theforeman-bot commented Oct 3, 2018

johnsonm325 commented Oct 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theforeman-bot commented Oct 4, 2018

theforeman-bot commented Oct 4, 2018

johnsonm325 commented Oct 4, 2018

johnpmitsch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnsonm325 commented Oct 5, 2018

johnpmitsch commented Oct 5, 2018

johnpmitsch commented Oct 5, 2018

johnsonm325 commented Oct 8, 2018

johnpmitsch commented Oct 8, 2018

johnpmitsch commented Oct 11, 2018