slco-2016 / clientcomm

A communications platform for case managers in the Criminal Justice System.
https://clientcomm.org/
Other
10 stars 3 forks source link

Better behavior for new messages alert #409

Closed tmaybe closed 7 years ago

tmaybe commented 7 years ago

If there are only messages from one client, clicking the alert will take you directly to that client's message stream. If there are messages from more than one client, you're taken to the client list.

Also, the number of unread messages are now part of the text of the alert.

Closes #384

codecov-io commented 7 years ago

Codecov Report

Merging #409 into master will increase coverage by 3.18%. The diff coverage is 88.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #409      +/-   ##
==========================================
+ Coverage   63.78%   66.96%   +3.18%     
==========================================
  Files         112      111       -1     
  Lines        4299     4262      -37     
  Branches      473      460      -13     
==========================================
+ Hits         2742     2854     +112     
+ Misses       1557     1408     -149
Impacted Files Coverage Δ
deploy/chef/cookbook/files/default/credentials.js 76.92% <ø> (-1.65%) :arrow_down:
app/controllers/alerts.js 62% <100%> (+8%) :arrow_up:
seeds/seeds.js 97.7% <100%> (+0.59%) :arrow_up:
app/controllers/access.js 18.91% <50%> (ø) :arrow_up:
app/models/messages.js 60.57% <58.33%> (+15.48%) :arrow_up:
app/lib/sms-status-check.js
app/controllers/voice.js 94.33% <0%> (+2.51%) :arrow_up:
app/lib/models.js 92.39% <0%> (+4.34%) :arrow_up:
app/models/conversations.js 75.4% <0%> (+4.91%) :arrow_up:
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4cfc3bf...7fbe855. Read the comment docs.

glassresistor commented 7 years ago

I think in general the changes look good and am only hesitant if we are slowing down the request and/or adding significant queries,. @tdooner point about hitting the endpoint every 1s is salient. I think we can definitely tune that down to ~5sec or slower for a quick solution.

If we have more time we can work a way to keep count of unread messages in a way thats fast to read regularly.

In terms of testing if we can't test the frontend js then I agree we should have it just take a count for now.

tmaybe commented 7 years ago

Thank you! I think that, while the polling may be inefficient, the changes don't make it more inefficient. Merging now.

tdooner commented 7 years ago

Agreed, and this is still a hypothetical performance problem. The CPU usage on the web servers is negligible, last time I checked.

When we rewrite it in Rails, we can give ActionCable a shot 😀

tmaybe commented 7 years ago

ooh, I was wondering if there was a socket.io for rails :)