Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed @ not working on latest build due to... #5878

Merged
merged 2 commits into from Mar 31, 2017
Merged

Fixed @ not working on latest build due to... #5878

merged 2 commits into from Mar 31, 2017

Conversation

prixone
Copy link
Contributor

@prixone prixone commented Mar 26, 2017

Summary

On latest build the @ shortcut to search for mentions was not working due to missing parameters on the performSearch method used by the onListenerChange method, due to changes done previously to post_action and search_bar that were not made to the performSearch that exists within the onListenerChange method.

Ticket Link

NONE

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • Added or updated unit tests (required for all new features)
  • Added API documentation (required for all new APIs)
  • All new/modified APIs include changes to the drivers
  • Has enterprise changes (please link)
  • Has UI changes
  • Includes text changes and localization file (.../i18n/en.json and .../webapp/i18n/en.json) updates
  • Touches critical sections of the codebase (auth, upgrade, etc.)

On latest build the @ shortcut to search for mentions was not working due to missing parameters on the onListenerChange method, due to changes done previously to post_action and search_bar that were not made to the onListenerChange method.
@mattermod
Copy link
Contributor

Thanks @prixone for the pull request!

Per the CONTRIBUTING.md file displayed when you created this pull request, we need to add you to the list of approved contributors for the Mattermost project.

Please help complete the Mattermost contribution license agreement?

This is a standard procedure for many open source projects. Your form should be processed within 24 hours and reviewers for your pull request will be able to proceed.

Please let us know if you have any questions.

We are very happy to have you join our growing community! If you're not yet a member, please consider joining our Contributors community channel to meet other contributors and discuss new opportunities with the core team.

@enahum enahum added the 2: Dev Review Requires review by a developer label Mar 26, 2017
@jasonblais jasonblais added the Setup Old Test Server Triggers the creation of a test server label Mar 28, 2017
@jwilander
Copy link
Member

@lindalumitchell did you have a chance yet to test this out?

@lindalumitchell
Copy link
Contributor

Ah, thank you @jwilander; I didn't see this one come through. I just tested, and the at-mention search is working as expected on the spinmint server. No issues found.

added fix for when no notification options for mentions is defined by the user...
@prixone
Copy link
Contributor Author

prixone commented Mar 30, 2017

Sorry everyone, I've found a new issue here and since it hasn't been pushed out I've merged it.

from:

if (doSearch) {

to:

if (doSearch && newState && newState.searchTerm.length) {

Basically, if the user does not have the option Your non-case sensitive username "user" selected at the notification settings for mentions, see image below:
notification settings

The system does not give anything to the search field thus this fix should avoid it from submitting the request in this specific scenario to avoid an "invalid search term" from being sent to the server.

I've looked thru the commits on that file for previous commit but NONE seemed to deal with or change this behavior so I assume that when the user does not have anything select, not sending the request to the server is the correct behavior here.

@lindalumitchell sorry to disturb you, would you mind testing for the above mentioned? thanks.

@coreyhulen coreyhulen added Setup Old Test Server Triggers the creation of a test server and removed Setup Old Test Server Triggers the creation of a test server labels Mar 30, 2017
@coreyhulen coreyhulen closed this Mar 30, 2017
@coreyhulen coreyhulen reopened this Mar 30, 2017
@coreyhulen coreyhulen added the Setup Old Test Server Triggers the creation of a test server label Mar 30, 2017
@jasonblais jasonblais added Setup Old Test Server Triggers the creation of a test server and removed Setup Old Test Server Triggers the creation of a test server labels Mar 30, 2017
@mattermod
Copy link
Contributor

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

@jasonblais jasonblais added the Setup Old Test Server Triggers the creation of a test server label Mar 30, 2017
@mattermod
Copy link
Contributor

Spinmint test server created at: http://i-0406880c488984158.spinmint.com:8065/pr5878

Test Account 1: Email: test@test.com | Password: passwd

Test Account 2: Email: test2@test.com | Password: passwd

Instance ID: i-0406880c488984158

@lindalumitchell
Copy link
Contributor

@prixone @jwilander @jasonblais I re-tested for the described scenario, with all boxes in the Words that trigger mentions setting unchecked. Clicking the @ icon does nothing in that case (even though @[username] still triggers a mention). No errors observed.

When at least one option is checked, the @ icon displays mentions in RHS as expected.

It sounds like the scenario with no mention options checked is a separate issue, and I'll file a separate bug for that one (https://pre-release.mattermost.com/core/pl/ycs8oijwcjfadmc8donc4kx7tr).

So this one checks out; no issues found.

@mattermod
Copy link
Contributor

Spinmint test server destroyed

@crspeller crspeller removed 2: Dev Review Requires review by a developer Setup Old Test Server Triggers the creation of a test server labels Mar 31, 2017
@crspeller crspeller merged commit fa40bfb into mattermost:master Mar 31, 2017
@lindalumitchell lindalumitchell added the Tests/Done Required release tests have been written label Mar 31, 2017
@esethna esethna added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Apr 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Tests/Done Required release tests have been written
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants