fix: correct delete messages permission behavior in Administration and resolve permissions index issue

[SinghaAnirban005]

Brief Title

Resolve Delete permission behaviour in Administration

Acceptance Criteria fulfillment

• [x] Ensure normal users cannot see the delete icon when deleteOwnMessage is set to false by admin

• [x] Align permission behaviour with the hierarchy: 1. forcedeletemessage 2. deletemessage 3. deleteOwnMessage

• [x] Fix visibility inconsistencies in delete icon functionality.

• [x] Fix permission index number inconsistencies by using _id across different server versions in Rocket Chat

Fixes #749 #778

Video/Screenshots

https://github.com/user-attachments/assets/b855e5c3-6be7-4d9e-9214-a6ad71567d15

PR Test Details

Note: The PR will be ready for live testing at https://rocketchat.github.io/EmbeddedChat/pulls/pr-767 after approval. Contributors are requested to replace <pr_number> with the actual PR number.

[SinghaAnirban005]

It was reported to me by a fellow contributor that , on setting permissions using index number from array, it was leading to inconsistencies because different users were using diff RC server versions which lead to difference in the permissions array index values , thus i modified to using _id instead of index number . Now this would lead to uniform behaviour across all server versions.

[Spiral-Memory]

Can you change the PR title accordingly..

[SinghaAnirban005]

@Spiral-Memory have memoized the permission thing as per your suggestion

[Spiral-Memory]

Memoizing alone is not enough; we need to ensure that this calculation never occurs again at any point during rendering. Maybe @abirc8010 , @SinghaAnirban005 , @devanshkansagra , @dhairyashiil , please look into this #778 . If you can figure out a better solution, please do. If not, you all should first test that this calculation only takes place when the embedded chat first renders and does not get repeated regardless of subsequent re-renders. Once all four of you confirm, we are good to merge.

Please treat this as high priority, as we need to make it version-resistant.

[abirc8010]

Hey @SinghaAnirban005, I noticed that the API is fetching the permissions twice. I believe this happens because getMessagesAndRoles is called from both ChatHeader.js and ChatBody.js.

I think we can separate the fetchAndSetPermissions call from getMessagesAndRoles and export it from useFetchChatData. Then, we can call it from ChatLayout.js like this:

 const anonymousMode = ECOptions?.anonymousMode;
  const { fetchAndSetPermissions } = useFetchChatData();
  useEffect(() => {
    RCInstance.auth.onAuthChange((user) => {
      if (user) {
        fetchAndSetPermissions();
      } else {
        fetchAndSetPermissions(anonymousMode);
      }
    });
  }, [RCInstance, anonymousMode, fetchAndSetPermissions]);

Also, as @devanshkansagra mentioned, I think we can use binary search instead of creating a map out of the entire response.

[SinghaAnirban005]

Sure , will let you know once I implement and test your suggestions . Thank You !!

[devanshkansagra]

Hey @Spiral-Memory, @abirc8010, @SinghaAnirban005 , I have one solution, by using HashMap(or simply Map)and probably would get result in O(1) time complexity

What are your thoughts on this?

[SinghaAnirban005]

Exactly @devanshkansagra

[SinghaAnirban005]

Because if we try to binary search on that permissions array it would involve sorting and then looking up for that particular _id

[devanshkansagra]

using hashmap we would search for permission and would get the index in O(1). and it would be independent of new array index when the new rocketchat release

[SinghaAnirban005]

We just need to make sure that the computation happens only once

[devanshkansagra]

yepp

[SinghaAnirban005]

using hashmap we would search for permission and would get the index in O(1). and it would be independent of new array index when the new rocketchat release

Exactly , in my PR i implemented those but it was getting computed many times so , i am updating those

[SinghaAnirban005]

Hey @SinghaAnirban005, I noticed that the API is fetching the permissions twice. I believe this happens because getMessagesAndRoles is called from both ChatHeader.js and ChatBody.js.

I think we can separate the fetchAndSetPermissions call from getMessagesAndRoles and export it from useFetchChatData. Then, we can call it from ChatLayout.js like this:

 const anonymousMode = ECOptions?.anonymousMode;
  const { fetchAndSetPermissions } = useFetchChatData();
  useEffect(() => {
    RCInstance.auth.onAuthChange((user) => {
      if (user) {
        fetchAndSetPermissions();
      } else {
        fetchAndSetPermissions(anonymousMode);
      }
    });
  }, [RCInstance, anonymousMode, fetchAndSetPermissions]);

Also, as @devanshkansagra mentioned, I think we can use binary search instead of creating a map out of the entire response.

Also abir's suggestion was correct in this case !!

[devanshkansagra]

So what @SinghaAnirban005, binary search or HashMap? coz I read your comments and I didn't got that you are talking about which approach?

[SinghaAnirban005]

So what @SinghaAnirban005, binary search or HashMap? coz I read your comments and I didn't got that you are talking about which approach?

i was referring to the reason that abir pointed out regarding the permissions getting computed multiple times ... and we are good to go with the Map approach

[devanshkansagra]

Ohh okay 👍

[devanshkansagra]

So what @SinghaAnirban005, binary search or HashMap? coz I read your comments and I didn't got that you are talking about which approach?

i was referring to the reason that abir pointed out regarding the permissions getting computed multiple times ... and we are good to go with the Map approach

Let other members and @Spiral-Memory can share their points also regarding this

[abirc8010]

Because if we try to binary search on that permissions array it would involve sorting and then looking up for that particular _id

Hey @SinghaAnirban005 I rechecked the response from the endpoint and noticed that the response is sorted according to the id . You can once verify that too.

[devanshkansagra]

Then we have two approaches to go with

[Spiral-Memory]

Hey @Spiral-Memory, @abirc8010, @SinghaAnirban005 , I have one solution, by using HashMap(or simply Map)and probably would get result in O(1) time complexity

What are your thoughts on this?

He's already using map only if i can see correctly.. but for creating the map, the T.C will be O(n), for fetching from the map, it might be O(1) or O(logN) depending on collision..

These are fine, the problem is, the computation should only happen once and not on any other re-render

This is something you'll have to verify before i merge this

[devanshkansagra]

Hey @Spiral-Memory, @abirc8010, @SinghaAnirban005 , I have one solution, by using HashMap(or simply Map)and probably would get result in O(1) time complexity

What are your thoughts on this?

He's already using map only if i can see correctly.. but for creating the map, the T.C will be O(n), for fetching from the map, it might be O(1) or O(logN) depending on collision..

Hey @Spiral-Memory, I am not talking about array map function, instead I am talking about Map datastructure

[SinghaAnirban005]

@devanshkansagra i have used the data structure you are referring to

[devanshkansagra]

Ohh then it would be efficient.

[SinghaAnirban005]

@abirc8010 not really sure if the premissions are sorted based on the _id

[devanshkansagra]

It maybe sorted by updatedAt

[SinghaAnirban005]

It maybe sorted by updatedAt

i believe that's not relevant

[abirc8010]

not really sure if the premissions are sorted based on the _id

Apologies for the confusion — I believe I may have logged the map instead of the response. Thank you for verifying it.

[SinghaAnirban005]

Now, on login permissions list mapping is generated just once

https://github.com/user-attachments/assets/2d81aabf-d12b-40b1-9673-adb3ca42d147

[SinghaAnirban005]

General behaviour after changes

https://github.com/user-attachments/assets/97f39f6f-52d4-4eb6-a09f-9ab729666a86