Skip to content

[FIX] Strange msg when setting room announcement, topic or description to be empty#11012

Merged
engelgabriel merged 3 commits into
RocketChat:developfrom
vynmera:the-empty-string-conundrum
Jun 6, 2018
Merged

[FIX] Strange msg when setting room announcement, topic or description to be empty#11012
engelgabriel merged 3 commits into
RocketChat:developfrom
vynmera:the-empty-string-conundrum

Conversation

@vynmera

@vynmera vynmera commented Jun 5, 2018

Copy link
Copy Markdown
Contributor

People in my team were getting confused by this, so a very simple fix for now as, looking at the code, changing it to e.g. "Room announcement removed by user" will be quite difficult:

Before
2018-06-05_21-00-30

After
2018-06-05_21-06-44

It looks better than just keeping that empty string there, I suppose.

@kaiiiiiiiii

Copy link
Copy Markdown
Contributor

Why don’t we change the hole message to something like “Room announcement was removed by ...”? :)

@vynmera

vynmera commented Jun 5, 2018

Copy link
Copy Markdown
Contributor Author

@kaiiiiiiiii I looked into that, actually. The way these messages are set up right now, it'd either be very hacky to add, or require a relatively large change in how the messages are rendered. Unless I'm missing something important...

@kaiiiiiiiii

Copy link
Copy Markdown
Contributor

Aaah, totally missed that part in your PR description ... sorry 'bout that ^^'

@vynmera

vynmera commented Jun 5, 2018

Copy link
Copy Markdown
Contributor Author

@kaiiiiiiiii Don't worry about it :-) For insight, you can see it hardcoded here: https://github.com/RocketChat/Rocket.Chat/blob/develop/packages/rocketchat-channel-settings/client/startup/messageTypes.js#L29

I tried to add a message_override, which would trigger if the new content was empty, but I couldn't get it to work, sadly (I suppose I'm still too new to this codebase). If someone else manages to do it, do tell as that'd look a lot better than both options we have right now~

@kaiiiiiiiii

kaiiiiiiiii commented Jun 5, 2018

Copy link
Copy Markdown
Contributor

I looked into it as well and your right, it's really tricky and there should be a complete revision of the hole process to create those messages..

An ugly, but probably working solution would be:

  • New messageType 'room_announcement_removed'
  • Extended function call in saveRoomAnnouncement.js

RocketChat.models.Messages.createRoomSettingsChangedWithTypeRoomIdMessageAndUser(escapedMessage ? 'room_changed_announcement' : 'room_announcement_removed', rid, escapedMessage, user);

But I don't really like that idea and I'm not a 100% sure if it's gonna work ^^'

@ggazzo ggazzo added this to the 0.66.0 milestone Jun 6, 2018
@engelgabriel engelgabriel changed the title [FIX] Some empty strings when setting room announcement, topic or description to be empty [FIX] Strange msg when setting room announcement, topic or description to be empty Jun 6, 2018
@engelgabriel engelgabriel merged commit 3e9a856 into RocketChat:develop Jun 6, 2018
@vynmera vynmera deleted the the-empty-string-conundrum branch June 7, 2018 07:03
@rodrigok rodrigok mentioned this pull request Jun 28, 2018
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.

4 participants