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] Bridge to get all users crated by app and also delete them #27877

Merged
merged 8 commits into from
Feb 7, 2023

Conversation

albuquerquefabio
Copy link
Contributor

@albuquerquefabio albuquerquefabio commented Jan 27, 2023

Proposed changes (including videos or screenshots)

We're working on MS Teams bridge, to make it usable we have created a new method for the apps engine to create new users and we'll also need to delete all when we uninstall the app. Therefore, we created this bridge to cover the action of deleting users on the app side.

Issue(s)

Steps to test or reproduce

Further comments

@debdutdeb
Copy link
Member

debdutdeb commented Jan 27, 2023

Should we depend on the app developer to do this or have the engine take care of this, like the other stuff- persistence, its own user etc?

https://github.com/RocketChat/Rocket.Chat.Apps-engine/blob/d017731aff04890c40750edeb5ad007b25457619/src/server/AppManager.ts#L549-L564

I like the general idea of having the capability of deleting a user, but more on the generic path than just deleteUsersCreatedByApp, like deleteUser? not sure

@codecov
Copy link

codecov bot commented Jan 27, 2023

Codecov Report

Merging #27877 (a8d53ad) into develop (c28bb08) will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #27877      +/-   ##
===========================================
- Coverage    42.08%   42.05%   -0.04%     
===========================================
  Files          848      848              
  Lines        17558    17558              
  Branches      2078     2078              
===========================================
- Hits          7390     7384       -6     
- Misses        9890     9896       +6     
  Partials       278      278              
Flag Coverage Δ
e2e 42.05% <ø> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@albuquerquefabio
Copy link
Contributor Author

Should we depend on the app developer to do this or have the engine take care of this, like the other stuff- persistence, its own user etc?

https://github.com/RocketChat/Rocket.Chat.Apps-engine/blob/d017731aff04890c40750edeb5ad007b25457619/src/server/AppManager.ts#L549-L564

I like the general idea of having the capability of deleting a user, but more on the generic path than just deleteUsersCreatedByApp, like deleteUser? not sure

So, we are preparing the Rocket.Chat and Apps Engine to support the App MS Teams bridge, these updates were necessary.

Let me explain some points:

  • We need to allow the apps engine to create only bot users;
  • We want to put the responsibility on the dev to call the method to exclude your bots;
  • When you uninstall the app it will run remove(user: IUser & { id: string }, appId: string) where appId is just to mount a logs notification, and we call a method deleteUser which will unsubscribe the user from many places;
  • getUserCreatedByApp is necessary to bring all users by appId and type, the next step is to run a loop and get each item to execute deleteUser step.
  • I believe that we can improve the name of the method, but we should explain that this method only deletes the bot or app user

@albuquerquefabio albuquerquefabio added this to the 6.0.0 milestone Jan 31, 2023
AllanPazRibeiro
AllanPazRibeiro previously approved these changes Feb 6, 2023
Copy link
Contributor

@AllanPazRibeiro AllanPazRibeiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@AllanPazRibeiro AllanPazRibeiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dionisio-bot dionisio-bot bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: needs QA labels Feb 7, 2023
@kodiakhq kodiakhq bot merged commit 34f0371 into develop Feb 7, 2023
@kodiakhq kodiakhq bot deleted the new/AD-117-delete-bot-user-created-by-app branch February 7, 2023 22:18
@sampaiodiego sampaiodiego mentioned this pull request Feb 17, 2023
@sampaiodiego sampaiodiego mentioned this pull request Mar 9, 2023
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: QA skipped stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants