Adding message page, for better content sharing #381

WojcikMike · 2016-10-03T12:18:40Z

Fixes #331

This PR requires corresponding change in SC to happen

WojcikMike · 2016-10-10T10:02:10Z

@Particular/servicepulse-maintainers please review. The PR for SC was merged, but now I need to learn when it will be released.

WilliamBZA

LGTM! A couple of suggestions around how we handle the promises for the serviceControlService service, but other than that 👍

WilliamBZA · 2016-10-12T06:09:15Z

src/ServicePulse.Host/app/js/views/message/controller.js

+        };
+
+        vm.archiveMessage = function () {
+            serviceControlService.archiveFailedMessages([vm.message.message_id]);


Can we change the archiveFailedMessages method to return the promise so that it's the same as the restoreMessageFromArchive method?

It also means we can chain the vm.message.archived = true; line onto only the success portion of the POST. In case the network is down or something goes wrong with the actual HTTP comms, then we won't set the archived flag to true incorrectly.

WilliamBZA · 2016-10-12T06:09:46Z

src/ServicePulse.Host/app/js/views/message/controller.js

+        };
+
+        vm.retryMessage = function () {
+            serviceControlService.retryFailedMessages([vm.message.message_id]);


Can we change the retryFailedMessages method to return the promise so that it's the same as the restoreMessageFromArchive method?

It also means we can chain the vm.message.retried = true; line onto only the success portion of the POST. In case the network is down or something goes wrong with the actual HTTP comms, then we won't set the retried flag to true incorrectly.

WilliamBZA · 2016-10-12T11:24:04Z

👍

johnsimons · 2016-10-28T04:58:00Z

@WojcikMike this is not released as part of v1.24.

To use it call GET /errors/last/{id}, see Particular/ServiceControl@40099c0#diff-6fe7def7f518c7a5832858f293f5e95dR31

WojcikMike · 2016-10-31T11:22:09Z

@johnsimons ok will revisit this PR

WojcikMike · 2017-01-31T15:08:33Z

@sergioc I did the changes, you can navigate to the message page by going to failed messages screen and clicking on a message name

…ssage)

- restored row hover state in All Failed Messages / Group detail views - updated message detail page

WojcikMike · 2017-02-09T13:54:55Z

@Particular/servicepulse-maintainers can you please review? We believe it is ready to be shipped

…king for failed message groups

sergioc · 2017-02-09T16:18:15Z

src/ServicePulse.Host/app/js/views/archive/view.html

@@ -51,7 +51,7 @@
                    </div>

                    <div class="col-xs-11 failed-message-data" ng-mouseenter="message.hover2 = true" ng-mouseleave="message.hover2 = false">
-                        <a href="#/failedMessages/{{message.id}}">
+                        <a href="#/message/{{message.id}}">


@WojcikMike isn't an invidual message a child of all /failedMessages?

It is and it isn't :)
Previously our URL's looked as follows:
/failedMessages - failed mesasges screen
/failedMessages/{{id}} - one message screen

Becasue @WilliamBZA made a pr to provide deep linking groups he used "our" url path, so currently it looks like this:
/failedMessages - failed mesasges screen
/failedMessages/{{id}} - failed messages screen with a group of given id selected

Which means that we lacked url for this feature. Now becasue on our page we show a bit more than only failed message (as it can also be archived message, or succesfful processed message) I decided to go with path:
/message/{{id}}

What do you think?

From a nav perspective, /failedMessages is parent of both groups and individual messages, so I'd nest both children under the parent. I'd also avoid two words in oneWord to improve legibility of the URL, as well as caps. So I'd use this structure in the end:

/failed-messages

/failed-messages/group/{{group.id}}

/failed-messages/message/{{message.id}}

Even that it looks nice, I don't like it :D.
With regards to groups, It should be more of:
/group/{{group.id}}/failed-messages
As the group is the parent entity that we care about, and group contains failed messages. So for me ^^ look more natural.
With regards to message path, as we are showing also archived messages I think we should not keep it under failed-message umbrela. Either way, I would tackle this as a separate issue, simply becasue group id path is already out there. And we can do the change of the URL's as a separate initiative with redirects from old paths to new etc.

Either way, I would tackle this as a separate issue, simply becasue group id path is already out there.

Yeah, thought about that as soon as I started seeing you disagree :-)

Let's leave URL formatting out of the picture for now and release the rest.

sergioc · 2017-02-09T16:18:23Z

src/ServicePulse.Host/app/js/views/failed_messages/view.html

