Issue/2009 Notifications Added #2064
Conversation
RachelDau
changed the title
There was a problem hiding this comment.
Initial thoughts:
You might need to describe the new notification_status table a bit. It's not clear what the user_id, draft_id or status fields are for. It doesn't look like draft_id is made anywhere.
Ideally all available notifications should be objects in an array and handled programmatically instead of explicitly defining notificationText and notificationTitle to handle only one notification at a time.
If the idea for notifications is that we aren't tracking which ones have been seen by which users, then the three fields mentioned above shouldn't be necessary. In order to preserve notifications between page loads, look into adding them to a cookie (see here for an example of using cookies) and changing the utilities to adjust what's saved in the cookies.
More specific comments per file below.
| .then(() => viewerUsersState.getLastLogin(userId)) | ||
| .then(result => { | ||
| if (result) lastLogin = result.last_login | ||
| }) |
There was a problem hiding this comment.
Ideally req.currentUser would just have a lastLogin property that you could check, rather than creating a whole new function to get that information separately.
| .then(() => viewerNotificationState.getId(lastLogin)) | ||
| .then(result => { | ||
| if (result) id = result.id | ||
| }) | ||
| .then(() => viewerNotificationState.getStatus(id)) | ||
| .then(result => { | ||
| if (result) isNotificationEnabled = result.status | ||
| }) | ||
| .then(() => viewerNotificationState.getTitle(id)) | ||
| .then(result => { | ||
| if (result) notificationTitle = result.title | ||
| }) | ||
| .then(() => viewerNotificationState.getText(id)) | ||
| .then(result => { | ||
| if (result) notificationText = result.text | ||
| }) |
There was a problem hiding this comment.
Try to provide all valid notifications to the frontend in a single array rather than going through multiple steps to grab individual pieces of a single notification like this.
| isNotificationEnabled, | ||
| notificationTitle, | ||
| notificationText |
There was a problem hiding this comment.
See above comment: rather than informing the frontend of a single notification with three new properties, try to provide all valid notifications as objects in a single array.
| function getStatus(id) { | ||
| return db.oneOrNone( | ||
| ` | ||
| SELECT status FROM notification_status | ||
| WHERE | ||
| id = $[id] | ||
| `, | ||
| { | ||
| id | ||
| } | ||
| ) | ||
| } | ||
| function getTitle(id) { | ||
| return db.oneOrNone( | ||
| ` | ||
| SELECT title FROM notification_status | ||
| WHERE | ||
| id = $[id] | ||
| `, | ||
| { | ||
| id | ||
| } | ||
| ) | ||
| } | ||
| function getText(id) { | ||
| return db.oneOrNone( | ||
| ` | ||
| SELECT text FROM notification_status | ||
| WHERE | ||
| id = $[id] | ||
| `, | ||
| { | ||
| id | ||
| } | ||
| ) | ||
| } |
There was a problem hiding this comment.
Don't create multiple functions for each individual field, just have a single getNotifications(ids) function to grab multiple notifications - pass it an array (could even be an array containing a single number) and handle results accordingly.
| function getId(lastLoginDate) { | ||
| return db.oneOrNone( | ||
| ` | ||
| SELECT id FROM notification_status | ||
| WHERE created_at >= $[lastLoginDate] | ||
| LIMIT 1 | ||
| `, | ||
| { | ||
| lastLoginDate | ||
| } | ||
| ) | ||
| } |
There was a problem hiding this comment.
Several things here:
- Change the name from
getIdto something more descriptive likegetRecentNotifications. - Generalize
lastLoginDatetotargetDateor maybe even justdate. - Use
manyOrNoneinstead ofoneOrNoneand remove theLIMITso this function grabs all notifications after a given date. - Add an
ORDER BY created_at ASCso notifications always appear in a predictable order.
| const db = oboRequire('server/db') | ||
| //const logger = oboRequire('server/logger') | ||
| /* | ||
| function setLastLogin(userId){ | ||
| const date = new Date(); | ||
| const today = date.getDate(); | ||
| return db | ||
| .none( | ||
| ` | ||
| INSERT INTO users | ||
| (id, last_login) | ||
| VALUES($[userId], $[today]) | ||
| ON CONFLICT (id) DO UPDATE | ||
| SET last_login = $[today] | ||
| WHERE id = $[userId] | ||
| `, | ||
| { | ||
| userId, | ||
| today | ||
| } | ||
| ) | ||
| .catch(error => { | ||
| logger.error('DB UNEXPECTED on users.set', error, error.toString()) | ||
| }) | ||
| } */ | ||
| function getLastLogin(userId) { | ||
| return db.oneOrNone( | ||
| ` | ||
| SELECT last_login FROM users | ||
| WHERE id = $[userId] | ||
| `, | ||
| { | ||
| userId | ||
| } | ||
| ) | ||
| } | ||
|
|
||
| module.exports = { | ||
| //setLastLogin, | ||
| getLastLogin | ||
| } |
There was a problem hiding this comment.
See if it's possible to attach the last login date to req.currentUser instead of creating a separate function like this to look it up separately.
| } | ||
|
|
||
| exports.down = function(db) { | ||
| return null |
There was a problem hiding this comment.
The down of a migration is the opposite of the up. In this case, the down of this migration should remove the last_login column.
|
While previously mentioned issues have been addressed current known issues now include:
|
|
merged with latest dev branch to solidify new node version , no notifications changes were included in the merge. |
RachelDau
changed the base branch from
dev/32-pigeonite
to
dev/33-serpierite
|
Taking into account all previous feedback, notifications are now in a pop-up with some style changes. Some CSS styles were reorganized and jest tested added for the new notification indicator. It now tracks the number of notifications present in a callback function from the notification component to repository-nav. When exiting the indicator updates immediately. As usually, after migrating the db up to create the notification table type directly into it to add notifications with a id, title, and text. Any and all feedback is appreciated :) |
There was a problem hiding this comment.
This is looking pretty good.
The main things I can see that still need addressing are tab reachability and click handlers. Currently the notifications indicator is not tab-reachable, so it can only be activated by clicking it directly. When the notifications are visible, only the 'close' buttons are tab-reachable so there's currently no way of screen readers reaching the text content.
It looks like clicking anywhere except one of the notification's 'X' buttons dismisses the notifications popup entirely. Even clicking inside one of the notifications.
packages/app/obojobo-repository/shared/components/repository-nav.jsx
Outdated
Show resolved
Hide resolved
|
Updated to include notification indicator in tab order and make notifications tab keyboard accessible. Clicking inside the notification no longer exits the pop-up. |
|
Updated the screen for when all notifications are exited (style update) |
|
These are ready for review In order to test locally, you must migrate the db up to create the new notification table and last login in the users table. yarn db:migrateup You then must type directly in the database notification table the content for the notification (id, title, and text). Let me know if you need help. Notification will be taken directly from the database, written to the cookie, pulled out of the cookie on the front-end side, and rendered from the dashboard repository nav file. If notifications are present (typed into Database) upon page refresh a number of notifications text indicator will appear under the username when on a dashboard page. When you click the indicator or the username area it will open a notifications pop-up. From here you can see and exit notifications which removes them from the cookie. If you input new notification into the database, the previous ones will disappear so they must be done together if you want them to appear together. (they will also disappear on re-login) You cannot use certain symbols such as ; in the cookie title or text and it is processed as a string and this symbol will mess up how the code processes the notifications from the cookie. Also if the title is one word thats more than 37 characters in length that may be an issue. |
There was a problem hiding this comment.
Sorry this has taken me so long to review.
This has shaped up really well, the only big thing I can see that needs doing now is controlling tab flow. You'll notice that other modal popups (the Module Options window, the user search sub-window in the Share dialog, etc.) only allow the tab key to reach elements that are within the currently open modal dialog window.
I'm not sure exactly how this is being accomplished, but I have a feeling it's a result of using the ReactModal component (see how it's used in the Dashboard component).
If you can figure out how to get the notifications working within ReactModal, you may not have to do as much tab index management yourself.
Also, I feel like we could potentially sanitize cookie string characters such as ;, but avoiding them should be easy enough. Addressing reserved/unsafe characters can be a future issue.
packages/app/obojobo-repository/shared/components/notification.jsx
Outdated
Show resolved
Hide resolved
packages/app/obojobo-repository/shared/components/notification.jsx
Outdated
Show resolved
Hide resolved
|
Alright I added React modal so that the notifications window is tab accessible and you can't tab back to the main page when accessing. The text and title seem to still need the tabIndex on them to be tab accessible. One thing to note is now clicking outside of the notification window will also close it as this is a sort of default when using modal. Adding modal caused a lot of issues with other tests and so a variety of files that use the repository-nav component and test files for those files needed appropriate mocks of react modal added in order to continue functioning. A few Dashboard tests needed a slight rewrite because they hinged on their being one modal and now there are two with notifications so the tests were modified to ensure that is reflected. |
There was a problem hiding this comment.
This is nearly ready to go - just a few very minor adjustments to unit tests and one correction to one of the new migrations.
I think the preferred tab order per notification would go title -> message -> close button, but that would require adjusting the order in which the elements are rendered and probably corresponding changes to the styles to make things show up properly. It can wait to be a follow-up issue, I think.
| } | ||
|
|
||
| exports.down = function(db) { | ||
| return db.removeColumn('last_login') |
There was a problem hiding this comment.
db.removeColumn takes two arguments, the first being the table name.
This should read return db.removeColumn('users', 'last_login').
packages/app/obojobo-repository/shared/components/notification.test.js
Outdated
Show resolved
Hide resolved
| tree = component.toJSON() | ||
| expect(tree).toMatchSnapshot() | ||
| expect(document.cookie).toBe( | ||
| 'notifications=%5B%7B%22key%22%3A2%2C%22text%22%3A%22Notification2%22%2C%22title%22%3A%22Title2%22%7D%5D;' |
There was a problem hiding this comment.
See above comment re: variables and encodeURIComponent.
| expect(tree).toMatchSnapshot() | ||
|
|
||
| if (document && document.cookie) { | ||
| //don't get here |
There was a problem hiding this comment.
We could probably have some expects above this to check that document has a value but document.cookie does not, rather than this if/else.
| cookiePropsRaw.forEach(c => { | ||
| const parts = c.trim().split('=') | ||
|
|
||
| if (parts[0] === 'notifications') { |
There was a problem hiding this comment.
See previous comment re: using expect to fail early instead of relying on an if/else to check for success.
|
I tested migrations up and down and it all appears to be good now. I also regenerated migration files so they are new. Unit test changes were made to reflect using expect and not if else statements. |
| const key = 0 | ||
| const exitButtons = component.root.findAllByProps({ className: 'notification-exit-button' }) | ||
|
|
||
| if (exitButtons[key]) { |
There was a problem hiding this comment.
Sorry if I didn't comment on or notice this before. Feels like I keep sending you back for different things every time.
Is there any scenario where this test should run successfully and exitButtons[key] would evaluate falsy, or if exitButtons[key] is falsy does that mean something went wrong?
Based on the findAllByProps line above I would expect a case where exitButtons[key] not resolving to something truthy to indicate that there was a problem. In a case like this you could use https://jestjs.io/docs/expect#tobetruthy, or forego the key variable altogether and expect(exitButtons.length).toBe(0) or even expect(exitButtons.length > 0).toBe(true).
| tree = component.toJSON() | ||
| expect(tree).toMatchSnapshot() | ||
|
|
||
| if (elementToExit) { |
There was a problem hiding this comment.
This is another case where you should use expect rather than if. Ideally you should populate elementToExit and then have an immediate expect to make sure it's what you expect it to be, at which point this if would be unnecessary because you've already made sure it exists.
There was a problem hiding this comment.
yes, removing the if worked perfectly. Seems it was redundant to check if it existed and then expect it to exist and be undefined in value
| const tree = component.toJSON() | ||
| expect(tree).toMatchSnapshot() | ||
|
|
||
| if (document && document.cookie) { |
There was a problem hiding this comment.
Since this test is only being run one time and should only be testing one thing, there should probably not be an if...else here to determine which expectations are run. In this case you could replace this if...else with two expectations:
expect(document).toBeTruthy() and expect(document.cookie).toBeTruthy(). At that point you can reasonably assume that only the if block would ever run, so you wouldn't need the else block or the if...else at all.
There was a problem hiding this comment.
oops i totally missed this but rewrote the very similar tests yesterday. Fixing now
|
Alright, I removed the if statements from testing as they weren't needed and added more expects to ensure things are tested. When testing make sure to test how it handles no notifications and an empty table. It was working when I tested it I just have a weird fear about that breaking everything. |
|
|
|
Hmm. The values of Quick Googling brings up this SO article discussing how to mock the constructor for |
|
My latest commit sets a mock timer to a specific date and using that set date to reference and test, eliminating any dependency on the execution time. Thanks for the link it helped! |
For issue 2009 adds notification to obojobo. Right now these display in the Nav section of a module. You must physically type the information into the database
notifications. It then uses the last login date and compares it to the notification created_at time to determine if the notification should be displayed and which one should be displayed.Notification will be taken directly from the database, written to the cookie, pulled out of the cookie on the front-end side, and rendered from the dashboard repository nav file. If notifications are present (typed into Database) upon page refresh a red circle indicator will appear by the username when on a dashboard page. When you click the indicator or the username area it will toggle a notifications drop down. From here you can see and exit notifications which removes them from the cookie. If you input new notification into the database, the previous ones will disappear so they must be done together if you want them to appear together. You cannot use certain symbols such as
;in the cookie title or text and it is processed as a string and this symbol will mess up how the code processes the notifications from the cookie.