Interface to create Bot user through Apps.engine by Cool-fire · Pull Request #133 · RocketChat/Rocket.Chat.Apps-engine

Cool-fire · 2019-08-20T09:44:34Z

changes made:

Added a new Interface IUserGenerate for creating user
Added IUserBuilder to create a user
Added a new Interface create in IUserBridge which accepts IUserGenerate

PR for implementation Rocket.Chat: Link

codecov · 2019-08-20T09:47:19Z

Codecov Report

Merging #133 into master will increase coverage by 2.7%.
The diff coverage is 82.89%.

@@            Coverage Diff            @@
##           master     #133     +/-   ##
=========================================
+ Coverage    55.4%   58.11%   +2.7%     
=========================================
  Files          68       77      +9     
  Lines        2397     2583    +186     
  Branches      360      366      +6     
=========================================
+ Hits         1328     1501    +173     
- Misses       1069     1082     +13

Impacted Files	Coverage Δ
src/server/accessors/index.ts	`100% <100%> (ø)`
src/server/accessors/ModifyCreator.ts	`63.51% <16.66%> (-9.51%)`	⬇️
src/server/accessors/UserBuilder.ts	`95.16% <95.16%> (ø)`
src/server/errors/index.ts	`100% <0%> (ø)`
src/server/managers/index.ts	`100% <0%> (ø)`
src/server/marketplace/license/index.ts	`100% <0%> (ø)`
src/server/compiler/index.ts	`100% <0%> (ø)`
src/server/storage/index.ts	`100% <0%> (ø)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d0af34...0ac4eef. Read the comment docs.

Cool-fire · 2019-08-20T09:55:06Z

@d-gubert Please review

graywolf336

Looking great. Is there any chance you can add some tests for the user builder?

Cool-fire · 2019-10-02T16:32:06Z

sure @graywolf336 I will definitely add some test for user builder

graywolf336 · 2019-10-02T16:41:52Z

Thanks! I'll work with @d-gubert to see about getting this merged once you do.

graywolf336 · 2019-10-07T20:00:37Z

@Cool-fire let me know if you need help with anything

graywolf336

A few quality of developer experience and improvements, but overall a fantastic pull request and something we have been needing. Along with the requested changes, can you fix the merge conflicts? We separated out the accessor definitions into individual files instead of all being in one.

graywolf336 · 2019-10-07T23:49:20Z

src/definition/users/IUserGenerate.ts

@@ -0,0 +1,12 @@
+export interface IUserGenerate {


I'm not 100% certain about this interface name. What is the main reason for not putting this on the existing user? What about extending the IUser interface so that it accepts everything else along with the new fields that are just for when creating a user?

@graywolf336 The main reason for not extending the IUser is that there are some fields which are non-nullable like lastLoginAt, createdAt which I felt are to be extended unnecessarily while creating, So I have created a new Interface which is meant for the only generation of a user.

Great point, I see the validity in it now. Then I think we should change the name of the interface to something like IUserCreation, what do you think?

sure @graywolf336 I will change that as you suggested :)

Awesome, thank you! 😄

graywolf336 · 2019-10-07T23:51:02Z

src/server/accessors/ModifyCreator.ts

+    private _finishUser(builder: IUserBuilder): Promise<string> {
+        const result = builder.getUser();
+        delete result.id;
+        return this.bridges.getUserBridge().create(result, this.appId);


Almost wonder if we should do a check to ensure the user doesn't exist by the username, email, etc before we proceed to create the user that way we can gracefully handle the error instead of letting Rocket.Chat handle the error for us.

Yes, @graywolf336 it would be better if we can check a user exists by username, email..etc but I couldn't find a way to check whether it exists from apps-engine. Anyways I think the server handles the case where a user exists gracefully throwing the reason for the error in logs.

Then let's create it that way we can throw custom errors. The Apps Engine is an abstraction away from Rocket.Chat since the internals of Rocket.Chat can change at any point in time and we want the Apps Engine to be stable and consistent. Thus I would rather us do the extra work that way it is not a guessing game for App developers of what will happen and instead they can know up front. Does that make sense?

graywolf336 · 2019-10-07T23:51:34Z

src/server/accessors/UserBuilder.ts

+            throw new Error('The "name" property is required.');
+        }
+        if (!this.user.roles) {
+            throw new Error('The "roles" property is required.');


We almost need a way for Apps to get fetch the roles from the Rocket.Chat system 🤔

graywolf336 · 2019-10-07T23:52:18Z

src/definition/accessors/IModify.ts

+    getDisplayName(): string;
+
+    /**
+     * Sets the username for the user


Might want to add a warning here that the username must be unique.

graywolf336 · 2019-10-07T23:52:48Z

src/definition/accessors/IModify.ts

+    *
+    * @param email the email address of the user
+    */
+    setEmail(email: string): IUserBuilder;


The email provided here can not exist already in the system otherwise it will cause an error to arise.

CLAassistant · 2019-10-08T17:01:55Z

All committers have signed the CLA.

wreiske · 2019-10-08T18:23:14Z

Would it be possible to add the ability for an app to edit a user, as well as create it? That would be amazing! Please, and thank you :)

I'd like to be able to modify user roles from the apps-engine.

Cool-fire · 2019-10-08T20:04:58Z

@graywolf336 can you please review the PR, I think I have added necessary tests for UserBuilder and resolved merge conflicts. Please let me know if any changes are needed.

graywolf336 · 2019-10-08T20:26:33Z

@Cool-fire will do, probably will be tomorrow or Thursday before I can.

Based upon your experience with this, how hard would it be to implement what @wreiske requested?

Cool-fire · 2019-10-08T20:48:36Z

@graywolf336 I think this pr gives the apps ability to create users. And I think it would be simple to make an interface like IUserModifier which has the ability to edit a user details but I think we have to look take a look on permissions to edit a user roles since I think that can be only done by an admin or the same user, I am wondering if the app developers have to use an admin role user to edit other user details.

d-gubert · 2020-02-24T15:27:47Z

We've merged part of this implementation on #195, along with its Rocket.Chat counterpart.

Thanks again @Cool-fire ! 🤗

Cool-fire added 4 commits August 6, 2019 16:44

add user creator

3b6caaf

add documentation for user creator

44a6c9b

Merge Develop

35544e3

fix Linting

81d5d6c

d-gubert self-requested a review August 20, 2019 17:05

Cool-fire mentioned this pull request Aug 21, 2019

Create PR's to Rocket.chat apps.engine upstream and Rocket.chat WideChat/Rocket.Chat.Android#654

Closed

graywolf336 suggested changes Oct 2, 2019

View reviewed changes

graywolf336 suggested changes Oct 7, 2019

View reviewed changes

create IUserCreator file

ea4c60e

Cool-fire requested a review from graywolf336 October 8, 2019 17:03

create IUserBuilder file

9ff1684

Cool-fire added 4 commits October 9, 2019 00:08

add basic tests

f9cc8bd

add tests for userbuilder

ed4b066

make changes to User Builder test

d4d415c

make changes to User Builder test

0ac4eef

This was referenced Nov 29, 2019

Auto create a user for app #195

Merged

[NEW] Create a user for the Apps during installation RocketChat/Rocket.Chat#15896

Merged

d-gubert closed this Feb 24, 2020

Comments

Conversation

Cool-fire commented Aug 20, 2019 • edited by shiqimei Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

changes made:

Uh oh!

codecov bot commented Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Cool-fire commented Aug 20, 2019

Uh oh!

graywolf336 left a comment

Choose a reason for hiding this comment

Uh oh!

Cool-fire commented Oct 2, 2019

Uh oh!

graywolf336 commented Oct 2, 2019

Uh oh!

graywolf336 commented Oct 7, 2019

Uh oh!

graywolf336 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!

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!

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!

CLAassistant commented Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wreiske commented Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cool-fire commented Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

graywolf336 commented Oct 8, 2019

Uh oh!

Cool-fire commented Oct 8, 2019

Uh oh!

d-gubert commented Feb 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Cool-fire commented Aug 20, 2019 •

edited by shiqimei

Loading

codecov bot commented Aug 20, 2019 •

edited

Loading

graywolf336 left a comment •

edited

Loading

CLAassistant commented Oct 8, 2019 •

edited

Loading

wreiske commented Oct 8, 2019 •

edited

Loading

Cool-fire commented Oct 8, 2019 •

edited

Loading