[NEW] adding custom sidebar icon method by AllanPazRibeiro · Pull Request #459 · RocketChat/Rocket.Chat.Apps-engine

AllanPazRibeiro · 2021-12-09T18:50:51Z

What? ⛵

This PR is about a feature that allows the App Developer to send a custom icon (SVG) for the room's sidebar.

With that in mind, this PR adds the following stuff:

Method to set the icon name;
Add the room icon source type;

Why? 🤔

Links 🌎

PS 👀

…s-engine into feat-adding-custom-sidebar-icon-method

CLAassistant · 2021-12-09T18:50:57Z

All committers have signed the CLA.

src/server/accessors/RoomBuilder.ts

graywolf336 · 2021-12-10T00:28:02Z

My concern with SVG is we open ourselves to the common SVG attacks, unless there’s a way to get around this

AllanPazRibeiro · 2021-12-10T13:21:59Z

Hi @graywolf336, thank you for your comments, regarding the security issues, it is dealt with on the frontend by the below actions:

All the potential injection attributes are removed.
And no inline script is executed.

For a more detailed explanation of how the frontend deal with it, you can reach @ggazzo

graywolf336 · 2021-12-10T13:24:20Z

src/definition/rooms/IRoom.ts

    customFields?: { [key: string]: any };
    parentRoom?: IRoom;
    livechatData?: { [key: string]: any };
+    source?: {


This source property is so generic that someone who does not know Rocket.Chat's internals would not know what this is. In fact, I personally have no idea what it is referring to. What's the purpose of this? Can it be named something better and then mapped inside of Rocket.Chat?

I agree that this name is too generic but I just decided to use the same property name as it used on the omnichannel room on Rocket.chat project as you can see on this PR

Right, I understand that. However, what I am saying is that we should map it to a name that actually provides meaning instead of further compounding the confusion and generics.

graywolf336 · 2021-12-10T13:24:55Z

src/server/accessors/RoomBuilder.ts

    }
