Refs #26729 - replace notifications polling by middleware #7121

Ron-Lavi · 2019-10-22T16:54:46Z

blocked by #7157

theforeman-bot · 2019-10-22T16:54:58Z

Issues: #26729

MariaAga · 2019-10-23T08:27:08Z

@LaViro tests in Notifications are failing, is it because this is a WIP?

Ron-Lavi · 2019-10-23T08:46:46Z

Yes, still work in progress @MariaAga thanks

coveralls · 2019-10-24T14:15:19Z

Coverage decreased (-0.2%) to 75.254% when pulling f057285 on laviro:replace_notifications_polling into 5e0496a on theforeman:develop.

jeremylenz

Great work @LaViro! Tested and seems to work as intended. I really like the way you've implemented it, by adding the START_INTERVAL and STOP_INTERVAL actions as basic "infrastructure," but then simplifying things for the developer's most common use case by adding the polling key that just does it for you. And that way, the documentation can be opinionated and thus easy to follow. I left some suggestions & nitpicks below.

webpack/assets/javascripts/react_app/components/notifications/index.js

webpack/assets/javascripts/react_app/components/notifications/notifications.test.js

webpack/assets/javascripts/react_app/redux/API/APIMiddleware.js

jeremylenz · 2019-11-19T16:17:31Z

webpack/assets/javascripts/react_app/redux/actions/notifications/index.js

