Add events to application pages #602

bafonins · 2019-05-01T13:40:46Z

Summary

This PR adds events to the application management pages.
References: #28

Changes

Add redux primitives for events
Add the <EventsSubscriptions /> component
Add the <ApplicationEvents /> container component
Add <Events.Widget /> to the Application Overview Page
Add the Application Data page with the <Events /> component
Move the paused state handling to the <Events /> component

Notes for Reviewers

After working on this one, it turned out that keeping the paused state in the <Events /> component seems to be a better option than outsourcing it redux. Mainly, because we want to pause events only in the full component, but not in the widget.
Currently, there is not error handling. @kschiffer how should it look like?

Release Notes

Added events to the application management pages.

coveralls · 2019-05-01T13:48:55Z

Coverage increased (+0.05%) to 73.692% when pulling 9bcd6e4 on feature/28-webui-events-applications into aaa2dd5 on master.

kschiffer

Very good!

Currently, there is not error handling. @kschiffer how should it look like?

This still needs to be discussed and decided here. We can then create some actionable issues from there.

A couple of overall remarks:

I think it would be good if clicking anywhere on the widget will take you to the data view
Can you add a small bottom margin in the application-data view? The event component now touches the footer.
I'm still not very fond of the level of abstraction in our store logic, which has now increased even more. It became quite hard (for me) to review as it is uncommented and has a partly quite misleading variable naming. Should be discussed in a backlog issue.

kschiffer · 2019-05-02T09:19:02Z

pkg/webui/components/events/index.js

+      return true
+    }
+
+    // do not rerender component on new events if it in the `paused` state


Suggested change

// do not rerender component on new events if it in the `paused` state

// do not rerender component on new events if it is in the `paused` state

kschiffer · 2019-05-02T09:24:34Z

pkg/webui/components/events/index.js

