Implement #27 #36

Merged
jordan merged 37 commits from lda/telodendria:implement-27 into master 2023-09-25 13:39:24 +00:00
Contributor

Implements #27.

Please review the developer certificate of origin:

  1. The contribution was created in whole or in part by me, and I have
    the right to submit it under the open source licenses of the
    Telodendria project; or
  2. The contribution is based upon a previous work that, to the best of
    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
  3. The contribution was provided directly to me by some other person
    who certified (1), (2), or (3), and I have not modified it.
  4. I understand and agree that this project and the contribution are
    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.
  • I have read the Telodendria Project development certificate of
    origin, and I certify that I have permission to submit this patch
    under the conditions specified in it.
Implements #27. --- Please review the developer certificate of origin: 1. The contribution was created in whole or in part by me, and I have the right to submit it under the open source licenses of the Telodendria project; or 1. The contribution is based upon a previous work that, to the best of 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 1. The contribution was provided directly to me by some other person who certified (1), (2), or (3), and I have not modified it. 1. I understand and agree that this project and the contribution are 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. - [x] I have read the Telodendria Project development certificate of origin, and I certify that I have permission to submit this patch under the conditions specified in it.
lda added 30 commits 2023-09-17 18:15:26 +00:00
Fixes #33.

Co-authored-by: LoaD Accumulator <lda@freetards.xyz>
Reviewed-on: Telodendria/telodendria#35
Co-authored-by: LoaD Accumulator <lda@noreply.git.telodendria.io>
Co-committed-by: LoaD Accumulator <lda@noreply.git.telodendria.io>
lda added 1 commit 2023-09-17 18:18:21 +00:00
lda added 1 commit 2023-09-17 18:20:59 +00:00
lda added 1 commit 2023-09-17 18:48:09 +00:00
lda added 1 commit 2023-09-17 18:50:34 +00:00
lda added 1 commit 2023-09-20 06:11:54 +00:00
jordan reviewed 2023-09-20 19:51:19 +00:00
@ -0,0 +116,4 @@
}
finish:
if (user)
Owner

This shouldn't be necessary. It should suffice to just UserUnlock() here without first checking for NULL, because UserUnlock() should have a NULL check in it already. I am going off of memory on that one, but if I'm wrong, UserUnlock() should have this check instead.

This shouldn't be necessary. It should suffice to just `UserUnlock()` here without first checking for `NULL`, because `UserUnlock()` should have a `NULL` check in it already. I am going off of memory on that one, but if I'm wrong, `UserUnlock()` should have this check instead.
lda marked this conversation as resolved
jordan reviewed 2023-09-20 19:51:36 +00:00
@ -0,0 +120,4 @@
{
UserUnlock(user);
}
if (removed)
Owner

Same as above.

Same as above.
lda marked this conversation as resolved
jordan reviewed 2023-09-20 19:53:11 +00:00
@ -0,0 +50,4 @@
if ((method != HTTP_DELETE) && (method != HTTP_PUT))
{
char * msg = "Route only supports DELETE and PUT as for now.";
Owner

Is there a reason for declaring a variable here instead of just passing the string literal into MatrixErrorCreate()?

Is there a reason for declaring a variable here instead of just passing the string literal into `MatrixErrorCreate()`?
Author
Contributor

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.

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.
Owner

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.

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.
jordan marked this conversation as resolved
jordan reviewed 2023-09-20 19:57:51 +00:00
@ -0,0 +96,4 @@
if (method == HTTP_DELETE)
{
char * str;
Owner

Again, why use an intermediate variable here?

Again, why use an intermediate variable here?
lda marked this conversation as resolved
jordan reviewed 2023-09-20 19:59:36 +00:00
@ -205,0 +207,4 @@
* .Fn UserDeactivate ,
* but saves the reason and admin that deactivated the user
*/
extern int UserDeactivateWithInfo(User *, char *, char *);
Owner

Is a separate function really necessary? Shouldn't we just modify UserDeactivate() to take the additional params?

Is a separate function really necessary? Shouldn't we just modify `UserDeactivate()` to take the additional params?
Author
Contributor

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.

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.
lda marked this conversation as resolved
Owner

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!

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!
lda added 1 commit 2023-09-21 06:08:57 +00:00
lda added 1 commit 2023-09-21 06:24:56 +00:00
jordan reviewed 2023-09-25 13:37:21 +00:00
@ -127,3 +127,3 @@
}
if (!UserDeleteTokens(user, NULL) || !UserDeactivate(user))
if (!UserDeleteTokens(user, NULL) || !UserDeactivate(user, NULL, NULL))
Owner

I think we should set these parameters instead of having them be NULL. The from should be the user, and the reason can just be hard-coded if necessary.

I think we should set these parameters instead of having them be `NULL`. The `from` should be the user, and the `reason` can just be hard-coded if necessary.
Owner

Oh nevermind, I see that the default behavior of this function is to set it to the user's own name.

Oh nevermind, I see that the default behavior of this function is to set it to the user's own name.
jordan marked this conversation as resolved
jordan merged commit cb41716bf3 into master 2023-09-25 13:39:24 +00:00
jordan referenced this pull request from a commit 2023-09-25 13:39:25 +00:00
lda referenced this pull request from a commit 2023-09-25 20:41:23 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Telodendria/Telodendria#36
No description provided.