Rocket.Chat.Apps-engine icon indicating copy to clipboard operation
Rocket.Chat.Apps-engine copied to clipboard

[NEW FEATURE] Allows Deletion of message when a messageID is given

Open kulkarnigaurav38 opened this issue 2 years ago • 11 comments

What? :boat:

Added: The method to delete message when a messageId is given.

Why? :thinking:

Links :earth_americas:

Closes issue #445

PS :eyes:

Please suggest any changes if required, thanks!

kulkarnigaurav38 avatar Jan 22 '22 12:01 kulkarnigaurav38

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Jan 22 '22 12:01 CLAassistant

Should probably be a new permission added to the entire Apps Framework for message deletion that has to be explicitly declared and then on the UI we display a warning that the app can remove messages?

graywolf336 avatar Jan 23 '22 02:01 graywolf336

Should probably be a new permission added to the entire Apps Framework for message deletion that has to be explicitly declared and then on the UI we display a warning that the app can remove messages?

Hey, if you could guide me as to which files to look for this feature I can work on it in an another PR as this one solves for only when a messageID is provided.

kulkarnigaurav38 avatar Jan 23 '22 04:01 kulkarnigaurav38

Hi @kulkarnigaurav38 Thanks for the contribution :1st_place_medal: This is indeed a useful tool to have on apps-engine, so, I'd be happy to assist you to get this feature released :smile:

From what I see here, there are 2 things that we'd need to get this feature to work

  1. Firstly, we'd need to add a method to delete the message on the core Repo. (Hint: Look into this file). Please mention the core repo PR in this PR once it's ready.
  2. Next, as Bradley suggested, we'd perhaps need to add new permission for apps to use this particular feature. For this, we'd need to make changes on both Apps-engine and the core Repo. (For inspiration, you can look into how existing read, write permission works on Messages in this file on Apps-Engine & then just update the core repo's JSON file with the user-friendly message about what the permission is for, like this)

Let me know if you have any further questions.

murtaza98 avatar Feb 08 '22 07:02 murtaza98

@kulkarnigaurav38 Any news here?

murtaza98 avatar Feb 21 '22 10:02 murtaza98

Hey @murtaza98, I will put in a PR in a few hours sorry have exams going on in college.

kulkarnigaurav38 avatar Feb 22 '22 04:02 kulkarnigaurav38

Hey @murtaza98, I will put in a PR in a few hours sorry have exams going on in college.

Hey man! No worries at all, I only pinged you since the release cut was scheduled on 21st Feb. Don't feel any pressure to do this right now in the middle of your exams. We can make this part of the release next month. All the best :+1:

murtaza98 avatar Feb 22 '22 04:02 murtaza98

The PR in the main repo : Link, please look into it.

kulkarnigaurav38 avatar Mar 11 '22 10:03 kulkarnigaurav38

Hi @kulkarnigaurav38 There was a permission issue with this PR where the new delete permission wasn't getting applied while using the new delete message method from apps. So I added that fix over #507 Feel free to apply the change on this PR & we can close that if you want & keep this as the main PR

murtaza98 avatar May 15 '22 15:05 murtaza98

Hi @kulkarnigaurav38 There was a permission issue with this PR where the new delete permission wasn't getting applied while using the new delete message method from apps. So I added that fix over #507 Feel free to apply the change on this PR & we can close that if you want & keep this as the main PR

Made the changes, thanks for correcting it.

kulkarnigaurav38 avatar May 15 '22 17:05 kulkarnigaurav38

@kulkarnigaurav38 Thanks, man! This PR looks good to me now. Do you also mind resolving my comment over here on the connected PR of the main repo? You can check out this PR if you like where I've applied my suggestion

murtaza98 avatar May 16 '22 06:05 murtaza98