notifications: Adds bots mention support to reply option. #395

abhigyank · 2018-01-25T20:30:02Z

Fixes: #391

What's this PR do?
Adds bots mention support to reply option in notifications.
Any background context you want to provide?
Make a getJSON request to /json/users/ to get list of members and filter out bots using is_bots property.

priyank-p

The feature work correctly here but this needs a bit of code refactoring and the approach or requesting data on each notification is not correct here instead what we want to do is:

Load the bots already outside the parseReply function and asyncronusly.

priyank-p · 2018-01-25T21:37:59Z

app/renderer/js/notification/helpers.js

@@ -83,6 +83,36 @@ function parseReply(reply) {
 		reply = reply.replace(regex, streamMention);
 	});

+	window.$.ajaxSetup({async: false});


Right here blocking the thread using synchronous request is not the right approach here

priyank-p · 2018-01-25T21:38:07Z

app/renderer/js/notification/helpers.js

+		const bots = [];
+		members.forEach(membersRow => {
+			if (membersRow.is_bot) {
+				botsList.push(membersRow);


Right here instead of pushing membersRow here you should push memberRow.full_name and the mention syntax so you do not need to loop through it again.

priyank-p · 2018-01-25T21:59:13Z

app/renderer/js/notification/helpers.js

@@ -83,6 +83,36 @@ function parseReply(reply) {
 		reply = reply.replace(regex, streamMention);
 	});

+	window.$.ajaxSetup({async: false});
+
+	window.$.getJSON('/json/users', data => {


One more minor comment you should deconstruct the $ variable up top.
const { $ } = window;

abhigyank · 2018-01-26T08:32:11Z

@cPhost The problem with loading the bots already outside the parseReply function would be that the list would be static. After the list is generated, if someone creates a new bot, then that bot can't be mentioned in the reply since that list wouldn't have that bot. Hence, despite knowing that blocking the thread using synchronous request is not the a good way, I have done that. I feel we need to get the list from the server every time parseReply is called to get new bots if there are any.

priyank-p · 2018-01-26T11:28:18Z

Do get your point but we don't care if new bot was added or not this is like a extra convince feature we provide people are not likely to be mentioning bots specifically new bots + in a mention so we shouldn't worry about it too much.

abhigyank · 2018-01-26T11:43:22Z

Oh, in that case it won't be a issue. I'll make the changes and push it.

priyank-p

It would be better to call loadsBots instead of repeating the code and in loadBots reintialze the botList array like botsList = [] in callback so no duplicates are being added.

And remove the function out of load event.

priyank-p · 2018-01-26T17:00:03Z

app/renderer/js/notification/helpers.js

+	} else {
+		$.ajaxSetup({async: false});
+
+		$.getJSON('/json/users', data => {


It would be better call loadBots() here instead here.

priyank-p · 2018-01-26T17:02:00Z

app/renderer/js/notification/helpers.js

@@ -92,6 +134,10 @@ function setupReply(id) {
 	narrow.by_subject(id, { trigger: 'notification' });
 }

+window.addEventListener('load', () => {


You should't put this under load because the while purpose of the function is to load bots before hand not after load event where notification could be created.

The problem here is that, before load, $ is not defined.

Oh, yeah i did not think about it you can ignore this then.

@abhigyank, @cPhost works great. I think the load listener should be added in notification/index.js. Can you do that?

priyank-p

@abhigyank this LGTM. requesting final review @akashnimare

akashnimare · 2018-01-29T12:46:32Z

app/renderer/js/notification/helpers.js

+const botsList = [];
+let botsListLoaded = false;
+
+function loadBots(sync = false) {


@abhigyank would be great if you can add few comments here about the function? In general, it's better to document your code so that other devs can see what's going on :)

akashnimare · 2018-01-29T12:46:58Z

app/renderer/js/notification/helpers.js

+		loadBots(true);
+	}
+
+	botsList.forEach(([bot, mention]) => {


here as well.

akashnimare · 2018-01-29T12:47:29Z

app/renderer/js/notification/helpers.js

@@ -92,6 +125,10 @@ function setupReply(id) {
 	narrow.by_subject(id, { trigger: 'notification' });
 }

+window.addEventListener('load', () => {


Put this in /notification/index.js.

abhigyank · 2018-01-29T14:22:54Z

@akashnimare Amended commit with comments at specified places and moved load to index.js . Request review.

akashnimare · 2018-01-29T20:14:25Z

Before merging, I would want you to handle following cases-

if the user is not logged in -

jquery.js?61dd:9566 GET https://chat.zulip.org/json/users 401 ()

In case of muliple servers -

jquery.js?61dd:9566 GET https://playground.zulipdev.org/json/users 401 (Unauthorized)

abhigyank · 2018-01-29T22:37:59Z

@akashnimare The 401() error would be thrown on the console by jQuery. How do you wish to handle these cases?

akashnimare · 2018-01-30T11:22:56Z

For now, we can just log them. Maybe use something like -

function loadBots(sync = false) {
	const { $ } = window;
	botsList.length = 0;
	if (sync) {
		$.ajaxSetup({ async: false });
	}
	$.getJSON('/json/users')
		.done((data) => {
			const members = data.members;
			members.forEach(membersRow => {
				if (membersRow.is_bot) {
					const bot = `@${membersRow.full_name}`;
					const mention = `@**${bot.replace(/^@/, '')}**`;
					botsList.push([bot, mention]);
				}
			});
			botsListLoaded = true;
			if (sync) {
				$.ajaxSetup({ async: true });
			}
		})
		.fail((error) => {
			console.log("Request Failed: ", error.responseText);
			console.log(`Request status: ${error.status + error.statusText}`);
		});
}

(dummy code feel free to refactor)

Fixes: #391

akashnimare · 2018-01-30T19:56:33Z

@abhigyank merged, thanks 🚀

zulipbot added size: M in progress Type: Documentation and removed Type: Documentation in progress labels Jan 25, 2018

priyank-p requested changes Jan 25, 2018

View reviewed changes

priyank-p reviewed Jan 25, 2018

View reviewed changes

priyank-p reviewed Jan 26, 2018

View reviewed changes

priyank-p approved these changes Jan 26, 2018

View reviewed changes

akashnimare requested changes Jan 29, 2018

View reviewed changes

zulipbot added size: L and removed size: M labels Jan 29, 2018

akashnimare approved these changes Jan 29, 2018

View reviewed changes

notifications: Adds bots mention support to reply option.

803bc4e

Fixes: #391

akashnimare merged commit 4a40c75 into zulip:master Jan 30, 2018

abhigyank deleted the bots_reply branch June 26, 2018 05:31

Uh oh!

notifications: Adds bots mention support to reply option. #395

notifications: Adds bots mention support to reply option. #395

Uh oh!

Conversation

abhigyank commented Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

priyank-p left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhigyank commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

priyank-p commented Jan 26, 2018

Uh oh!

abhigyank commented Jan 26, 2018

Uh oh!

priyank-p left a comment

Choose a reason for hiding this comment

Uh oh!

priyank-p Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

priyank-p left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhigyank commented Jan 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akashnimare commented Jan 29, 2018

Uh oh!

abhigyank commented Jan 29, 2018

Uh oh!

akashnimare commented Jan 30, 2018

Uh oh!

akashnimare commented Jan 30, 2018

Uh oh!

Uh oh!

abhigyank commented Jan 25, 2018 •

edited

Loading

abhigyank commented Jan 26, 2018 •

edited

Loading

priyank-p Jan 26, 2018 •

edited

Loading

abhigyank commented Jan 29, 2018 •

edited

Loading