-  });
-  dispatch(getNotifications(url));
-};
+export const startNotificationsPolling = url => ({


Looking at the diff of this file and loving how much simpler the middleware solution makes it ❤️

webpack/assets/javascripts/react_app/redux/middlewares/IntervalMiddleware/IntervalActions.js

webpack/assets/javascripts/react_app/redux/reducers/notifications/index.js

webpack/stories/docs/APIPollingMiddleware.md

webpack/stories/docs/intervalMiddleware.md

Ron-Lavi · 2019-11-20T18:20:22Z

thanks @jeremylenz for the review !
updated with the code from #7157,
I appreciate if you could review only the last commit as the first commits are always keep updated by #7157 - if you have comments on the code it is best to comment there, but I will update with your suggestions, thanks again 👍

jeremylenz

Reviewed again but didn't really see any new changes except the notifications polling actions, which look fine to me

webpack/assets/javascripts/react_app/redux/actions/notifications/index.js

jeremylenz · 2019-11-20T21:19:52Z

@LaViro Something to consider-- For the review phase, you could temporarily change the PR on Github so it's comparing with your other branch, rather than comparing with master. Then we would only see the relevant changes and there may be less confusion when reviewing.

Ron-Lavi · 2019-11-21T17:35:48Z

Rebased the interval middleware part and it still works the same

amirfefer

Awesome work! removes a lot of redundant code 👍
I left a few inline comments

amirfefer · 2019-12-01T10:32:07Z

webpack/assets/javascripts/react_app/components/notifications/notifications.test.js

-  it('full flow', () => {
-    const wrapper = mount(
-      <Notifications data={componentMountData} store={generateStore()} />
-    );
-    wrapper.find('.fa-bell').simulate('click');
-    expect(wrapper.find('.panel-group')).toHaveLength(1);
-    wrapper.find('.panel-group .panel-heading').simulate('click');
-    expect(wrapper.find('.unread')).toHaveLength(1);
-    wrapper.find('.unread').simulate('click');
-    expect(wrapper.find('.unread')).toHaveLength(0);
  });


don't we need a full flow test in the integration test?

I will add it with the other tests refactor in #7138

webpack/assets/javascripts/react_app/redux/reducers/notifications/index.js

amirfefer · 2019-12-01T10:42:35Z

webpack/assets/javascripts/react_app/redux/reducers/notifications/notifications.test.js

+    expect(reducer(stateWithNotifications, action)).toMatchSnapshot();
+  });
+
+  it('Should handle FAILURE', () => {


How about using testReducerSnapshotWithFixtures from our testsHelper?

originally I thought to add it in #7138, though I see no problem adding it here 👍

it seem a lot of work to update the convention of all tests,
which is a little bit out of the scope of this PR, would it be ok to change it in #7138 ?

mmoll · 2020-01-05T20:57:59Z

[test foreman]

mmoll · 2020-01-05T21:23:43Z

Integration test failures seem related

Ron-Lavi · 2020-01-06T05:49:40Z

Thanks @mmoll, hope I fixed it.
it seems that the API action structure changed in middleware and caused some errors.

ohadlevy

minor comments inline

ohadlevy · 2020-01-06T07:43:59Z

webpack/assets/javascripts/react_app/components/notifications/helpers.js

@@ -0,0 +1,3 @@
+export const redirectToLogin = () => {


nitpick: why is this a notification thing? redirect to login can happen on session timeout regardless of notification (could be triggered by any api call..)

I agree, things like that should be handled by the API middleware,
this was a method which was used before in the notifications component.
I wanted to add this feature as a small PR after #7222 will get merged

why waiting for another PR? this can be moved easily as you mention to our API middleware
does #7222 block this by any means?

opened #7337
it is blocked by #7222 since that PR refactors almost every file of the API middleware,
@amirfefer waiting also for your PR #7186 to use the connected-react-router instead of window.replace

webpack/assets/javascripts/react_app/redux/reducers/notifications/index.js

Ron-Lavi · 2020-01-06T12:50:14Z

@amirfefer can you review again? thanks

amirfefer

Thanks @LaViro ! Looks great 👍
few inline comments

amirfefer · 2020-01-06T17:32:06Z

webpack/assets/javascripts/react_app/components/notifications/helpers.js

@@ -0,0 +1,3 @@
+export const redirectToLogin = () => {


why waiting for another PR? this can be moved easily as you mention to our API middleware
does #7222 block this by any means?

amirfefer · 2020-01-06T17:51:25Z

webpack/assets/javascripts/react_app/components/notifications/notifications.test.js

-    expect(global.location.replace).toBeCalled();
+    const testHelper = configureIntegrationHelper();
+    const wrapper = testHelper.mount(<Notifications {...notificationProps} />);
+    setTimeout(() => {


does it necessary? there are some other async testing techniques than using setTimeOut, i.e for timers jest has useFakeTimers

Thanks, I tried to use the different jest mock timers but it seem to destroy also the API response so sadly the notification component doesn't get updated..

The setTimeout solution seem to work and there is no regression on the testing time period.

Please feel free to try and adjust this test and see what I mean

webpack/assets/javascripts/react_app/redux/actions/notifications/index.js

webpack/assets/javascripts/react_app/redux/actions/notifications/notifications.test.js

Ron-Lavi · 2020-01-08T10:44:48Z

rebased

mmoll · 2020-01-09T15:15:57Z

[test katello]

amirfefer

Thanks @LaViro ! Tested and works as expected 👍
just a few nitpicks

amirfefer · 2020-01-13T18:02:07Z

webpack/assets/javascripts/react_app/redux/reducers/notifications/index.js

+    case SUCCESS:
+      return state.merge({
+        notifications: response.notifications,
+        hasUnreadMessages: hasUnreadMessages(response.notifications),
+      });
+    case FAILURE: {
+      return state.set('error', response);
+    }


does it make sense to use API reducer here?

it is a bit tricky, since the notifications are consumed also on other cases in this reducer,
and the provided state doesn't give access to the API reducer, a workaround I can do is to use getState() from the action and use a selector to retrieve the notifications data.

what do you think about that?

another option is to move the logic to selectors

third option is to wait for all types of API operations to be supported - #7261
and replace all of the notifications actions with API calls.

the API calls will update the API reducer anyway and only than we won't need those cases in the reducer. imo that would be the best option.

👍 to the third option

webpack/assets/javascripts/react_app/components/notifications/notifications.test.js

replace it by using the interval middleware

Ron-Lavi · 2020-01-26T08:36:25Z

ready for another review :)

jeremylenz

Fantastic work @LaViro 😄

Tested again and works as expected. ACK

amirfefer

Thanks @LaViro ! great update 👍

theforeman-bot added Needs testing Not yet reviewed UI labels Oct 22, 2019

Ron-Lavi mentioned this pull request Oct 22, 2019

Fixes #26729 - add polling to API middleware #7069

Closed

Ron-Lavi force-pushed the replace_notifications_polling branch from 7294372 to 6b81cf4 Compare October 24, 2019 14:09

Ron-Lavi force-pushed the replace_notifications_polling branch from 6b81cf4 to 9f1db56 Compare October 28, 2019 17:01

Ron-Lavi mentioned this pull request Oct 29, 2019

Fixes #28144 - update notifications with our modern conventions #7138

Closed

Ron-Lavi force-pushed the replace_notifications_polling branch 3 times, most recently from 64150cd to e7a636a Compare November 7, 2019 12:51

Ron-Lavi changed the title ~~[WIP] Refs #26729 - replace notifications polling by middleware~~ Refs #26729 - replace notifications polling by middleware Nov 10, 2019

Ron-Lavi force-pushed the replace_notifications_polling branch 2 times, most recently from b6e75ac to afaa43c Compare November 14, 2019 10:54

Ron-Lavi force-pushed the replace_notifications_polling branch from afaa43c to b158331 Compare November 18, 2019 08:12

jeremylenz reviewed Nov 19, 2019

View reviewed changes

Ron-Lavi force-pushed the replace_notifications_polling branch from b158331 to 4b29c62 Compare November 20, 2019 18:15

jeremylenz reviewed Nov 20, 2019

View reviewed changes

webpack/assets/javascripts/react_app/redux/actions/notifications/index.js Outdated Show resolved Hide resolved

Ron-Lavi force-pushed the replace_notifications_polling branch from 4b29c62 to 8ed2e42 Compare November 21, 2019 09:35

Ron-Lavi force-pushed the replace_notifications_polling branch from 8ed2e42 to 61b1b69 Compare November 28, 2019 14:16

amirfefer suggested changes Dec 1, 2019

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Dec 1, 2019

Ron-Lavi force-pushed the replace_notifications_polling branch from 61b1b69 to 65de757 Compare December 3, 2019 13:44

mmoll force-pushed the replace_notifications_polling branch from 423068f to f03fe92 Compare January 5, 2020 20:26

Ron-Lavi force-pushed the replace_notifications_polling branch from f03fe92 to c3832e9 Compare January 6, 2020 05:47

ohadlevy reviewed Jan 6, 2020

View reviewed changes

amirfefer suggested changes Jan 6, 2020

View reviewed changes

theforeman-bot added Waiting on contributor Needs re-review and removed Needs re-review Waiting on contributor labels Jan 6, 2020

Ron-Lavi force-pushed the replace_notifications_polling branch from acea028 to a181917 Compare January 8, 2020 10:44

Ron-Lavi force-pushed the replace_notifications_polling branch from a181917 to 0eb500c Compare January 9, 2020 06:01

amirfefer suggested changes Jan 13, 2020

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Jan 13, 2020

Ron-Lavi force-pushed the replace_notifications_polling branch from 0eb500c to f057285 Compare January 15, 2020 09:29

theforeman-bot added Needs re-review and removed Waiting on contributor labels Jan 15, 2020

Refs #26729 - refactor notifications polling

433797d

replace it by using the interval middleware

Ron-Lavi force-pushed the replace_notifications_polling branch from f057285 to 433797d Compare January 26, 2020 08:35

jeremylenz approved these changes Jan 29, 2020

View reviewed changes

theforeman-bot removed the Needs re-review label Jan 29, 2020

amirfefer approved these changes Jan 30, 2020

View reviewed changes

amirfefer merged commit 62a0439 into theforeman:develop Jan 30, 2020

Ron-Lavi deleted the replace_notifications_polling branch February 12, 2020 12:37

Refs #26729 - replace notifications polling by middleware #7121

Refs #26729 - replace notifications polling by middleware #7121

Conversation

Ron-Lavi commented Oct 22, 2019 • edited Loading

theforeman-bot commented Oct 22, 2019

MariaAga commented Oct 23, 2019

Ron-Lavi commented Oct 23, 2019

coveralls commented Oct 24, 2019 • edited Loading

jeremylenz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ron-Lavi commented Nov 20, 2019

jeremylenz left a comment

Choose a reason for hiding this comment

jeremylenz commented Nov 20, 2019

Ron-Lavi commented Nov 21, 2019

amirfefer left a comment

Choose a reason for hiding this comment

amirfefer Dec 1, 2019 • edited Loading

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

mmoll commented Jan 5, 2020

mmoll commented Jan 5, 2020

Ron-Lavi commented Jan 6, 2020

ohadlevy 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

Ron-Lavi commented Jan 6, 2020

amirfefer 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

Ron-Lavi commented Jan 8, 2020

mmoll commented Jan 9, 2020

amirfefer 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

Ron-Lavi commented Jan 26, 2020

jeremylenz left a comment

Choose a reason for hiding this comment

amirfefer left a comment

Choose a reason for hiding this comment

Ron-Lavi commented Oct 22, 2019 •

edited

Loading

coveralls commented Oct 24, 2019 •

edited

Loading

amirfefer Dec 1, 2019 •

edited

Loading