Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 109 additions & 0 deletions app/javascript/chat/__tests__/util.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
import { getUserDataAndCsrfToken } from '../util';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a note that these tests won't appear in the coverage report for the moment. As soon as the chat folder is within the src folder, they'll appear in coverage reports. This will happen in another PR.


const ERROR_MESSAGE = "Couldn't find user data on page.";

describe('Chat utilities', () => {
describe('getUserDataAndCsrfToken', () => {
afterEach(() => {
document.head.innerHTML = '';
document.body.removeAttribute('data-user');
});

test('should reject if no user or csrf token found.', async () => {
await expect(getUserDataAndCsrfToken(document)).rejects.toThrow(
ERROR_MESSAGE,
);
});

test('should reject if csrf token found but no user.', async () => {
document.head.innerHTML =
'<meta name="csrf-token" content="some-csrf-token" />';
await expect(getUserDataAndCsrfToken(document)).rejects.toThrow(
ERROR_MESSAGE,
);
});

test('should reject if user found but no csrf token found.', async () => {
document.body.setAttribute('data-user', '{}');
await expect(getUserDataAndCsrfToken(document)).rejects.toThrow(
ERROR_MESSAGE,
);
});

test('should resolve if user and csrf token found.', () => {
const csrfToken = 'some-csrf-token';
const currentUser = {
id: 41,
name: 'Guy Fieri',
username: 'guyfieri',
profile_image_90:
'/uploads/user/profile_image/41/0841dbe2-208c-4daa-b498-b2f01f3d37b2.png',
followed_tag_names: [],
followed_tags: '[]',
followed_user_ids: [
2,
31,
7,
38,
22,
37,
14,
26,
20,
24,
11,
27,
29,
3,
6,
28,
4,
39,
8,
40,
25,
30,
35,
34,
5,
12,
33,
36,
21,
18,
23,
1,
32,
19,
15,
13,
16,
9,
10,
17,
],
reading_list_ids: [48, 49, 34, 51, 64, 56],
saw_onboarding: true,
onboarding_checklist: {
write_your_first_article: false,
follow_your_first_tag: false,
fill_out_your_profile: false,
leave_your_first_reaction: true,
follow_your_first_dev: true,
leave_your_first_comment: false,
},
checked_code_of_conduct: false,
number_of_comments: 0,
display_sponsors: true,
trusted: false,
};
document.head.innerHTML = `<meta name="csrf-token" content="${csrfToken}" />`;
document.body.setAttribute('data-user', JSON.stringify(currentUser));

expect(getUserDataAndCsrfToken(document)).resolves.toEqual({
currentUser,
csrfToken,
});
});
});
});
97 changes: 52 additions & 45 deletions app/javascript/chat/util.js
Original file line number Diff line number Diff line change
@@ -1,35 +1,40 @@
import 'intersection-observer';
import { sendKeys } from './actions';

export function getCsrfToken() {
const element = document.querySelector(`meta[name='csrf-token']`);

return element !== null ? element.content : undefined;
}

const getWaitOnUserDataHandler = ({ resolve, reject, waitTime = 20 }) => {
let totalTimeWaiting = 0;

return function waitingOnUserData() {
if (totalTimeWaiting === 3000) {
reject(new Error("Couldn't find user data on page."));
return;
}

const csrfToken = getCsrfToken(document);
const { user } = document.body.dataset;

if (user && csrfToken !== undefined) {
const currentUser = JSON.parse(user);

resolve({ currentUser, csrfToken });
return;
}

totalTimeWaiting += waitTime;
setTimeout(waitingOnUserData, waitTime);
Copy link
Contributor Author

@nickytonline nickytonline Aug 26, 2018

Choose a reason for hiding this comment

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

The previous implementation was using setInterval which could have bogged things down as setInterval just fires after n ms. regardless of whether or not what's in it is completed. With setTimeout things can run within it and once they're complete it can be called again. Having said that, this still doesn't seem like the best approach, but the current implementation is looking for DOM attributes being updated.

MutationObserver is an option as you can configure it to only listen for specific attributes' changes, but I think there is probably a better approach than setting these values on DOM attributes. However, since this PR was initally just to rename and move some files, it seems out of scope of this PR.

};
};

export function getUserDataAndCsrfToken() {
const promise = new Promise((resolve, reject) => {
let i = 0;
const waitingOnUserData = setInterval(() => {
let userData = null;
const dataUserAttribute = document.body.getAttribute('data-user');
const meta = document.querySelector("meta[name='csrf-token']");
if (
dataUserAttribute &&
dataUserAttribute !== 'undefined' &&
dataUserAttribute !== undefined &&
meta &&
meta.content !== 'undefined' &&
meta.content !== undefined
) {
userData = JSON.parse(dataUserAttribute);
}
i += 1;
if (userData) {
clearInterval(waitingOnUserData);
resolve(userData);
} else if (i === 3000) {
clearInterval(waitingOnUserData);
reject(new Error("Couldn't find user data on page."));
}
}, 5);
return new Promise((resolve, reject) => {
getWaitOnUserDataHandler({ resolve, reject })();
});
return promise;
}

export function scrollToBottom() {
Expand Down Expand Up @@ -76,39 +81,41 @@ export function adjustTimestamp(timestamp) {
return time;
}


export function setupNotifications() {
navigator.serviceWorker.ready.then((serviceWorkerRegistration) => {
serviceWorkerRegistration.pushManager.getSubscription()
.then(function(subscription) {
navigator.serviceWorker.ready.then(serviceWorkerRegistration => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

prettier in action here.

serviceWorkerRegistration.pushManager
.getSubscription()
.then(subscription => {
if (subscription) {
return subscription;
}
return serviceWorkerRegistration.pushManager.subscribe({
userVisibleOnly: true,
applicationServerKey: window.vapidPublicKey
applicationServerKey: window.vapidPublicKey,
});
}).then(function(subscription) {
sendKeys(subscription.toJSON(), null, null)
})
.then(subscription => {
sendKeys(subscription.toJSON(), null, null);
});
});
}

export function getNotificationState() {
//Not yet ready
if (!window.location.href.includes('ask-for-notifications')){
return "dont-ask"
// Not yet ready
Copy link
Contributor Author

@nickytonline nickytonline Aug 26, 2018

Choose a reason for hiding this comment

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

A little note for whoever comes across this PR, there are only three permissions available. See https://developer.mozilla.org/en-US/docs/Web/API/Notification/permission

if (!window.location.href.includes('ask-for-notifications')) {
return 'dont-ask';
}

// Let's check if the browser supports notifications
if (!("Notification" in window)) {
return "not-supported"
} else if (Notification.permission === "granted") {
if (!('Notification' in window)) {
return 'not-supported';
}

const { permission } = Notification;

if (permission === 'granted') {
setupNotifications();
return "granted"
} else if (Notification.permission !== 'denied') {
return "waiting-permission"
} else {
return "denied"
}
}

return permission === 'default' ? 'waiting-permission' : permission;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since there are only three possible permission types, https://developer.mozilla.org/en-US/docs/Web/API/Notification/permission, I'm assuming when it's 'default' @benhalpern, you wish it to be set to 'waiting-permission'?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah.

}
49 changes: 49 additions & 0 deletions app/javascript/packs/Chat.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import { h, render } from 'preact';
import { getUserDataAndCsrfToken } from '../chat/util';
import Chat from '../chat/chat';

function initializeChat(loadChat) {
getUserDataAndCsrfToken()
.then(loadChat)
.catch(error => {
// eslint-disable-next-line no-console
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned elsewhere, client-side error reporting should be put in place.

console.error('Unable to load chat', error);
});
}

function renderChat(root) {
render(<Chat {...root.dataset} />, root, root.firstChild);
}

const loadChat = ({ currentUser, csrfToken }) => {
const root = document.getElementById('chat');

if (!root) {
return;
}

window.currentUser = currentUser;
window.csrfToken = csrfToken;

const placeholder = document.getElementById('chat_placeholder');

renderChat(root);

if (placeholder) {
root.removeChild(placeholder);
}

window.InstantClick.on('change', () => {
if (window.location.href.indexOf('/connect') === -1) {
return;
}

renderChat(root);
});
};

initializeChat(loadChat);

window.InstantClick.on('change', () => {
initializeChat(loadChat);
});
48 changes: 48 additions & 0 deletions app/javascript/packs/Onboarding.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import { h, render } from 'preact';
import { getUserDataAndCsrfToken } from '../chat/util';
import getUnopenedChannels from '../src/utils/getUnopenedChannels';

HTMLDocument.prototype.ready = new Promise(resolve => {
if (document.readyState !== 'loading') {
return resolve();
}
document.addEventListener('DOMContentLoaded', () => resolve());
return null;
});

function isUserSignedIn() {
return (
document.head.querySelector(
'meta[name="user-signed-in"][content="true"]',
) !== null
);
}

function renderPage() {
import('../src/Onboarding')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benhalpern, you'll like this performance tweak. We only load onboarding if the user is signed in and hasn't previously seen the onboarding. Before, the component was imported all the time.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do. I'm also excited to improve on several fronts here.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is great!

.then(({ default: Onboarding }) =>
render(<Onboarding />, document.getElementById('top-bar')),
)
.catch(error => {
// eslint-disable-next-line no-console
console.error('Unable to load onboarding', error);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see that there is airbrake for the back-end, but from what I can tell, we don't use any client-side error reporting services. Consider a package for handling client-side errors as we'll creating more and more in the front-end now.

});
}

document.ready.then(
getUserDataAndCsrfToken()
.then(({ currentUser, csrfToken }) => {
window.currentUser = currentUser;
window.csrfToken = csrfToken;

getUnopenedChannels();

if (isUserSignedIn() && !currentUser.saw_onboarding) {
renderPage();
}
})
.catch(error => {
// eslint-disable-next-line no-console
console.error('Error getting user and CSRF Token', error);
}),
);
16 changes: 7 additions & 9 deletions app/javascript/packs/articleForm.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,17 @@ HTMLDocument.prototype.ready = new Promise(resolve => {
});

document.ready.then(
getUserDataAndCsrfToken().then(currentUser => {
getUserDataAndCsrfToken().then(({ currentUser, csrfToken }) => {
window.currentUser = currentUser;
window.csrfToken = csrfToken;

const root = document.getElementById('article-form');
window.csrfToken = document.querySelector(
"meta[name='csrf-token']",
).content;
const { article, organization } = root.dataset;

render(
<ArticleForm
article={document.getElementById('article-form').dataset.article}
organization={document.getElementById('article-form').dataset.organization}
/>,
<ArticleForm article={article} organization={organization} />,
root,
root.firstElementChild
root.firstElementChild,
);
}),
);
Loading