Implement #27 #36
Loading…
Reference in a new issue
No description provided.
Delete branch "lda/telodendria:implement-27"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Implements #27.
Please review the developer certificate of origin:
the right to submit it under the open source licenses of the
Telodendria project; or
my knowledge, is covered under an appropriate open source license and
I have the right under that license to submit that work with
modifications, whether created in whole or in part by me, under the
Telodendria project license; or
who certified (1), (2), or (3), and I have not modified it.
made public and that a record of the contribution—including all
personal information I submit with it—is maintained indefinitely
and may be redistributed consistent with this project or the open
source licenses involved.
origin, and I certify that I have permission to submit this patch
under the conditions specified in it.
CHANGELOG.md
6f14d0eef3CONTRIBUTING.md
. 36c07ed17dsend-patch
andtp
. See #20. 5067b5bcf0@ -0,0 +116,4 @@
}
finish:
if (user)
This shouldn't be necessary. It should suffice to just
UserUnlock()
here without first checking forNULL
, becauseUserUnlock()
should have aNULL
check in it already. I am going off of memory on that one, but if I'm wrong,UserUnlock()
should have this check instead.@ -0,0 +120,4 @@
{
UserUnlock(user);
}
if (removed)
Same as above.
@ -0,0 +50,4 @@
if ((method != HTTP_DELETE) && (method != HTTP_PUT))
{
char * msg = "Route only supports DELETE and PUT as for now.";
Is there a reason for declaring a variable here instead of just passing the string literal into
MatrixErrorCreate()
?Honestly, it's merely just to keep it all under 80 columns without having to do line-breaks in the middle of a function call.
Fair enough, I'm good with that. It doesn't look bad by any means, the code is still readable. Everyone has their own style and I have no real problems with this one.
@ -0,0 +96,4 @@
if (method == HTTP_DELETE)
{
char * str;
Again, why use an intermediate variable here?
@ -205,0 +207,4 @@
* .Fn UserDeactivate ,
* but saves the reason and admin that deactivated the user
*/
extern int UserDeactivateWithInfo(User *, char *, char *);
Is a separate function really necessary? Shouldn't we just modify
UserDeactivate()
to take the additional params?Considering how easy refactoring the code to use the proper
UserDeactivate
signature(which, to be frank, I should have done sooner), I think it's fine to change a few lines.This looks pretty good. I will read the code more closely and test it when I have time. In the meantime, take a look at my comments and see what you feel is relevant. If you disagree with something, that's fine, we can certainly discuss it!
@ -127,3 +127,3 @@
}
if (!UserDeleteTokens(user, NULL) || !UserDeactivate(user))
if (!UserDeleteTokens(user, NULL) || !UserDeactivate(user, NULL, NULL))
I think we should set these parameters instead of having them be
NULL
. Thefrom
should be the user, and thereason
can just be hard-coded if necessary.Oh nevermind, I see that the default behavior of this function is to set it to the user's own name.