[WIP] threads review/adjustments #572

ggazzo · 2019-02-13T18:18:58Z

I have opened that PR to show/share the modifications I made to be able to merge on rocketchat core (:

mrsimpson

@ggazzo Great! I don't fully understand why you removed the entry-options or how you're going to replace them, but of course: Feel free to merge it.

mrsimpson · 2019-02-15T11:39:33Z

.meteor/versions

@@ -226,7 +226,6 @@ rocketchat:slackbridge@0.0.1
 rocketchat:slashcommands-archive@0.0.1
 rocketchat:slashcommands-asciiarts@0.0.1
 rocketchat:slashcommands-create@0.0.1
-rocketchat:slashcommands-create-thread@0.0.1


Did you not like the slash command? I use it frequently myself

I will reinsert :)

mrsimpson · 2019-02-15T11:40:17Z

packages/assistify-threading/client/createThreadMessageAction.js

-						});
-					});
+	Tracker.autorun(() => {
+		if (RocketChat.settings.get('Thread_from_context_menu') !== 'button') {


much better... Kind-of-ashamed...

mrsimpson · 2019-02-15T11:41:32Z

packages/rocketchat-ui-sidenav/client/sideNav.html

@@ -2,9 +2,6 @@
 		<aside class="sidebar sidebar--{{sidebarViewMode}} {{#if sidebarHideAvatar}}sidebar--hide-avatar{{/if}}" role="navigation">
 			{{> sidebarHeader }}
 			{{#if loggedInUser}}
-				{{#if threadingFromSidebar}}


I absolutely understand you didn't like that button - I don't like it either. But: Is there a replacement for that? I believe there should be an option to start a thread without opening the room first.

mrsimpson · 2019-02-15T11:42:18Z

packages/rocketchat-ui-sidenav/client/sideNav.js

@@ -68,10 +64,6 @@ Template.sideNav.events({
 	'dropped .sidebar'(e) {
 		return e.preventDefault();
 	},
-
-	'click .js-create-thread'() {
-		return FlowRouter.go('create-thread');


So you left the route, as far as I can see. 👍

ggazzo · 2019-02-19T20:19:40Z

@mrsimpson I'm afraid I didnt finish yet, I realize I removed your "cloud tags", actually I couldnt open/find on https://threads-qa.rocket.chat...

I had to change some "internal things(popover/modal)" to fit the forms inside popups... and some "props names" we are trying to "normalize"... there are some bugs I didnt fix yet(remove parent thread), but I will :)...

mrsimpson · 2019-02-20T07:25:01Z

@mrsimpson I'm afraid I didnt finish yet,

Of course - there's a lot to do if you want to catch all the details. I "approved" it just to signal "if you want to, just merge it any time. Since you are a maintainer, I'll (have to) accept it anyway ;) "

I realize I removed your "cloud tags", actually I couldnt open/find on https://threads-qa.rocket.chat...

Ah, yes. We tried a word cloud for selecting the parent channel. However, this didn't work out as we wanted to, so we thought a normal autocomplete should do as well. I have hidden it from the UI (might be only a display: none), but it's not a big loss if you deleted the wordcloud.

I had to change some "internal things(popover/modal)" to fit the forms inside popups... and some "props names" we are trying to "normalize"

I still think that the DB should be responsible for compression and thus preferred parentRoomId instead of prid (see here and particularly here ), but well - most of the DB prop names are cryptical as well...

there are some bugs I didnt fix yet(remove parent thread), but I will :)...

Cascading delete might be hard to understand in the context of heavy threading, since the rooms are only very loosely coupled. What about an option to keep the threads or at least a warning when deleting a room with threads?

In general, it sounds as if you do have a plan - which is good ;)
Have you seen my open questions in RocketChat#11803 (comment) ?

mrsimpson

@ggazzo I had a look at your ~~re-write~~ enhancements ;)

All looks excellent. I voiced doubts where I have them, just for your consideration.

mrsimpson · 2019-02-20T07:28:46Z

packages/assistify-threading/client/createThreadMessageAction.js

 			},
-			condition({ rid, u: { _id: uid } }) {
+			condition({ rid, u: { _id: uid }, attachments }) {
+				if (attachments && attachments[0].fields && attachments[0].fields[0].type === 'messageCounter') {


Cascading question:
Is this you validation for ensuring the parent room is not a "thread started" message?
If so: Do you want to revert this being a system message?
If so: You'll be able to (and need) to handle the events like updating or deleting the origin message.

mrsimpson · 2019-02-20T07:31:55Z

packages/assistify-threading/client/views/ThreadList.js

-		this.threadSubscriptions.set(ChatSubscription.find(query, { sort }).fetch());
-	});
+		query.prid = { $exists: true };
+		return ChatSubscription.find(query, { sort });


There was a reason why I opted for an own reactive collection - if only I knew what it was, long time ago... Anyway, this is comparatively easy to test

let me know if you remember why you choose that way :)

packages/assistify-threading/client/views/creationDialog/CreateThread.html

packages/assistify-threading/client/views/fieldTypeThreadLastMessageAge.html

packages/assistify-threading/server/hooks/propagateThreadMetadata.js

mrsimpson · 2019-02-20T07:39:50Z

packages/assistify-threading/server/methods/createThread.js

-		this._parentRoom = ThreadBuilder.getRoom(this._parentRoomId);
-		this.rocketCatUser = RocketChat.models.Users.findOneByUsername('rocket.cat');
-	}
+// export class ThreadBuilder {


Feel free to delete this.
We've had quite some evolution on this and various types of threads which is why we implemented a factory.
With the current functionality, this is definitely over-engineered.

mrsimpson · 2019-02-20T07:45:42Z

packages/assistify-threading/server/methods/createThread.js

+	const name = getName({ t_name, p_name: p_room.name, message: message.msg });
+
+	const thread = RocketChat.createRoom(t_type, name, user.username, users, false, {
+		description: message.msg, // TODO threads remove


@ggazzo agreed we should not utilize another field for this. What's important: The initial question needs to visualized in the thread even when scrolling. with a custom header, this should be manageable though.

mrsimpson · 2019-02-20T07:52:29Z

packages/rocketchat-ui/client/views/app/fullModal.js

-import { Template } from 'meteor/templating';
-let oldRoute = '';
-const parent = document.querySelector('.main-content');
+// import { Blaze } from 'meteor/blaze';


Are you going to remove this completely?

mrsimpson · 2019-02-20T07:58:46Z

packages/rocketchat-lib/server/functions/deleteRoom.js

+	RocketChat.models.Messages.removeFilesByRoomId(rid);
+	RocketChat.models.Messages.removeByRoomId(rid);
+	RocketChat.models.Subscriptions.removeByRoomId(rid);
+	RocketChat.callbacks.run('afterDeleteRoom', rid);


What happens if one thread cannot be deleted (e. g. since the owner has changed)? May this result in a situation where you cannot delete the parent room while having it deleted for the biggest part?
You wouldn't even see the parent room anymore if this fails since the subscription will be gone.
What about triggering afterDeleteRoom as first step in the chain?

mrsimpson · 2019-02-20T08:01:23Z

packages/assistify-threading/authorization.js

+	}
+
+	addRoomAccessValidator(function(room, user) {
+		return room.prid && canAccessRoom(Rooms.findOne(room.prid), user);


Ah, nice. I thought about this too. This makes it optional to invite users to the thread, which was a major flaw in the current implementation.

However, we got feedback that some users shall be able to join a thread while not being able to see the parent. Is this possible?

…o threads

…DP Rate Limiter (#13529)

* Move mongo config away from cors package * Remove TLS fix

… login via email/crowd_username (#12981) * Fix to close issue #12979 - Enable custom rocketchat usernames for crowd users - Enable login via rocketchat username, crowd_username and email address - Don't authenticate local users against crowd - Allow/Disallow resyncing/overwride of local usernames with crowd usernames in crowd options - Consistent user sync on login and on cron sync * Fix to close issue #12979 Integrate PR suggestions/improvements * Change mongodb queries * Access settings directly via imported settings object

#13388)

ThomasRoehl · 2019-03-26T09:59:03Z

comes with RC 1.0

ggazzo added 5 commits February 13, 2019 16:02

removed sidenav button

46f65b4

removed slashcommands

6da53aa

create thread message action function improved

0fe376f

thread from message box method refactor

c2f7fbb

Merge branch 'develop' of github.com:RocketChat/Rocket.Chat into threads

f8f4c96

mrsimpson approved these changes Feb 15, 2019

View reviewed changes

moved create thread and channel to popup

74392fa

ggazzo added 2 commits February 19, 2019 19:59

delete room, deletes all chain

c953ad6

added addRoomAccessValidator to allow user to join private subthreads

9443868

mrsimpson reviewed Feb 20, 2019

View reviewed changes

Merge branch 'pr/11803-mrsimpson-core/threading' into threads

0466e1b

ggazzo force-pushed the threads branch from 647267e to 0466e1b Compare February 20, 2019 14:52

ggazzo and others added 16 commits February 20, 2019 15:29

fix imports

fa889d4

fix imports

f8caa84

Merge branch 'core/threading' of github.com:assistify/Rocket.Chat int…

ab8c169

…o threads

deleting message will delete the thread

2332f0e

fix back icon

5c964e2

Regression: Add debounce on admin users search to avoid blocking by D…

00c59f2

…DP Rate Limiter (#13529)

fixed logic to prune thread message

0369f89

Merge branch 'develop' into threads

af87678

send message system on thread created

8a7fa76

Merge branch 'threads' of github.com:RocketChat/Rocket.Chat into threads

e1e7507

Move mongo config away from cors package (#13531)

cdfc3c0

* Move mongo config away from cors package * Remove TLS fix

[IMPROVE] Disable X-Powered-By header in all known express middlewares (

61debea

#13388)

fix typo and changed _id for username

5addfc4

Merge branch 'develop' into threads

23942a8

Regression: fix app pages styles (#13567)

b96b825

sampaiodiego and others added 22 commits March 7, 2019 09:36

Allow sending message before joining a thread

22002de

Allow any character on thread name

773ed6f

Fix 404 error when trying to see a not joined thread

1e54d65

Change thread icon

46b5231

Do not allow change avatars of another users without permission (#13629)

6ec15c8

auto invite user, freeze thread settings

dce3144

Merge branch 'threads' of github.com:RocketChat/Rocket.Chat into threads

d656657

Create threads as private groups

7773fa6

Prevent nested threads

d005119

fix search room

2c68516

name suggestion

09ae45c

changed text field name

8b57358

Fix getRoomById too-many-calls by adding a memoize

782b007

threads sidebar

33a3a6f

Create a new message to notify about a thread

16e90e2

Merge branch 'threads' of github.com:RocketChat/Rocket.Chat into threads

4045d3d

removed useless code

98d40c0

Merge branch 'develop' into threads

a4ce2d0

useless configs

f933e59

Merge branch 'threads' of github.com:RocketChat/Rocket.Chat into threads

3885401

Merge branch 'threads' of github.com:RocketChat/Rocket.Chat into threads

30900b7

Fix tests

881980e

sampaiodiego force-pushed the threads branch from 82141fa to 881980e Compare March 8, 2019 01:05

sampaiodiego added 4 commits March 7, 2019 23:13

Move package to mainModule

108c13b

Respect setting to show threads on sidebar

359b12e

Remove slash command files

574b96f

Add condition to show threads category

d2be600

ThomasRoehl closed this Mar 26, 2019

engelgabriel deleted the threads branch April 7, 2019 00:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] threads review/adjustments #572

[WIP] threads review/adjustments #572

ggazzo commented Feb 13, 2019

mrsimpson left a comment

mrsimpson Feb 15, 2019

ggazzo Feb 19, 2019

mrsimpson Feb 15, 2019

mrsimpson Feb 15, 2019

mrsimpson Feb 15, 2019

ggazzo commented Feb 19, 2019

mrsimpson commented Feb 20, 2019

mrsimpson left a comment

mrsimpson Feb 20, 2019

mrsimpson Feb 20, 2019

ggazzo Feb 20, 2019

mrsimpson Feb 20, 2019

mrsimpson Feb 20, 2019

mrsimpson Feb 20, 2019

mrsimpson Feb 20, 2019

mrsimpson Feb 20, 2019

ThomasRoehl commented Mar 26, 2019

[WIP] threads review/adjustments #572

[WIP] threads review/adjustments #572

Conversation

ggazzo commented Feb 13, 2019

mrsimpson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggazzo commented Feb 19, 2019

mrsimpson commented Feb 20, 2019

mrsimpson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThomasRoehl commented Mar 26, 2019