+      limit,
+    } = this.props
+
+    if (


Not sure if we do ourselves a favor with this if statement. We would need to update this for every prop/state structure change we do. Wouldn't it be better to assume rerenders by default and only rule out cases (like below with the paused state)?

We would need to update this for every prop/state structure change we do

If anyone changes the internals of the component then the person should also check the shouldComponentUpdate logic. This does influence the api of the component only the logic within the component.

kschiffer · 2019-05-02T09:59:33Z

pkg/webui/console/store/actions/events.js

+)
+
+export const createGetEventMessageFailureActionType = name => (
+  `GET_${name}_EVENT_MESSAGE_SUCCESS`


Suggested change

`GET_${name}_EVENT_MESSAGE_SUCCESS`

`GET_${name}_EVENT_MESSAGE_FAILURE`

kschiffer · 2019-05-02T09:59:44Z

pkg/webui/console/store/actions/events.js

+)
+
+export const createStartEventsStreamFailureActionType = name => (
+  `START_${name}_EVENT_STREAM_SUCCESS`


Suggested change

`START_${name}_EVENT_STREAM_SUCCESS`

`START_${name}_EVENT_STREAM_FAILURE`

kschiffer · 2019-05-02T10:01:19Z

pkg/webui/console/store/middleware/events.js

+  const START_EVENTS_FAILURE = createStartEventsStreamFailureActionType(name)
+  const STOP_EVENTS = createStopEventsStreamActionType(name)
+  const startEventsSuccess = startEventsStreamSuccess(name)
+  const startEventsFailrue = startEventsStreamFailure(name)


Suggested change

const startEventsFailrue = startEventsStreamFailure(name)

const startEventsFailure = startEventsStreamFailure(name)

kschiffer · 2019-05-02T10:01:45Z

pkg/webui/console/store/middleware/events.js

+          channel.on('error', error => dispatch(getEventFailure(id, error)))
+          channel.on('close', () => dispatch(stopEvents(id)))
+        } catch (error) {
+          dispatch(startEventsFailrue(error))


Suggested change

dispatch(startEventsFailrue(error))

dispatch(startEventsFailure(error))

kschiffer · 2019-05-02T10:12:37Z

pkg/webui/console/store/reducers/events.js

+  createClearEventsActionType,
+} from '../actions/events'
+
+const defualtState = {


Suggested change

const defualtState = {

const defaultState = {

kschiffer · 2019-05-02T10:13:06Z

pkg/webui/console/store/reducers/events.js

+  const GET_EVENT_FAILURE = createGetEventMessageFailureActionType(reducerName)
+  const CLEAR_EVENTS = createClearEventsActionType(reducerName)
+
+  return function (state = defualtState, action) {


Suggested change

return function (state = defualtState, action) {

return function (state = defaultState, action) {

kschiffer · 2019-05-02T10:21:04Z

pkg/webui/console/store/selectors/events.js

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+const storeSelector = (state, props) => state[props.id]


It really took me a while to figure out what these are. selectors clashes with our term for selectors in the JS SDK. I would really prefer the name mappers, which is more in line with their usage in mapStateToProps().

no, a selector is a well known term in the redux world, not a mapper.

kschiffer · 2019-05-02T10:35:28Z

pkg/webui/console/views/application-data/index.js

+  render () {
+    const { appId } = this.props.match.params
+
+    return (


Please add a <ReactHelmet /> entry for the view. In general, a lot of application pages still lack these. Can you follow up with an issue?

Never mind, I'll create an issue for this.

bafonins · 2019-05-02T11:00:33Z

I think it would be good if clicking anywhere on the widget will take you to the data view

What is the purpose of the See All Activity link then?

kschiffer · 2019-05-02T11:09:06Z

What is the purpose of the See All Activity link then?

To give a visual clue towards the data view with the full event component.

bafonins · 2019-05-02T11:42:41Z

I'm still not very fond of the level of abstraction in our store logic, which has now increased even more. It became quite hard (for me) to review as it is uncommented and has a partly quite misleading variable naming. Should be discussed in a backlog issue.

reducers/middleware/action creators are just functions, so we can write tests for them

bafonins added the c/console This is related to the Console label May 1, 2019

bafonins added this to the May 2019 milestone May 1, 2019

bafonins requested a review from kschiffer May 1, 2019 13:40

bafonins self-assigned this May 1, 2019

kschiffer requested changes May 2, 2019

View reviewed changes

bafonins added 11 commits May 2, 2019 13:41

console: Add connection status constants

b123158

console: Add events subsription for application

76c160a

console: Move paused state to events

7a15d97

console: Add redux primitives for events

0bef97c

console: Add redux primitives for app events

9f2aa12

console: Add events subscription component

04669bb

console: Add application events component

41d4d15

console: Add application data page

8b27c94

console: Add events widget to app overview

826d1b0

console: Change events widget header text

8c982de

dev: Update locales

9bcd6e4

bafonins force-pushed the feature/28-webui-events-applications branch from cc45c90 to 9bcd6e4 Compare May 2, 2019 11:46

bafonins requested a review from kschiffer May 2, 2019 11:46

kschiffer approved these changes May 2, 2019

View reviewed changes

bafonins merged commit 3097538 into master May 2, 2019

bafonins deleted the feature/28-webui-events-applications branch May 2, 2019 12:12

bafonins mentioned this pull request May 24, 2019

Event streaming for gateways #745

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add events to application pages #602

Add events to application pages #602

bafonins commented May 1, 2019 •

edited

Loading

coveralls commented May 1, 2019 •

edited

Loading

kschiffer left a comment

kschiffer May 2, 2019

kschiffer May 2, 2019

bafonins May 2, 2019

kschiffer May 2, 2019

kschiffer May 2, 2019

kschiffer May 2, 2019

kschiffer May 2, 2019

kschiffer May 2, 2019

kschiffer May 2, 2019

kschiffer May 2, 2019

bafonins May 2, 2019

kschiffer May 2, 2019

kschiffer May 2, 2019

bafonins commented May 2, 2019

kschiffer commented May 2, 2019 •

edited

Loading

bafonins commented May 2, 2019

	// do not rerender component on new events if it in the `paused` state
	// do not rerender component on new events if it is in the `paused` state

	`GET_${name}_EVENT_MESSAGE_SUCCESS`
	`GET_${name}_EVENT_MESSAGE_FAILURE`

	`START_${name}_EVENT_STREAM_SUCCESS`
	`START_${name}_EVENT_STREAM_FAILURE`

	const startEventsFailrue = startEventsStreamFailure(name)
	const startEventsFailure = startEventsStreamFailure(name)

	dispatch(startEventsFailrue(error))
	dispatch(startEventsFailure(error))

	return function (state = defualtState, action) {
	return function (state = defaultState, action) {

Add events to application pages #602

Add events to application pages #602

Conversation

bafonins commented May 1, 2019 • edited Loading

Summary

Changes

Notes for Reviewers

Release Notes

coveralls commented May 1, 2019 • edited Loading

kschiffer 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

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

bafonins commented May 2, 2019

kschiffer commented May 2, 2019 • edited Loading

bafonins commented May 2, 2019

bafonins commented May 1, 2019 •

edited

Loading

coveralls commented May 1, 2019 •

edited

Loading

kschiffer commented May 2, 2019 •

edited

Loading