Add gRPC methods for admin panel#10
Conversation
|
The latest Buf updates on your PR. Results from workflow Validate Pull Request / check_buf (pull_request).
|
|
Why are mod audit logs expected to be in the account server? That doesn't make a ton of sense to me. These are audit logs for moderator actions done on the admin panel, that feels like it would be much better suited to be stored in a database the admin panel maintains itself (it's fine for the admin panel to use a database for itself, the idea with removing the database connections was to remove it's direct connections to other services databases), and the admin panel would just update it's audit logs before/after the gRPC calls. I don't see a reason to keep these logs in the account server? Keeping the audit logs in the admin panel itself will also help keep everything centralized, rather than having logs spread across multiple services. For example another future goal of the admin panel is to move Miiverse moderation to it which will also need audit logs for it's admin panel actions |
|
Moving audit logs to the admin panel would mean that it wouldn't be possible for any other services to manage audit logs. (Nextjs is a frontend, it can't host a grpc server) Moving juxt mod to admin panel will not be possible. It was added to juxt web so mods don't have to go to a separate spreadsheet just to remove a post or ban someone (it resulted in no moderation being done, it was too much effort). Moving it to admin panel would reintroduce the problem. |
|
Would would any other service besides the admin panel need to manage audit logs? I don't see the logic here, nor do I see what the frontend has to do with this? I am aware that Next is a frontend, but that doesn't really change what I said? Currently, for example, the admin panel does this for updating a PNID: await PNID.updateOne({ pid: pid }, { $set: body });What I've said is that this would instead look more like: await AuditLog.create({ whatever }});
await grpcClientV2.modifyAccount(whatever); // however this would look with the new protobufsThe admin panel already maintains database connections, I don't see why it isn't possible to run a database along side the admin panel and connect to it the same way it already connects to the account server database, to manage its own audit logs or why any service other than the admin panel itself needs to know about admin panel audit logs, this feels like an odd mixing of responsibilities? Also no one is suggesting moving back to a spreadsheet for Juxtaposition moderation. I don't doubt that the old system resulted in a lack of moderation, but I do somewhat doubt that the crux of the issue was "opening another tab" and not "having to manually maintain a spreadsheet", that seems pretty extreme to me (especially since there's virtually no friction there outside of just a new tab being opened, since being logged into one service keeps you logged in to all of them) I am, of course, willing to hear the mods out on the Juxtaposition issue, but I do still think that at the minimum keeping the audit logs for admin panel actions on the admin panel makes more sense than trying to mix that into the responsibilities of the account server? To be 100% clear I am genuinely asking for clarifications here in case there's something I've missed, and am giving my view point on things |
|
My gut tells me that there will be more things that need to be logged, not just admin panel actions. Like deleting a juxt post or stripe tier changes. To me it makes sense to keep the the audit log close to the resource it's auditing. A pnid ban has a audit log in the same db. I would expect the same from juxt posts. Just have a separate That said, I don't really have the free time to come to a middleground. Could you take over on the account server changes and this pr? As for juxt mod, it's best if you talk to the moderation team there on how it will best be implemented. |
I agree that more things will need to be logged, that's why I was trying to have a dialogue about it to clarify the intentions and make sure we're using the best solution. To clarify, was the intent to store things like Juxtaposition information in this same audit log database on the account server? I ask because that was the feeling I got, and why I was bringing it up, and because Juxtaposition was brought up in the same sentence as Stripe here. I'm just asking so we're on the same page, and because later in your reply seems to imply that this isn't actually the case.
I also agree this makes sense. It seemed to me like the idea was to just dump all the audit stuff onto the account server, which is why I had concerns about the mixing of responsibilities. My logic was to keep the audit logs close to the tool that performed the action, but keeping the logs close to the data being affected is also fine logic that I'm happy with.
Sure. |
The intent was to store the audit log next to the resource it's auditing. And since admin panel only manages accound atm, it's only implemented for account.
Thanks 🙂 |
this field can be used to add additional metadata to an audit log
tracks the comments parent log ID, and comment ID, separately in case we ever need to target a specific audit log comment, such as for edits to a comments content
making this an enum rather than a string gives us type safety/hints in the implementation side of things
|
I did a few touch ups so far. Namely I swapped @DaniElectra I want to get you in on this as well for input on the design. There's a couple things I'm not to sure what the use is atm (like |
|
@DaniElectra poke |
I assume those would be for adding context about what information is being changed specifically (kinda like
I fully agree that we want to handle bans separately, that way we could also implement temporary / game-specific bans. I'm thinking on the following:
|
In that case they seem to be a little redundant when paired with
That looks good to me! |
DaniElectra
left a comment
There was a problem hiding this comment.
Looking pretty good! Only a couple of comments
DaniElectra
left a comment
There was a problem hiding this comment.
Outside of this, after resolving conflicts LGTM
Using "remove" can imply deleting the entry, "lift" is more clear Co-authored-by: Daniel López Guimaraes <112760654+DaniElectra@users.noreply.github.com>
|
Hm, it's complaining about renaming one of the RPCs as If we're keeping the rename we also have to rename |
Using It looks like we can get around this by disabling |
|
I'd say we just force merge it so that we don't have to mess with the current config I'll approve after the other RPC is renamed |
some files were renamed in PR #10 which causes build errors. these are acceptable though, we want these changes, so temporarily add this exception
Changes:
device_idis a unique ID for the deviceSome notes about the implementation:
Todos before it can be marked as ready for review:
deleteAccount?