+
+    public setSidebarIcon(sidebarIcon: string): IRoomBuilder {
+        this.room.source.sidebarIcon = sidebarIcon;


source can be potentially undefined since it is optional according to the definition provided above.

Since this property is not mandatory I decided to set it as an optional property

Right, but what I am saying is that you're setting source.sidebarIcon and source could be undefined. Thus, you need to ensure that the object exists before you set a value on it.

graywolf336 · 2021-12-10T13:25:26Z

src/definition/package.json

 {
  "name": "@rocket.chat/apps-ts-definition",
-  "version": "1.28.0-alpha",
+  "version": "1.29.0-alpha.0",


Please revert this change. I know it is added automatically but it does not need to be committed by this PR.

graywolf336 · 2021-12-10T13:47:32Z

src/definition/rooms/IRoom.ts

    livechatData?: { [key: string]: any };
+    source?: {
+        sidebarIcon?: string;
+        type: OmnichannelSourceType;


According to the PR on Rocket.Chat side, this value is not required and is not always provided either. https://github.com/RocketChat/Rocket.Chat/pull/23912/files#diff-9716852bd39e3ddd6e0db833e8f0b3f3eed21a1aebbb3201741dd90e67c48965R99

AllanPazRibeiro · 2021-12-10T14:46:44Z

@graywolf336 I just commit some changes about your comments

murtaza98 · 2021-12-15T12:26:45Z

src/definition/rooms/IRoom.ts

    customFields?: { [key: string]: any };
    parentRoom?: IRoom;
    livechatData?: { [key: string]: any };
+    sideBarIconSource?: {


Considering this is an omnichannel property, so can we add this ILivechatRoom instead of the base room object?

From what I understand, this property should not be exposed actually.

I mean, the idea of that prop is to not be exposed so others can modify (we didn't enforce this), currently, the apps-engine sets the value on room creation based on apps info (but this happens only for omnichannel rooms) since that prop should not exists on other room types

@KevLehman

@ggazzo just created a PR with some modifcations on this PR #462 (review)

I'm a bit confused with the implementation. It basically allows N number of channel icons (different for each room). That means an app would be able to set a different icon for every room it creates (which is a feature that can generate confusion to the end user as well)

From what I remember, an app would be able to use one icon and then all rooms created by the app would use that icon (if the app wanted it). Was this changed? 👀

(Additionally, will we limit the ability to set custom room icons to only omnichannel rooms? I know this is what we need right now, but, with PR 462, the code validations would need to be removed if apps engine wanted to allow this in the future)

we don't know if the same app can't have more than one integration, today we develop one by one, but it's up to the developer. The app will decide this when creating the room, this is a little less "plug and play", but at the same time more flexible, it involves less metadata and the behavior is also simpler. Today we are going to store the data in the room, otherwise we would need to store the information in the app, collect and evaluate this data, I don't think we have that prepared (I'm not an apps-engine expert, but I think it would take an infinite amount of work and a code more "coupled")

Today we are not interested in changing room icons to other types, we think this is a mess actually, but who knows, maybe in the future this 'source' field can live in all rooms not just omnichannel.

I'm a bit confused with the implementation. It basically allows N number of channel icons (different for each room). That means an app would be able to set a different icon for every room it creates (which is a feature that can generate confusion to the end user as well)

From what I remember, an app would be able to use one icon and then all rooms created by the app would use that icon (if the app wanted it). Was this changed? 👀

(Additionally, will we limit the ability to set custom room icons to only omnichannel rooms? I know this is what we need right now, but, with PR 462, the code validations would need to be removed if apps engine wanted to allow this in the future)

murtaza98

Sorry a bit late to the review party here 😅

I had one piece of feedback. On all our omnichannel apps, we use this method to create an omnichannel room, so it would be awesome if we could add an extra param to this method to define the source of the channel when it is getting created... This way we don't have to make another call to DB to update the source post room creation

felipetomm · 2021-12-15T18:34:00Z

Sorry a bit late to the review party here sweat_smile

I had one piece of feedback. On all our omnichannel apps, we use this method to create an omnichannel room, so it would be awesome if we could add an extra param to this method to define the source of the channel when it is getting created... This way we don't have to make another call to DB to update the source post room creation

Wow!!! That's very awesome if you can add this info. Is an very important info to get metrics about customers channels, like others behaviors. At moment we identify channels by customFields of every Omnichannel Apps (and this is so rong)... A time ago i make an question about this here when Apps Source Type were implemented.
Thanks for note this @murtaza98 . We are looking forward to it :)

…ub.com/RocketChat/Rocket.Chat.Apps-engine into feat-adding-custom-sidebar-icon-method

AllanPazRibeiro · 2021-12-16T19:39:44Z

Folks,

I just committed a few modifications along with @ggazzo modifications to add the source as a parameter on LiveChat's create room method as @murtaza98 mentioned.

marking whose is in this thread:
@graywolf336 @ggazzo @felipetomm @murtaza98 @KevLehman @d-gubert

lgtm-com · 2021-12-16T20:57:48Z

This pull request introduces 1 alert when merging e698efe into 756bfff - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

murtaza98

Thanks @AllanPazRibeiro It looks good to me!! Really looking forward to the release of this one 🚀

AllanPazRibeiro added 2 commits December 9, 2021 14:16

feat: adding set custom icon method

5887668

Merge branch 'alpha' of https://github.com/RocketChat/Rocket.Chat.App…

ef2d23e

…s-engine into feat-adding-custom-sidebar-icon-method

AllanPazRibeiro mentioned this pull request Dec 9, 2021

[NEW] Show Omnichannel room icon based on source definition RocketChat/Rocket.Chat#23912

Merged

AllanPazRibeiro changed the title ~~Feat adding custom sidebar icon method~~ [NEW] adding custom sidebar icon method Dec 9, 2021

AllanPazRibeiro self-assigned this Dec 9, 2021

AllanPazRibeiro requested review from d-gubert, graywolf336 and marceloschmidt December 9, 2021 19:14

graywolf336 reviewed Dec 10, 2021

View reviewed changes

src/server/accessors/RoomBuilder.ts Outdated Show resolved Hide resolved

chore: removing unnecessary space

fd6c9eb

graywolf336 reviewed Dec 10, 2021

View reviewed changes

chore: changing propertie names and ensure it before assign

ed9ad07

Review Proposal

858fad5

murtaza98 reviewed Dec 15, 2021

View reviewed changes

murtaza98 suggested changes Dec 15, 2021

View reviewed changes

ggazzo and others added 7 commits December 16, 2021 09:59

PR review

26579a1

feat: adding source parameter on live chat create room

9c00609

chore: rewinding the version name

dbd4e70

Merge branch 'alpha' into feat-adding-custom-sidebar-icon-method

3ee8510

fix: fixing unit tests

7a417ce

Merge branch 'feat-adding-custom-sidebar-icon-method' of https://gith…

e051ac8

…ub.com/RocketChat/Rocket.Chat.Apps-engine into feat-adding-custom-sidebar-icon-method

fix: fixing a merge conflict

93e4eb8

feat: removing getters and setters of source icon

e698efe

murtaza98 approved these changes Dec 17, 2021

View reviewed changes

Change the parameter in createRoom for more flexibility

c119122

ggazzo approved these changes Dec 20, 2021

View reviewed changes

ggazzo merged commit 073efca into alpha Dec 20, 2021

ggazzo deleted the feat-adding-custom-sidebar-icon-method branch December 20, 2021 14:38

murtaza98 mentioned this pull request Dec 21, 2021

[NEW] Add new icons for omni source channels RocketChat/fuselage#597

Closed

8 tasks

Comments

Conversation

AllanPazRibeiro commented Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What? ⛵

Why? 🤔

Links 🌎

PS 👀

Uh oh!

CLAassistant commented Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

graywolf336 commented Dec 10, 2021

Uh oh!

AllanPazRibeiro commented Dec 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AllanPazRibeiro commented Dec 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

murtaza98 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipetomm commented Dec 15, 2021

Uh oh!

AllanPazRibeiro commented Dec 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgtm-com bot commented Dec 16, 2021

Uh oh!

murtaza98 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

AllanPazRibeiro commented Dec 9, 2021 •

edited

Loading

CLAassistant commented Dec 9, 2021 •

edited

Loading

AllanPazRibeiro commented Dec 10, 2021 •

edited

Loading

murtaza98 left a comment •

edited

Loading

AllanPazRibeiro commented Dec 16, 2021 •

edited

Loading