Conversation
Codecov Report
@@ Coverage Diff @@
## master #195 +/- ##
==========================================
+ Coverage 56.97% 57.06% +0.08%
==========================================
Files 76 77 +1
Lines 2536 2599 +63
Branches 362 368 +6
==========================================
+ Hits 1445 1483 +38
- Misses 1091 1116 +25
Continue to review full report at Codecov.
|
Apply (Abstract) Factory pattern here, we can extend this interface to add other types of users.
| return new UserBuilder(data); | ||
| } | ||
|
|
||
| public finish(builder: IMessageBuilder | ILivechatMessageBuilder | IRoomBuilder | IUserBuilder): Promise<string> { |
There was a problem hiding this comment.
This keeps haunting us... I wonder if it isn't time we stopped following this start/finish pattern 🤔
There was a problem hiding this comment.
Should we also remove the IUserBuilder and UserBuilder from the code base?
There was a problem hiding this comment.
No, I don't think so. I think for now we just enable Apps to call a modify.getCreator().finishUser(builder: IUserBuilder) - this is going to make it possible to stop adding types on the finish method. What do you think?
There was a problem hiding this comment.
Do you mean make different finish*() methods standalone?
There was a problem hiding this comment.
@d-gubert One thing to note is that if you go that route, you will have to introduce a breaking change and all of the previous Apps will no longer work on new installations. The reason I say you will "have to" is because if you have two places for them to do it then it's not a clear and concise. If you don't want this method to be so long, then create a specific type above for it like:
type BuilderFinisher = IMessageBuilder | ILivechatMessageBuilder | IRoomBuilder | IUserBuilder;
Then in the finish method do public finish(builder: BuilderFinisher): Promise<string>
There was a problem hiding this comment.
@graywolf336 I think that could be a solution for now :)
But I don't think that introducing new ways of achieving things in the engine should require breaking changes - we can always deprecate APIs and keep them around for compatibility reasons until the time comes for the next major version. Even if it means having "two places for them to do it then it's not clear and concise". This could be solved by improving our docs effort (which we definitely have to)
Do you mean make different
finish*()methods standalone?
@lolimay that's what I was thinking, but let's maybe leave this change for another PR
|
@d-gubert @lolimay Question. Rocket.Chat Servers are billed for their seats on Cloud and when it comes to App Subscriptions. So, us enabling Apps to create users raises the question of whether the users created by Apps are billable or not. What are your thoughts on this? Because if a developer decided to go rough, they could create a lot of users and then cause the Server to be billed for a ton of users. |
5 participants
App. Base the permission of this role of thebotrole, since they will be very similar👉 The PR in the Rocket.Chat side: RocketChat/Rocket.Chat#15896