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

[FIX] No new room created when conversation is closed #13753

Merged
merged 6 commits into from
Mar 18, 2019

Conversation

knrt10
Copy link
Contributor

@knrt10 knrt10 commented Mar 17, 2019

Closes #13733

This PR solves the problem of creating a new room, again and again, once the conversation is closed.

Also fixed little CSS for analytics page from

old

to

new

cc @renatobecker would you please review.

@renatobecker-zz renatobecker-zz self-requested a review March 17, 2019 14:59
if (!guest) {
throw new Meteor.Error('invalid-token');
}

// Check if conversation is closed or not.
const closed = Rooms.findOneLivechatById(rid, fields);

Choose a reason for hiding this comment

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

We can't add this validation here because the current version of the Livechat client needs to create a new room when the current room is closed.
The point is that the issue #13733 is related to REST API endpoints, and our new Livechat client works over this approach, so I suggest you add validation on the Livechat message endpoint:

https://github.com/RocketChat/Rocket.Chat/blob/develop/app/livechat/server/api/v1/message.js#L33

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, will do that.

@knrt10
Copy link
Contributor Author

knrt10 commented Mar 17, 2019

tested

API test

@@ -34,6 +34,11 @@ API.v1.addRoute('livechat/message', {
throw new Meteor.Error('invalid-room');
}

const closed = findRoomClosed(rid);

Choose a reason for hiding this comment

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

You don't need to create another method to find rooms.

@@ -34,6 +34,11 @@ API.v1.addRoute('livechat/message', {
throw new Meteor.Error('invalid-room');
}

const closed = findRoomClosed(rid);
if (closed && closed.closedAt) {

Choose a reason for hiding this comment

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

You can check the open field, like:

if !(room && room.open) {
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it should be if (room && !room.open)

Choose a reason for hiding this comment

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

Actually my suggestion was to check both room and room.open in only one validation, but if you want to check the room.open separately, yeah you just need to check:

if (!room.open) {
...

Below the existing validation:

if (!room) {
    throw new Meteor.Error('invalid-room');
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it's fine right?

@@ -94,6 +94,21 @@ export class Rooms extends Base {
return this.find(query, options);
}

findOneLivechatById(_id, fields) {

Choose a reason for hiding this comment

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

You don't need this method.

@knrt10
Copy link
Contributor Author

knrt10 commented Mar 17, 2019

@renatobecker updated

@@ -34,6 +34,10 @@ API.v1.addRoute('livechat/message', {
throw new Meteor.Error('invalid-room');
}

if (room && !room.open) {

Choose a reason for hiding this comment

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

You can just check:

if (!room.open) {
...
}

Because the room object is checked here:
https://github.com/RocketChat/Rocket.Chat/blob/develop/app/livechat/server/api/v1/message.js#L33

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool, let me update it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -34,6 +34,10 @@ API.v1.addRoute('livechat/message', {
throw new Meteor.Error('invalid-room');
}

if (!room.open) {
throw new Meteor.Error('chat-closed');

Choose a reason for hiding this comment

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

Please, replace the chat-closed to room-closed, according we do here:
https://github.com/RocketChat/Rocket.Chat/blob/develop/app/livechat/server/api/v1/room.js#L56

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 😄

@engelgabriel engelgabriel added this to the 1.0.0 milestone Mar 18, 2019
@renatobecker-zz renatobecker-zz merged commit 6c08a6f into RocketChat:develop Mar 18, 2019
@sampaiodiego sampaiodiego changed the title [BUG] No new room created when conversation is closed [FIX] No new room created when conversation is closed Mar 18, 2019
@rodrigok rodrigok mentioned this pull request Apr 28, 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.

Bug Report: Livechat: New room created when send message to a closed room via LiveChat API
4 participants