@@ -65,7 +65,7 @@ <h3 class="active group-title group-message-count">{{vm.selectedExceptionGroup.c
                    </div>

                    <div class="col-xs-11 failed-message-data" ng-mouseenter="message.hover2 = true" ng-mouseleave="message.hover2 = false">
-                        <a href="#/failedMessages/{{message.id}}">
+                        <a href="#/message/{{message.id}}">


@WojcikMike isn't an invidual message a child of all /failedMessages?

WilliamBZA · 2017-02-14T10:48:14Z

src/ServicePulse.Host/app/css/particular.css

@@ -390,7 +391,7 @@ h3 {
    border-radius: 4px;
    color: #777f7f;
    display: block;
-    height: 130px;
+//    height: 130px;


Commented out?

@WojcikMike I'll fix this once you're done with your changes. Or you can just remove the commented out line yourself.

I already removed it

WilliamBZA · 2017-02-15T06:55:15Z

src/ServicePulse.Host.Tests/tests/js/views/message/controller.spec.js

+        $controller = _$controller_;
+    }));
+
+    describe('retrying message', function () {


Can we improve the names of these tests? It's a bit hard to see what is being tested at a glance.

Perhaps: when retrying a message

I mean, we can be quite verbose with qUnit.

WilliamBZA · 2017-02-15T06:55:28Z

src/ServicePulse.Host.Tests/tests/js/views/message/controller.spec.js

+            });
+        }));
+
+        it('message marked as resolved', function () {


Perhaps: and the retry succeeds, the message is marked as resolved

WilliamBZA · 2017-02-15T06:59:26Z

src/ServicePulse.Host.Tests/tests/js/views/message/controller.spec.js

+            expect(serviceControlService.retryFailedMessages).toHaveBeenCalled();
+        });
+
+        it('failed, message not marked as resolved', function () {


Perhaps and the retry fails, the message is not marked as resolved

WilliamBZA · 2017-02-15T06:59:55Z

src/ServicePulse.Host.Tests/tests/js/views/message/controller.spec.js

+        });
+    });
+
+    describe('archiving message', function () {


Perhaps when archiving a message

WilliamBZA · 2017-02-15T07:38:29Z

src/ServicePulse.Host/app/js/views/message/controller.js

+        vm.retryMessage = function () {
+            serviceControlService.retryFailedMessages([vm.message.id])
+                .then(function() {
+                        toastService.showInfo("Retrying the message " + vm.message.message_id + " ...");


This should probably be shown before the promise completes

WilliamBZA · 2017-02-15T07:38:34Z

src/ServicePulse.Host/app/js/views/message/controller.js

+        vm.archiveMessage = function () {
+            serviceControlService.archiveFailedMessages([vm.message.id])
+                .then(function() {
+                    toastService.showInfo("Archiving the message " + vm.message.message_id + " ...");


This should probably be shown before the promise completes

WilliamBZA · 2017-02-15T07:39:22Z

src/ServicePulse.Host/app/js/views/message/controller.js

+        };
+
+        //to be deleted
+        vm.resolveButtonClicked = function() {


to be deleted?

WilliamBZA · 2017-02-15T07:40:28Z

src/ServicePulse.Host/app/js/views/message/controller.js

+                });
+        };
+
+        function calculateDeleteDate(message, errorRetentionPeriod) {


calculate to me means a single result (an answer) will be provided, but this method mutates the incoming object.

Maybe the name should be updateMessageDeleteDate ?

WilliamBZA · 2017-02-15T07:41:12Z

src/Setup/res/License.rtf

-
-\pard\widctlpar\sb240\sa240\b0 YOU EXPRESSLY ACKNOWLEDGE THAT YOU HAVE READ THESE TERMS AND UNDERSTAND THE RIGHTS, OBLIGATIONS, TERMS AND CONDITIONS SET FORTH HEREIN. BY CLICKING ON THE AGREE BUTTON AND/OR CONTINUING TO INSTALL OR USE THE SOFTWARE, YOU EXPRESSLY CONSENT TO BE BOUND BY THESE TERMS.\par
-}
+{\rtf1\ansi\ansicpg1252\deff0\nouicompat\deflang3081\deflangfe3081{\fonttbl{\f0\fswiss\fprq2\fcharset0 Calibri;}{\f1\froman\fprq2\fcharset0 Times New Roman;}{\f2\fswiss\fprq2 Calibri;}}


What are these changes?

THe file before and after is identical, I think that this checkin sneaked upon me ;)

WilliamBZA · 2017-02-15T07:42:26Z

I suspect this was not a small and should not have been done under maintainer budget. Do you both still think the small label was appropriate?

If not, how can we better our triage process so we don't make a similar mistake again?

WojcikMike · 2017-02-15T10:39:17Z

With regards to it being bigger than small. Yes overall I think it was. At first it seemed like small anough but after I brough @sergioc in he wanted to work on the experience between the screens to make them better and consistent. And that brough additional work. I would risk a statement that we can't make anything small when it includes UI changes.

sergioc · 2017-02-15T14:59:22Z

I would risk a statement that we can't make anything small when it includes UI changes.

I wouldn't generalize it like that. That would make UI changes difficult to do because it would suffer on prioritization. This is especially dangerous since the main component of SP is the UI.

There's also the question of what's the definition of small. Is it the no. of work hours? Lines of code? Time the issue sits in progress? I can only correctly assess size if the criteria for sizing is clear.

…ssages screen

WojcikMike · 2017-02-16T12:38:55Z

@WilliamBZA I believe that all your suggestions were addressed

WilliamBZA

Thanks. I neatened up some promises that weren't changed by you guys too. LGTM.

Are we going to hold on shipping this until we've done #402?

WilliamBZA · 2017-02-17T06:32:15Z

I've also added #404 to the same milestone

WilliamBZA · 2017-02-19T09:36:15Z

Also, I think this should be a squash merge

sergioc · 2017-02-20T08:13:04Z

@WilliamBZA the maintainers agreed #402 is indispensable, so I added it to the same milestone.

WojcikMike mentioned this pull request Oct 3, 2016

Modify GetErrorById to return data in similar format as GetAllErrors Particular/ServiceControl#795

Merged

WojcikMike self-assigned this Oct 4, 2016

WilliamBZA approved these changes Oct 12, 2016

View reviewed changes

WojcikMike force-pushed the add_message_page branch 2 times, most recently from 0df559f to e28f171 Compare October 12, 2016 13:27

WojcikMike force-pushed the add_message_page branch from f751648 to 55d4c6c Compare October 31, 2016 12:06

WilliamBZA changed the title ~~[WIP] Adding message page, for better content sharing~~ [WIP] Adding message page, for better content sharing Dec 1, 2016

WilliamBZA added the in progress label Dec 1, 2016

WojcikMike force-pushed the add_message_page branch from 6a0c066 to 7606813 Compare January 31, 2017 15:02

WilliamBZA added the State: In Progress label Feb 6, 2017

sergioc self-assigned this Feb 6, 2017

WojcikMike changed the title ~~[WIP] Adding message page, for better content sharing~~ Adding message page, for better content sharing Feb 9, 2017

WojcikMike and others added 13 commits February 9, 2017 14:52

a

db54ede

Adding message page, for better content sharing (sending link to a me…

caa71e6

…ssage)

Fixing an issue of not selecting menu item when on message page

0510066

Adding unarchive button to message screen

0f8168e

Changing the methods to support promise.

0ca2996

Adding tests to controller

2150182

fixing tests

255a192

Fixing tests to run in phantomjs

b98ab22

Setting minimum SC version. adding support for events

5fb2115

Supporting the flow of the message

c7ae31d

Adding Mock view for Sergio to test the 1 page message

cdfa1c6

Initial layout of message page

043418c

- restored row hover state in All Failed Messages / Group detail views - updated message detail page

More message type view tweaks

c190348

WojcikMike force-pushed the add_message_page branch from 1347501 to c4e583f Compare February 9, 2017 13:54

WojcikMike added this to the 1.7.2 milestone Feb 9, 2017

sergioc and others added 2 commits February 9, 2017 16:23

tweak msg group action link behavior

17ba1a9

Change the route for the feature to work after William added deep lin…

034f645

…king for failed message groups

sergioc reviewed Feb 9, 2017

View reviewed changes

WojcikMike mentioned this pull request Feb 10, 2017

Improve URL structure #402

Closed

WilliamBZA requested changes Feb 15, 2017

View reviewed changes

WojcikMike added 2 commits February 15, 2017 11:25

Fixing tests to pass

bc7144d

Addressing Williams comments

4d87940

WojcikMike and others added 3 commits February 16, 2017 12:00

Fixing checkbox issue on the archive screen

5867da0

Fixed pending retries list

6a25123

making changes to align pending retries impl with archived and all me…

d9730c1

…ssages screen

Neaten up Service Control promises

0a7903f

WilliamBZA approved these changes Feb 17, 2017

View reviewed changes

WojcikMike merged commit ab73324 into master Feb 20, 2017

WojcikMike deleted the add_message_page branch February 20, 2017 15:17

WojcikMike added Type: Feature Type: Feature Size: M and removed State: In Progress labels Feb 20, 2017

sergioc mentioned this pull request Feb 21, 2017

Show message data in a tabbed modal instead of inline in the list #308

Closed

Adding message page, for better content sharing #381

Adding message page, for better content sharing #381

Conversation

WojcikMike commented Oct 3, 2016 • edited by kbaley Loading

WojcikMike commented Oct 10, 2016

WilliamBZA left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilliamBZA commented Oct 12, 2016

johnsimons commented Oct 28, 2016

WojcikMike commented Oct 31, 2016

WojcikMike commented Jan 31, 2017

WojcikMike commented Feb 9, 2017

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilliamBZA commented Feb 15, 2017

WojcikMike commented Feb 15, 2017

sergioc commented Feb 15, 2017 • edited Loading

WojcikMike commented Feb 16, 2017

WilliamBZA left a comment

Choose a reason for hiding this comment

WilliamBZA commented Feb 17, 2017

WilliamBZA commented Feb 19, 2017

sergioc commented Feb 20, 2017

WojcikMike commented Oct 3, 2016 •

edited by kbaley

Loading

sergioc commented Feb 15, 2017 •

edited

Loading