Move logic for two functions:
- removing user from thread
- switch user role between user and admin
into lib shares, so they could be used also in web app.
Details
Details
Promoting user to an admin and removing the user from a thread should work exactly as before
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- jacek/member-callbacks-lib
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
I don't have that much context here. My only suggestion would be to make this diff smaller. @ashoat -- When moving code, do you suggest we break each function into its own diff? Rather than moving multiple functions? Or is that a bit of overkill? I personally find it harder to review with multiple functions.
I looked a the https://www.notion.so/commapp/Diff-review-notes-b4cb81127f0d4269a361e6cf5a438707 but there wasn't anything about moving multiple functions.
Comment Actions
I think we should think about what is easier to review. In this case it would be easier to review the diff if only one function has been moved at a time. But sometimes, moving all of them at once makes more sense, as the number of diffs might become too big. What makes this diff a good fit for moving one at a time is that we're not just moving, but also creating the function (we had just code blocks). Phabricator marks the code that is just moved, but it can't know that when it was formatted. In this case most of switchMemberAdminRoleInThread is marked as move, so it is safer to review.
But overall, this diff is quite simple and I don't think it is worth to split it now.
| lib/shared/thread-utils.js | ||
|---|---|---|
| 1275–1276 | On one hand we can make this function easier to use in some contexts by changing the api to require threadId and memberId, but on the other, it might be possible to use the parameters in wrong order. So the current implementation is safer, so for now we can keep it and reconsider later, when trying to use this function in some other place. | |