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

PLT-6214 Move channel store and actions over to redux #6235

Merged
merged 6 commits into from
Apr 28, 2017
Merged

Conversation

jwilander
Copy link
Member

Summary

Move channel store and actions over to redux. Waiting on #6222 but ready for PM review.

Testing areas:

  • Creating/deleting/joining/leaving channels
  • Channel user management (roles, removing, adding, etc.)
  • Unreads between channels
  • Updating channel notification settings
  • Updating channel (header, purpose, etc.)
  • Auto-linking channels
  • Searching in a channel
  • Anything else channel related you can think of

Ticket Link

https://mattermost.atlassian.net/browse/PLT-6214

@jwilander jwilander added 1: PM Review Requires review by a product manager Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) Setup Old Test Server Triggers the creation of a test server labels Apr 25, 2017
@jwilander jwilander added this to the v3.9.0 milestone Apr 25, 2017
@jasonblais jasonblais self-assigned this Apr 26, 2017
@jasonblais
Copy link
Contributor

@jwilander the build seems to fail

@jwilander
Copy link
Member Author

jwilander commented Apr 26, 2017

Yeah, I'm looking into it. Seems unrelated to my changes

EDIT: I broke a unit test -.- (fixed now)

@jasonblais
Copy link
Contributor

jasonblais commented Apr 26, 2017

Tested on Win10/Chrome.

i'll have time later today to test channel notification preferences for email/mobile, gotta hop into something else for a bit,

  • Creating/deleting/joining/leaving channels
  • Unreads (messages and mentions) across channels
  • Updating channel (header, purpose, etc.)
  • Auto-linking channels
  • Searching in a channel
  • Pinning posts, favoriting channels
  • Max notifications settings
  • Channel member count icon
  • Channel user management (promoting/demoting from channel admin, removing, adding, etc.)
  • Notification preferences for 'mark channels as unread' and 'desktop notifications'
  1. Joining a channel post an incorrect system message:

Observed: {username} added to the channel by {username}

image

Expected: {username} has joined the channel.

  1. Deleted channel appears bolded in the sidebar for other users
  • create a private channel
  • add other users to the channel
  • delete the channel

observed: (for other users) Channel remains bolded in the sidebar. System message jason has archived the channel. is posted

image

expected: (for other users) Channel disappears from the sidebar

  1. Leaving a channel posts an incorrect system message
  • observed: test was removed from the channel
  • expected: test has left the channel.
  1. Private channels appear in "more public channel" list giving a permission error when trying to join

image

image

  1. Resetting channel URL doesn't close the "rename channel" modal properly
  • create a channel
  • go to "Rename Channel" option
  • change the URL
  • hit "Save"
  • observed: no visible action; unable to close the modal without refresh (the URL does successfully change though)
  1. Creating a DM fails and redirects you to Town Square
  • Click "More..." below DM list
  • Choose one member
  • Click "Go"
  • observed: User redirected to Town Square
  1. Changing "Mark Channel Unread" only to mentions also changes "Send desktop notifications" to "Never" in the UI (although the desktop notification setting itself doesn't change)

image

EDIT: It seems the "Mark Channel Unread" option in the UI keeps changing based on "send desktop notification" setting, although the functionality doesn't actually change

@jasonblais
Copy link
Contributor

Also found a bug that repros on master: https://mattermost.atlassian.net/browse/PLT-6414

@jwilander
Copy link
Member Author

Thanks @jasonblais! I'll go through these and fix them a bit later

@jwilander
Copy link
Member Author

@jasonblais fixed all above issues except for 7 which I wasn't able to reproduce

@jasonblais jasonblais added Setup Old Test Server Triggers the creation of a test server and removed Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) Setup Old Test Server Triggers the creation of a test server labels Apr 27, 2017
@jasonblais
Copy link
Contributor

Confirm 1 - 6 are fixed.

Got better repro steps for 7

  • Set Channel Notification Preferences > Mark Channel Unread to Only for mentions
  • Save
  • Close the modal
  • reopen channel notification preferences

Observed: Send desktop notifications set to Never in the UI (although it's actually set to whatever it was before)

image


Testing email/mobile push notifications right now,

@jasonblais
Copy link
Contributor

jasonblais commented Apr 27, 2017

8 - Setting one channel notification preference resets the other

  • set Send Desktop Notifications to 'only for mentions' (observed: notifications received for mentions only)
  • set Mark Channel Unread to 'only for mentions'
  • observed: Desktop notifications received for all messages. (The UI says never due to the bug described in 7)

Similarly, setting Send Desktop Notifications back to 'only for mentions' resets the Mark Channel Unread option. Clicking to edit the setting doesn't show either option being selected.

image

9 - Channel mobile push notification preferences don't work. They won't overwrite the account preferences.

  • have mobile push notification set 'for all activity' in Account Settings
  • change channel notification preferences to 'never'

Observed: Mobile push notifications for that channel keep coming. Same with different combinations of account/channel notification preferences.

(PS: No issues with email notifications at the account-level - I realized we don't have channel-specific settings for them)

@jasonblais jasonblais added Awaiting Submitter Action Blocked on the author and removed Awaiting Submitter Action Blocked on the author labels Apr 27, 2017
@jwilander
Copy link
Member Author

Alright fixed 7,8,9. Thanks for the extra repro steps, that helped a lot

@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 Apr 27, 2017
Copy link
Contributor

@jasonblais jasonblais left a comment

Choose a reason for hiding this comment

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

confirmed fixes

@jasonblais jasonblais added 2: Dev Review Requires review by a developer and removed 1: PM Review Requires review by a product manager labels Apr 28, 2017
@jasonblais jasonblais removed the Setup Old Test Server Triggers the creation of a test server label Apr 28, 2017
patchChannel,
getChannelMembersByIds,
deleteChannel as deleteChannelRedux
} from 'mattermost-redux/actions/channels';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not do something like import * as ChannelActions from 'mattermost-redux/actions/channels'? im really 0/5 but it seems simpler that way

@@ -283,11 +239,11 @@ export function unmarkFavorite(channelId) {
}

export function loadChannelsForCurrentUser() {
AsyncClient.getChannels().then(() => {
AsyncClient.getMyChannelMembers().then(() => {
fetchMyChannelsAndMembers(TeamStore.getCurrentId())(dispatch, getState).then(
Copy link
Contributor

Choose a reason for hiding this comment

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

im also 0/5 on this but you could do

fetchMyChannelsAndMembers(TeamStore.getCurrentId())(dispatch, getState).
then(loadDMsAndGMsForUnreads);

@@ -70,7 +70,7 @@
},
"SqlSettings": {
"DriverName": "mysql",
"DataSource": "mmuser:mostest@tcp(dockerhost:3306)/mattermost_test?charset=utf8mb4,utf8&readTimeout=30s&writeTimeout=30s",
Copy link
Member

Choose a reason for hiding this comment

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

Unintended change.

@jwilander
Copy link
Member Author

@crspeller fixed

@crspeller crspeller merged commit 9690648 into master Apr 28, 2017
@crspeller crspeller deleted the redux-part3 branch April 28, 2017 17:16
@jasonblais jasonblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Tests/Not Needed New release tests not required labels Apr 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a developer Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Tests/Not Needed New release tests not required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants