MM-12233: local user mention results #1974

lieut-data · 2018-10-30T14:32:31Z

Summary

Leverage existing data in the Redux store to present local at mentions even before the server responds with results. This is not strictly dependent on mattermost/mattermost-redux#693, but that PR pre-fills the data using knowledge from the posts.

Ticket Link

https://mattermost.atlassian.net/browse/MM-12233

Checklist

Ran make check-style to check for style errors (required for all pull requests)
Ran make test to ensure unit and component tests passed
Added or updated unit tests (required for all new features)
Needs to be implemented in mobile (link to PR or User Story)
Has server changes (please link)
Has redux changes: MM-12233: update profiles in/not in channel on ws mattermost-redux#715
Has UI changes
Includes text changes and localization file (.../i18n/en.json) updates
Touches critical sections of the codebase (auth, posting, etc.)

jasonblais · 2018-11-09T03:29:38Z

@lieut-data Should I test this PR after the redux one is merged, or before? Should I also test the Redux PR separately?

lieut-data · 2018-11-09T03:30:38Z

@jasonblais, this PR has the Redux commit baked into it, so it's worth testing together. I've added you as reviewer to mattermost/mattermost-redux#693 explicitly to get your feedback on that part of the change.

jasonblais · 2018-11-09T03:33:18Z

Thanks Jesse, just saw the note in the redux PR. Will review both 👍

lieut-data · 2018-11-09T03:35:35Z

Heads up that there is an issue with the with the RHS throwing an exception for which I'll push up a commit for, but I won't bother recreating the test server right away.

jasonblais

Observations (Mac Sierra; Chrome):

1 - Join a channel with previous posts; Type @ in the message box.

Observed: User autocomplete doesn't open. Clearing the at-symbol and re-typing it opens the user autocomplete.

This also reproduces for channels I've joined but haven't yet viewed in my session. Also reproduces after posting an at-mention, and re-typing @ in the message box.

Every at-mention results in user not in channel even if they still were

In the attached screenshot, brandon.ford has left the channel but appears in channel member lists, and the in channel list in user autocomplete. It persists after a page reload.

If the user is removed from the channel, the above doesn't reproduce. Not reproducing on the master branch.

4 - I want to pause testing and fully understand the consequences of the Redux PR mattermost/mattermost-redux#693

I assume what we see in point 3 would be a consequence, since the user sees a post from Brandon in the channel and the app wouldn't know if they have left or not.

If so, I worry about that consequence. We have received reports in the past from customers where they saw a user leave a channel (or a user was removed from the channel), yet they still appeared to be a member of the channel. They were concerned if they still had access to the private conversations.

Previously those reports were caused by actual bugs, but I worry the results in 3 will be perceived as such.

jasonblais

Based on above

lieut-data · 2018-11-10T19:40:02Z

Thanks for the feedback, @jasonblais!

Interesting: I'll have to dig deeper on this one.
I'm glad you observed this: the generated data data on the spinmint servers isn't realistic, as posts are made from users at random without taking into account if they are in the channel. This dissonance helps illustrate the effect of the Redux commit, though it does make it hard to assess the "correctness" of this implementation.
(same as above)
I agree that the potential for confusion is present. We could certainly drop the Redux commit from the equation, at the cost of no longer having immediate results on first load. I'm wondering if, instead, we might rebrand the autocomplete from Channel Members and Not In Channel to Recent Users and Other Users, effectively making the sections more ambiguous. Thoughts?

jasonblais · 2018-11-10T22:00:08Z

4 - There might certainly be UX changes we could consider - some competitors don't separate users in the autocomplete at all.

To confirm:

without Redux commit, we present local at mention suggestions immediately, followed by the server results
with Redux commit, we do the same but ALSO have them on first load.

Is the above correct?

5 - Question: Do we need to make any changes on mobile?

jasonblais · 2018-11-20T18:04:59Z

Thanks all! @lieut-data I moved this to dev review, please help queue it for the appropriate devs :)

lieut-data · 2018-11-20T19:01:17Z

components/create_comment/create_comment.jsx

@@ -444,7 +444,9 @@ export default class CreateComment extends React.PureComponent {

 if (allowSending) {
 e.persist();
- this.refs.textbox.blur();
+ if (this.refs.textbox) {
+ this.refs.textbox.getWrappedInstance().blur();


I'd love feedback on connecting Textbox and updating the world to poke through the wrapped instance like this. An alternative was to push up the the store access to all components that used the Textbox, but that seems like a violation of concerns. Alternatively, I could have accessed the store globally from within the suggestion provider, but there's currently no way to "trigger" the user of the provider to try a pretext match.

I'm not sure I understand. Do you mean to avoid the mess of getWrappedInstance?

Yeah, that :/

lieut-data · 2018-11-20T19:05:48Z

components/textbox/textbox.jsx

@@ -52,7 +58,12 @@ export default class Textbox extends React.Component {
 this.state = {};

 this.suggestionProviders = [
- new AtMentionProvider(this.props.channelId),
+ new AtMentionProvider({


Ideally, AtMentionProvider would be connected itself, but while this is technically possible, it still leaves us without a way to back-propagate pretext changes on props-only changes unless Textbox is in on the loop.

hmhealey · 2018-11-21T17:00:12Z

components/suggestion/at_mention_provider/at_mention_provider.jsx

+ }
+
+ // setProps gives the provider additional context for matching pretexts. Ideally this would
+ // just be something akin to a connected component with access to the store itself.


This is something I've wanted to look at as well. We could either:

Change the Provider classes to components that just render nothing so that we can render something like <SuggestionBox><AtMentionProvider/></SuggestionBox>

Hook into the context when instantiating the Textbox to pass the store directly to the Provider

The first method is a bit hacky since the provider is not really a component, but it is cleaner than the second one since that requires us to breach the encapsulation provided by react-redux

The first also naturally moves us towards triggering a "render" (i.e. prefix match) on any change, while the latter has the difficulty of still needing to notify the renderer that the provider has changed, perhaps outside of an explicit pretext match. Complicated either way, I suppose!

hmhealey · 2018-11-21T17:04:42Z

components/suggestion/suggestion_list.jsx

@@ -65,6 +65,10 @@ export default class SuggestionList extends React.PureComponent {
 const contentBottomPadding = parseInt(content.css('padding-top'), 10);

 const item = $(ReactDOM.findDOMNode(this.refs[term]));
+ if (!item[0]) {


Minor nit, but could this be item.length === 0? It might just be a bit clearer

hmhealey · 2018-11-21T17:08:47Z

components/textbox/index.js

+
+import Textbox from './textbox.jsx';
+
+const autocompleteUsersInChannel = (prefix) => (dispatch, getState) => {


This should probably live in actions/views

hmhealey · 2018-11-21T17:12:03Z

utils/constants.jsx

@@ -1269,6 +1269,7 @@ export const Constants = {
 MENTION_MORE_CHANNELS: 'mention.morechannels',
 MENTION_UNREAD_CHANNELS: 'mention.unread.channels',
 MENTION_MEMBERS: 'mention.members',
+ MENTION_MORE_MEMBERS: 'mention.moremembers',


Does this need to be wrapped in the t function somewhere?

hmhealey · 2018-11-21T17:13:44Z

i18n/en.json

@@ -2567,6 +2567,7 @@
 "suggestion.mention.channels": "My Channels",
 "suggestion.mention.here": "Notifies everyone in the channel and online",
 "suggestion.mention.members": "Channel Members",
+ "suggestion.mention.moremembers": "Other Members",


Should this be "Other Users"? Without "Channel" or "Team" for context, "Members" looks weird to be

Where/when does this string occur?

I feel it was discussed, but I forget now, and didn't spot it on the latest spinmint below.

This would appear over the loading section, but disappears once everything is merged together.

Got it. I think Members referring to channel members is quite clear in this context.

Are those users channel members though? I thought this was for loading people who aren't in the channel

@hmhealey Some are channel members, some are not.

We are first loading users locally, so the query returns immediate results. We then load everyone else, most of which are not in the channel but a few that could be.

Once the loading has finished, there will be three sections: Channel Members, Special Mentions, and Not In Channel

(Jesse is more equipped to go into more of the technical details if needed)

The gist of these changes is to:

Source results from the local redux cache immediately (if available)

Make a network request to fetch more details

The Other Members will never be shown except with the loading indicator, as the results of the network request will be merged into the appropriate Channel Members or Not In Channel. Other Members is somewhat inaccurate, in that it suggests its just loading other channel members, when in fact it will return non-channel members as well. We could say, Other Users if it would help clarify. 0/5

mattermod · 2018-11-21T17:39:20Z

Setup Test Server label detected. Spinmint test server created if build succeeds (checks pass and no conflicts with base branch).

mattermod · 2018-11-21T17:41:34Z

Spinmint test server created at: https://i-08a63d874aaf7e2bc.test.spinmint.com

Test Admin Account: Username: sysadmin | Password: sysadmin

Test User Account: Username: user-1 | Password: user-1

Instance ID: i-08a63d874aaf7e2bc

…-mention-results

jasonblais · 2018-11-22T14:23:21Z

@crspeller @hmhealey this one is ready for re-review

crspeller

Looks good!

mattermod · 2018-11-23T17:54:42Z

Spinmint test server destroyed

mattermod · 2018-11-26T18:36:28Z

Spinmint test running for more than 7 days. This test server was terminated.

mattermod · 2018-11-27T04:48:19Z

Spinmint test running for more than 7 days. This test server was terminated.

lieut-data added Work in Progress Not yet ready for review Do Not Merge Should not be merged until this label is removed Setup Old Test Server Triggers the creation of a test server labels Oct 30, 2018

lieut-data force-pushed the mm-12233-local-user-mention-results branch 4 times, most recently from 26d8c47 to f3cd349 Compare November 8, 2018 18:35

mattermost deleted a comment from mattermod Nov 8, 2018

lieut-data force-pushed the mm-12233-local-user-mention-results branch from f3cd349 to be4eae5 Compare November 9, 2018 03:25

lieut-data removed the Setup Old Test Server Triggers the creation of a test server label Nov 9, 2018

lieut-data requested a review from jasonblais November 9, 2018 03:26

lieut-data added 1: PM Review Requires review by a product manager Setup Old Test Server Triggers the creation of a test server and removed Do Not Merge Should not be merged until this label is removed Work in Progress Not yet ready for review labels Nov 9, 2018

lieut-data mentioned this pull request Nov 9, 2018

MM-12233: local user mention results mattermost/mattermost-redux#693

Closed

5 tasks

jasonblais self-assigned this Nov 9, 2018

jasonblais reviewed Nov 10, 2018

View reviewed changes

jasonblais suggested changes Nov 10, 2018

View reviewed changes

amyblais added this to the v5.6.0 milestone Nov 16, 2018

mattermost deleted a comment from mattermod Nov 17, 2018

jasonblais removed the Setup Old Test Server Triggers the creation of a test server label Nov 20, 2018

mattermost deleted a comment from mattermod Nov 20, 2018

lindy65 added the Tests/Done Release tests have been written label Nov 20, 2018

lieut-data requested review from crspeller and hmhealey November 20, 2018 18:59

lieut-data commented Nov 20, 2018

View reviewed changes

hmhealey reviewed Nov 21, 2018

View reviewed changes

jasonblais added the Setup Old Test Server Triggers the creation of a test server label Nov 21, 2018

lieut-data added 2 commits November 21, 2018 21:34

Merge remote-tracking branch 'origin/master' into mm-12233-local-user…

a0d09f1

…-mention-results

address review feedback

ddbcf1b

Merge branch 'master' into mm-12233-local-user-mention-results

22a82ab

crspeller approved these changes Nov 23, 2018

View reviewed changes

hmhealey approved these changes Nov 23, 2018

View reviewed changes

hmhealey merged commit a89ee40 into master Nov 23, 2018

hmhealey deleted the mm-12233-local-user-mention-results branch November 23, 2018 17:54

hmhealey removed 2: Dev Review Requires review by a core commiter Setup Old Test Server Triggers the creation of a test server labels Nov 23, 2018

amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Nov 23, 2018

JtheBAB mentioned this pull request Jan 15, 2019

rebase (#1) #2272

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MM-12233: local user mention results #1974

MM-12233: local user mention results #1974

lieut-data commented Oct 30, 2018 •

edited

Loading

jasonblais commented Nov 9, 2018

lieut-data commented Nov 9, 2018

jasonblais commented Nov 9, 2018

lieut-data commented Nov 9, 2018

jasonblais left a comment

jasonblais left a comment

lieut-data commented Nov 10, 2018 •

edited

Loading

jasonblais commented Nov 10, 2018

jasonblais commented Nov 20, 2018

lieut-data Nov 20, 2018

hmhealey Nov 21, 2018 •

edited

Loading

lieut-data Nov 22, 2018

lieut-data Nov 20, 2018

hmhealey Nov 21, 2018

lieut-data Nov 22, 2018

hmhealey Nov 21, 2018

hmhealey Nov 21, 2018

hmhealey Nov 21, 2018

hmhealey Nov 21, 2018

jasonblais Nov 21, 2018

lieut-data Nov 22, 2018

jasonblais Nov 22, 2018

hmhealey Nov 23, 2018

jasonblais Nov 23, 2018 •

edited

Loading

lieut-data Nov 23, 2018

mattermod commented Nov 21, 2018

mattermod commented Nov 21, 2018

jasonblais commented Nov 22, 2018

crspeller left a comment

mattermod commented Nov 23, 2018

mattermod commented Nov 26, 2018

mattermod commented Nov 27, 2018


		import Textbox from './textbox.jsx';

		const autocompleteUsersInChannel = (prefix) => (dispatch, getState) => {

MM-12233: local user mention results #1974

MM-12233: local user mention results #1974

Conversation

lieut-data commented Oct 30, 2018 • edited Loading

Summary

Ticket Link

Checklist

jasonblais commented Nov 9, 2018

lieut-data commented Nov 9, 2018

jasonblais commented Nov 9, 2018

lieut-data commented Nov 9, 2018

jasonblais left a comment

Choose a reason for hiding this comment

jasonblais left a comment

Choose a reason for hiding this comment

lieut-data commented Nov 10, 2018 • edited Loading

jasonblais commented Nov 10, 2018

jasonblais commented Nov 20, 2018

Choose a reason for hiding this comment

hmhealey Nov 21, 2018 • edited Loading

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

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

jasonblais Nov 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattermod commented Nov 21, 2018

mattermod commented Nov 21, 2018

jasonblais commented Nov 22, 2018

crspeller left a comment

Choose a reason for hiding this comment

mattermod commented Nov 23, 2018

mattermod commented Nov 26, 2018

mattermod commented Nov 27, 2018

lieut-data commented Oct 30, 2018 •

edited

Loading

lieut-data commented Nov 10, 2018 •

edited

Loading

hmhealey Nov 21, 2018 •

edited

Loading

jasonblais Nov 23, 2018 •

edited

Loading