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

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

Open
wants to merge 5 commits into
base: alpha
Choose a base branch
from

Conversation

kulkarnigaurav38
Copy link

What? β›΅

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

Why? πŸ€”

Links 🌎

Closes issue #445

PS πŸ‘€

Please suggest any changes if required, thanks!

@CLAassistant
Copy link

CLAassistant commented Jan 22, 2022

CLA assistant check
All committers have signed the CLA.

@graywolf336
Copy link
Contributor

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?

@kulkarnigaurav38
Copy link
Author

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.

@murtaza98
Copy link
Contributor

murtaza98 commented Feb 8, 2022

Hi @kulkarnigaurav38 Thanks for the contribution πŸ₯‡ This is indeed a useful tool to have on apps-engine, so, I'd be happy to assist you to get this feature released πŸ˜„

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
Copy link
Contributor

@kulkarnigaurav38 Any news here?

@kulkarnigaurav38
Copy link
Author

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

@murtaza98
Copy link
Contributor

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 πŸ‘

@kulkarnigaurav38
Copy link
Author

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

@murtaza98
Copy link
Contributor

murtaza98 commented May 15, 2022

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

@kulkarnigaurav38
Copy link
Author

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.

@murtaza98
Copy link
Contributor

@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

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.

4 participants