Skip to content

task/FP-391: Notifications #77

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 46 commits into from
Jul 30, 2020
Merged

task/FP-391: Notifications #77

merged 46 commits into from
Jul 30, 2020

Conversation

rstijerina
Copy link
Member

@rstijerina rstijerina commented Jun 30, 2020

Overview:

Enables Django Channels on the backend, running as a daphne server for websocket events.

Creates a ReconnectingWebSocket connection on client instantiation. The channels ws connection is added to two groups:

  • the user's username group, for messages to the user
  • the portal_events group, for messages to all users

Credit to @jarosenb for laying the groundwork.

PR Status:

  • Ready.
  • Work in Progress.
  • Hold.

Related Jira tickets:

Summary of Changes:

Testing Steps:

  1. Install updated requirements.txt by rebuilding image:
make build
  1. Install updated packages.json:
cd client && npm ci
npm run dev
  1. Run an ngrok session to route webhooks to frontera_prtl_nginx:
ngrok http 443
  1. Take url generated by ngrok and paste it into the _WH_BASE_URL setting in settings_secret.py
  2. Launch a job and see the History notifications badge increment as the job status changes.

UI Photos:

Screen Shot 2020-07-02 at 4 20 44 PM

Notes:

Todo:

  • If 3 job status updates come through for one job, that increments the badge counter by 1, not 3

@rstijerina rstijerina marked this pull request as draft June 30, 2020 23:39
@codecov
Copy link

codecov bot commented Jun 30, 2020

Codecov Report

Merging #77 into master will decrease coverage by 0.11%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #77      +/-   ##
==========================================
- Coverage   54.08%   53.96%   -0.12%     
==========================================
  Files         211      216       +5     
  Lines        7585     7686     +101     
  Branches     1028     1065      +37     
==========================================
+ Hits         4102     4148      +46     
- Misses       3287     3332      +45     
- Partials      196      206      +10     
Flag Coverage Δ
#javascript 50.62% <48.48%> (-0.52%) ⬇️
#unittests 55.22% <51.25%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
client/src/components/Workbench/Workbench.js 0.00% <0.00%> (ø)
client/src/utils/notifications.js 0.00% <0.00%> (ø)
server/portal/apps/datafiles/views.py 35.89% <ø> (ø)
server/portal/apps/notifications/routing.py 0.00% <0.00%> (ø)
server/portal/apps/signals/signals.py 100.00% <ø> (ø)
server/portal/apps/workspace/tasks.py 0.00% <ø> (ø)
server/portal/asgi.py 0.00% <0.00%> (ø)
server/portal/settings/settings.py 0.00% <0.00%> (ø)
...lient/src/redux/reducers/notifications.reducers.js 7.14% <7.14%> (ø)
server/portal/apps/notifications/views.py 20.83% <11.11%> (-3.66%) ⬇️
... and 21 more

@rstijerina rstijerina force-pushed the task/FP-391 branch 2 times, most recently from faa32e8 to 690277f Compare July 2, 2020 21:18
Copy link
Member

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

This is a UI approval.

@wesleyboar wesleyboar self-requested a review July 22, 2020 22:02
Copy link
Member

@nathanfranklin nathanfranklin left a comment

Choose a reason for hiding this comment

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

Besides #77 (comment) thread, looks good to merge 👍

In the future, would it be useful to change the backend response so that the list contains the total unread count but instead of a single list it contains a separate list (with their unread count) for each of the event types.

Copy link
Member

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

Two suggestions (fixes based on incorrect padding that I believe came from me).

Also, there's a merge conflict to resolve. Probably related to icons. I'll look. Update: PR author is looking.

Copy link
Member

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

Re-approving, after testing changes, and reviewing last minute commits.

Update: Assuming client-side test failure will be fixed. The client-side test failure is fixed.

rstijerina and others added 3 commits July 30, 2020 15:45
* Alter counting of job notifications

* Move calculation of unread messages to reducer

* Fix refactor

* Add missing total

* Fix for which notifications counters are displayed
@rstijerina rstijerina merged commit 142c5c4 into master Jul 30, 2020
@rstijerina rstijerina deleted the task/FP-391 branch July 30, 2020 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants