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

[NEW] Store the last sent message to show below the room's name by default #10597

Merged
merged 3 commits into from
May 3, 2018

Conversation

graywolf336
Copy link
Contributor

Closes #10490 - There were some discussions about this a few days ago regarding the mobile applications and their future. Yes, this setting can be disabled and yes the mobile applications need to deal with that possibility but it looks better with this enabled by default as there are several questions and issues raised daily about this.

@graywolf336 graywolf336 added this to the 0.64.0 milestone Apr 26, 2018
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-10597 April 26, 2018 21:11 Inactive
@geekgonecrazy
Copy link
Contributor

@sampaiodiego @rodrigok can we get this in? This fits in with our path forward and the desired out of the box user experience.

@sampaiodiego
Copy link
Member

I'm not sure about the migration..

it can enable it to instances that have already tested but disabled again on purpose..

if we go this way I think we need to add a proper notice on release notes.

@geekgonecrazy
Copy link
Contributor

geekgonecrazy commented Apr 26, 2018

@sampaiodiego Can't we check packageValue == true ? If so then they never touched?

I know its false if different. I assume its false if the user modified it at all

@graywolf336
Copy link
Contributor Author

That's a fantastic suggestion @geekgonecrazy

@sampaiodiego
Copy link
Member

seems the tests are failing 🤔

@rodrigok
Copy link
Member

@graywolf336 what if we change the default to true but let to migrate the server on next version, until there we could receive more feedbacks from the community

@graywolf336
Copy link
Contributor Author

@rodrigok No sir, because by then it will be too late and we won't be viewed as proactive from the community in my opinion. The version of Android which expects this is already out in production, so if we delay then we will only get more issues opened regarding this. Thus I stand behind this pull request and that it should be in this version, not delayed.

@rodrigok
Copy link
Member

@graywolf336 I don't share your vision, we didn't decided that the last message would be required for mobile now, we have the option to enable and will enable for new installations, just don't want to have big problems with big installations now. So I don't agree with changing all the existent installations to enable this feature.

@graywolf336
Copy link
Contributor Author

@rodrigok When was that not decided? Because the internal discussion that we had a few days ago was quite the opposite of what you're saying now. It was conclusion that because we are moving all three mobile clients to the new "messenger" style views, that this change is needed to be enabled by default because it is breaking the user's experience. Yes, we need to support this setting being disabled on the mobile applications and present it in a nice way but until that happens, which @rafaelks can give us the rough timeline, then we had decided this was the route to go. If you still feel strongly against this, then @RocketChat/core we need everyone's opinions. If you're worried about large servers, then let's just list this change as BREAK so administrators of servers can quickly realize what the change is and what it does. :)

@rodrigok
Copy link
Member

@graywolf336 Yes, I understand, just don't see why we can't execute this in 2 steps.

@sampaiodiego
Copy link
Member

I understand @graywolf336 's points and it follows the discussion we had previously, but I liked the two step approach: in this release we say we're changing it to default and make it clear we will enforce that on the next release.

also mobile apps updates are much faster then server updates.. so even if we force enabling last message on this release, that does not mean everyone will update the servers and then have a better mobile experience. if the mobile experience is the focus here, it should be done on the mobiles to support servers without last message, since the stores will try to update the apps as soon they're released.

@rafaelks
Copy link
Contributor

I share @graywolf336, @geekgonecrazy and @sampaiodiego points guys. This setting was initially created to experiment with it.

The internal decision was (copying):

  1. We need to enable this option by default on new servers. We need to add a migration to enable it on all servers and then also update the room/subscription object with the last message.
  2. The mobile apps need to detect if this setting is disabled and if so, come up with a decent way to show the chat lists without the latest message due to the setting being able to be disabled for performance reasons or privacy concerns.

The experience on the mobile apps (Android right now, iOS in near future) is much better with this option available and most of the people don't know about the existence of this setting (it's proved by the negative reviews we had, issues opened, etc).

IMHO we can add this migration, the default value but we need to add a WARNING in the release notes talking about it.

People are using the app now, not in the future. They upgrade their server and they keep seeing the "No messages yet." message. The experience now is buggy (also client's fault, of course).

@rafaelks
Copy link
Contributor

@sampaiodiego

Also mobile apps updates are much faster then server updates.. so even if we force enabling last message on this release, that does not mean everyone will update the servers and then have a better mobile experience. if the mobile experience is the focus here, it should be done on the mobiles to support servers without last message, since the stores will try to update the apps as soon they're released.

Next release cycle for the mobile apps is in May 27th and there's lots of things we need to do before taking care of this setting disabled, to be honest. This should not be the default experience.

@rafaelks
Copy link
Contributor

rafaelks commented Apr 27, 2018

BTW, if it IS risky to enable it by default and migrate, then I would wait and think more about it. But do we know if this is risky?

@rafaelks
Copy link
Contributor

@graywolf336 Is there a way to migrate ONLY if the admin never touched this setting?

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-10597 April 27, 2018 22:37 Inactive
@engelgabriel
Copy link
Member

@rafaelks I agree that this should not be the default experience, but we can't turn this feature on by default retroactive before we are sure about the performance impact. So the mobile apps MUST support this feature being off ASAP, even if the mobile team have to do a mid cycle bug fix release.

@engelgabriel engelgabriel modified the milestones: 0.64.0, 0.65.0 Apr 27, 2018
@rafaelks
Copy link
Contributor

@engelgabriel That's not a problem, we can prioritize this change during our release candidate phase next week as we spoke few minutes ago. 👍

But please, let's make sure that this migration gets included ASAP in our back-end (if we find out that it works correctly to everybody) and set the default TRUE to 0.64.0?

@rodrigok
Copy link
Member

I'll change this PR to remove the migration and keep the default value as true.

@rafaelks the mobile could warning the users when accessing a server with the lastMessage setting disabled, something like Your server has the setting *Store Last Message* disabled, your experience will not be the best as possible, please ask the administrators to enable the setting for the first access. We could add that to our web interface too for admins.

@rodrigok rodrigok temporarily deployed to rocket-chat-pr-10597 April 28, 2018 02:12 Inactive
@rodrigok rodrigok changed the title [NEW] Storing of the last sent message is stored by default [NEW] Store the last sent message to show bellow the room's name by default Apr 28, 2018
@rodrigok rodrigok force-pushed the fix/enable-last-message-by-default branch from 68b2068 to e7fee81 Compare April 28, 2018 02:54
@rodrigok rodrigok temporarily deployed to rocket-chat-pr-10597 April 28, 2018 02:54 Inactive
@rafaelks
Copy link
Contributor

rafaelks commented May 1, 2018

@engelgabriel @rodrigok @sampaiodiego We're releasing the Android already with this check implemented:

img_0078
img_0077

@rodrigok rodrigok merged commit cfe6c08 into develop May 3, 2018
@rodrigok rodrigok deleted the fix/enable-last-message-by-default branch May 3, 2018 14:05
@rodrigok rodrigok mentioned this pull request May 3, 2018
@rodrigok rodrigok changed the title [NEW] Store the last sent message to show bellow the room's name by default [NEW] Store the last sent message to show below the room's name by default May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Store last message" default breaks Android native 2.x
6 participants