[IMPROVE][Omnichannel] More info and better design of Past Chats List

[nitinkumartiwari]

The main goal expected from this improvement is to replace the current UI/UX of the Omnichannel Past Chats List feature.

From:

To:

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[renatobecker]

@nitinkumartiwari There is a lot of unnecessary code.
You need to make it more simple and readable, I saw unused import files, indentation issues and so on.
This is my first review, I'll be waiting for your changes and then I will review it again.

Thanks.

[renatobecker]

Remove all of the blank lines like this.

[renatobecker]

Why do you need this {{#if isAllChat}}?

[renatobecker]

You can move this to the top of the template, don't need any if/else statement.

[renatobecker]

Why do you need this {#if isfound }}?

[renatobecker]

Whi do you need another list to display the search results? You can use only one list and apply the filters through the same endpoint you fetch the data from the server.

[renatobecker]

Suggested change

	historyResult = await APIClient.v1.get(`livechat/messages.history/${ id }?token=${token}`);
	{ messages } = await APIClient.v1.get(`livechat/messages.history/${ id }?token=${token}`);

[nitinkumartiwari]

done

[renatobecker]

Why did you declare this if it's empty?

[nitinkumartiwari]

i have removed this

[renatobecker]

I don't think you need this method, you can use moment, the lib you imported in this file.

[nitinkumartiwari]

i have removed this function and now using moment

[renatobecker]

I don't think you need this method, you can use moment, the lib you imported in this file.

[nitinkumartiwari]

i have removed these function and now using moment

[renatobecker]

If you're replacing the old template you'll need to remove the old file as well.

[nitinkumartiwari]

i have removed the file as well

[lgtm-com]

This pull request introduces 12 alerts when merging 6ec867f into 5a1ef96 - view on LGTM.com

new alerts:

• 9 for Unused variable, import, function or class
• 3 for Missing variable declaration

[renatobecker]

There is a lot of indentation problems.
Also, I left again new comments trying to explain the correct way this implementation.

[renatobecker]

Fix the indentation.
Your indentation has both spaces and tabs. We use tabs for the indentation code.

[nitinkumartiwari]

fixed

[renatobecker]

Let's check if the customer already has chat history, if not, let's display a message.

Suggested change

	<div class="list-view">
	{{#if chatHistoryEmpty}}
	{{_ "Customer_has_no_chat_history"}}
	{{else}}
	<div class="list-view">

[nitinkumartiwari]

added a check if there is chat history available or not

[renatobecker]

Suggested change

	<li class='list-chat' id='{{_id}}' aria='{{v.token}}' >
	{{#each room in previousChats}} {{> chatRoomHistoryItem room }} {{/each}}

[nitinkumartiwari]

created A template called chatRoomHistoyItem

[renatobecker]

Why do you need this?
Also, you don't use var, we use let. In addition, when you assign a value to a variable that will not be reassigned, you can use const.

[nitinkumartiwari]

fixed

[renatobecker]

You can create a helper method to return the history item, as I mentioned above(customerChatHistoryItem) and then you can have a method like:

    getRoom(item) {
          // do what you need here and return...
    }

[renatobecker]

As I mentioned, all of this code below is unnecessary.
You need to create a loadRooms method that supports an optional parameter: searchTerm.
The method will call the livechat/visitors.chatHistory/ endpoint. When you are searching, you will call loadRooms(searchTerm).

Is not much more simple?
I shared with you an example on how to implement this, why not just follow this instruction?

There is no reason for this loop, well you call the endpoint, searching or not, then you'll get the rooms you need to render.
If you need to change the object before rendering, you can create a helper method to do it, the this will be the object you pass to the chatRoomHistoryItem template.

[renatobecker]

You don't need this.

[nitinkumartiwari]

done

[renatobecker]

The helper method is still there.

[renatobecker]

Remove all of these variables.

[nitinkumartiwari]

removed

[renatobecker]

Create a ReactiveVar to store the endpoint result.

[nitinkumartiwari]

Done

[renatobecker]

All of the code below is unnecessary.

[nitinkumartiwari]

removed the unnecessary code only required code is here

[lgtm-com]

This pull request introduces 7 alerts when merging 45ee0dd into 536ee97 - view on LGTM.com

new alerts:

• 6 for Unused variable, import, function or class
• 1 for Useless assignment to local variable

[renatobecker]

We are making progress, but we still need improvements.
Please use ESLint to fix your code, there are a lot of minor problems with your code that would be identified by ESLint extension.

[renatobecker]

't' is defined but never used.

[nitinkumartiwari]

Removed

[renatobecker]

My editor(VS CODE) is displaying this => "'addTime' is not defined".
Do you guys have the ESLint installed on your code editor?

In addition, this is happening because the addTime method is declared without context, there is no const assigned, etc.

In addition, you don't need that function(addTime). You have created a new template -> chatRoomHistoryItem, right? So, what you need to do is to add this as a helper method in that template, you don't need this method here in this template.

[nitinkumartiwari]

fixed

[renatobecker]

You're not using this const anywhere, you aren't applying the debounce on the method keyup #searchInput.
You need to do something like:

    ...
	'keyup #searchInput': _.debounce((e, t) => {
		e.stopPropagation();
		e.preventDefault();
		// your-code-here...        
	}, DEBOUNCE_TIME_FOR_SEARCH_CHATS_IN_MS),
    

[nitinkumartiwari]

done

[renatobecker]

Suggested change

	const searchText='null';
	const searchText='null';

Please, don't do this. You just need to declare the loadRooms(searchTerm) method inside the customerChatHistory.onCreated method, just like this:

	this.loadRooms(searchTerm) { ...

Inside this method, you will call the endpoint, and then you'll just apply a ternary operator to add the search term as parameters only when necessary.

[renatobecker]

Move this code inside the this.loadRooms(searchTerm) method.

Suggested change

	let baseUrl = `livechat/visitors.chatHistory/room/${ currentData.rid}/visitor/${ this.visitorId.get() }?searchText=${searchText}&count=${ ITEMS_COUNT }&offset=${ offset }`
	let baseUrl = `livechat/visitors.chatHistory/room/${ currentData.rid}/visitor/${ this.visitorId.get() }?count=${ ITEMS_COUNT }&offset=${ offset }`

this.loadRooms(searchTerm) {
instance.ready.set(false);

let baseUrl = `livechat/visitors.chatHistory/room/${ currentData.rid}/visitor/${ this.visitorId.get() }?count=${ ITEMS_COUNT }&offset=${ offset }`

if (searchTerm) {
	baseUrl += `&searchText=${ searchTerm }`;
}

const { history, total } = await APIClient.v1.get(baseUrl);
this.history.set(this.history.get().concat(history));	
this.total.set(total);
thisready.set(true);

};

[nitinkumartiwari]

done

[renatobecker]

Suggested change

	var roomid = history[i]._id;
	var roomId = history[i]._id;

[nitinkumartiwari]

fixed

[renatobecker]

Why 1000?
You just one match to know that the room has at least one message with the search term.

[nitinkumartiwari]

okay now count is 1

[renatobecker]

You don't need any if/else condition, you just need an if condition.
If the text variable is assigned, then you will perform the additional search of each room.

[nitinkumartiwari]

done

[renatobecker]

Is this update really needed?

[nitinkumartiwari]

i think its has updated automatically but i have changed as previous

[renatobecker]

Is this update really needed?

[nitinkumartiwari]

i think its has updated automatically but i have changed as previous

[lgtm-com]

This pull request introduces 1 alert when merging 4a7e476 into 5c993fe - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[renatobecker]

This template will only display closed conversations, so this if condition does not make sense.
Please, remove it.

[nitinkumartiwari]

removed

[renatobecker]

Create a helper method to return the formatted closedAt field.

[nitinkumartiwari]

done

[renatobecker]

Suggested change

	<p>{{room.msgs}} messages</p>
	<p>{{msgs}} messages</p>

[nitinkumartiwari]

done but this also includes the one as a connected message and one as a promptscript so there will be always +2 then the original message

[renatobecker]

The closing room message is not mandatory, there is a setting that forces the agent to leave a message when closing the conversation. So, this block of code is conditional, the template will need to provide a helper method that will be used to check if the closing message exists or not.
Something like:

• hasClosingRoomMessage

[nitinkumartiwari]

done

[renatobecker]

The lastMessage.msg is not the message that will be displayed here. The closing room message needs to be fetched from the DB, so you will need a helper method to return the message to the template.

Suggested change

	<p class="comment-text">"{{lastMessage.msg}}"</p>
	<p class="comment-text">"{{closingRoomMessage}}"</p>

[nitinkumartiwari]

performed a check if the lastMesaage is livechat-close message only then it will be displayed

[renatobecker]

You don't need to this here, you need to create a customerChatHistoryMessagesItem template and add a helper method there to display this kind of value.

Please, remove this loop and apply it in the right place.

[nitinkumartiwari]

done

[renatobecker]

Why did you import this method if you're not using it?

[nitinkumartiwari]

removed

[renatobecker]

You need to fetch only closed rooms from the database, not test it after fetching the data from the DB.
Suggestion: Create a new method on the LivechatRooms model similar to the findByVisitorId, you can create a LivechatRooms.findClosedByVisitorId and then you can use the correct method according to the query parameter value:

Suggested change

	if (closedChatsOnly) {
	const options = { sort: sort || { ts: -1 }, skip: offset, limit: count };
	const cursor = closedChatsOnly ? LivechatRooms.findClosedByVisitorId(visitorId, options) : LivechatRooms.findByVisitorId(visitorId, options);

[nitinkumartiwari]

done

[renatobecker]

Suggested change

	history.map(function(val) {
	history.map((val) => {

[nitinkumartiwari]

done

[renatobecker]

Suggested change

	result.map(function(e) {
	result.map((e) => resultArray.push(e));

[nitinkumartiwari]

done

[renatobecker]

Please, follow the prototype spec:

Suggested change

	<p>Close at time {{closedAt}}</p>
	<p>Close at {{closedAt}}</p>

[nitinkumartiwari]

done

[renatobecker]

This is not working and it's not the right way to implement it.
You need to fetch the livechat-close from the database and set this to the closingRoomMessage ReactiveVar.

Then, you can check here if the closingRoomMessage exists or not.

[renatobecker]

Fetch the livechat-close from the database.

[renatobecker]

Rename both js/html files template to customerChatRoomHistoryItem.

[renatobecker]

I have explained before:

• When you're searching or listing in the customerChatRoomHistory template, you'll display the results using the customerChatRoomHistoryItem template;
  When you're searching or listing in the customerChatHistoryMessages template you'll display the results using the customerChatHistoryMessagesItem template;

[renatobecker]

Why not using a correct name for this CONST?
DEBOUNCE_TIME_FOR_SEARCH_DEPARTMENTS_IN_MS -> DEBOUNCE_TIME_FOR_SEARCH_CHATS_IN_MS

[nitinkumartiwari]

done

[renatobecker]

You don't need this information here, you can pass it through the Template data.

[renatobecker]

You NEED To use the same template -> chatRoomHistoryItem you're search in the customerChatHistory template.
Here, the user wants to see the ROOMS that have a message that matches the filter.

[renatobecker]

We don't need to update this file, please undo it.

[nitinkumartiwari]

it is not updated as i can see on Rocket chat github it is same as mine

[renatobecker]

There is a lot of indentation problems here, your code has spaces and tabs.
Also, add these changes to the livechat.css file, not the main.css.

Another important thing to mention is related to CSS stuff. These changes messed up the styling of the other components, as you can see the Room Info. Panel:

• Correct version(develop):
  

• Your version:
  

We can't approve these changes.

[nitinkumartiwari]

I have created livechat css and solved the issue of messing issue but for the indention issue on css i was trying to resolve but i wont be able to . I am now only using indention of tab only but it always showing needed 1 indention weather i have added indention or not it always shows add indention i dont know how to resolve this issue

[renatobecker]

Remove the blank line

[nitinkumartiwari]

removed the blank lines from the code