Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding "UserEnabled" and "CreatedAt" member to the json output of a User #2523

Merged

Conversation

Lowaiz
Copy link

@Lowaiz Lowaiz commented Jun 1, 2022

Adding "UserEnabled" and "CreatedAt" member to the json output of a User in the admin/users and admin/users/<ID> web routes.

Copy link
Collaborator

@BlackDex BlackDex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not add those items there, since these are also used to return the user object to the Bitwarden clients.
It is better to add them to the generated json output within admin.rs instead.

Same goes for #2524, and in that case, i would probably suggest to merge them into one PR instead of two different ones.

@Lowaiz
Copy link
Author

Lowaiz commented Jun 1, 2022

I applied the changes. But I have one question, do we name those json members as in the users_overview functions of admin.rs or with Camel Case syntax as in the to_json method of the User class ?

@Lowaiz Lowaiz changed the title Adding "Enabled" member to the json output of a User Adding "user_enabled" and "created_at" member to the json output of a User Jun 1, 2022
@BlackDex
Copy link
Collaborator

BlackDex commented Jun 1, 2022

Since the rest of the json is CamelCase i think it is best to keep that the same in this case.

@Lowaiz Lowaiz changed the title Adding "user_enabled" and "created_at" member to the json output of a User Adding "UserEnabled" and "CreatedAt" member to the json output of a User Jun 2, 2022
@Lowaiz
Copy link
Author

Lowaiz commented Jun 2, 2022

Here is the Camel Case !

@BlackDex
Copy link
Collaborator

BlackDex commented Jun 2, 2022

Could you squash the commits into one commit? Else it will clutter the git-history a bit.
You could do that with (from the top of my head)

git reset --soft HEAD~4
git commit -a
git push origin +add_disabled_member_to_json_user

That should reset the commits to the starting point you were, and leave the changes you made.
Then you can commit that again, and do a force push so the latest commit will be the only commit here :) .

@Lowaiz Lowaiz force-pushed the add_disabled_member_to_json_user branch from af525f2 to 97f0955 Compare June 2, 2022 09:40
@Lowaiz
Copy link
Author

Lowaiz commented Jun 2, 2022

Squash done and thank you for the commands.

@Lowaiz Lowaiz force-pushed the add_disabled_member_to_json_user branch 3 times, most recently from a6dcca4 to ab87802 Compare June 2, 2022 12:47
src/api/admin.rs Outdated Show resolved Hide resolved
@Lowaiz Lowaiz force-pushed the add_disabled_member_to_json_user branch from ab87802 to e699160 Compare June 2, 2022 13:06
…ser in the admin/users and admin/users/<ID> web routes.
@Lowaiz Lowaiz force-pushed the add_disabled_member_to_json_user branch from 80a07a8 to dbd95e0 Compare June 2, 2022 13:14
@BlackDex
Copy link
Collaborator

BlackDex commented Jun 2, 2022

The build fails because it's not formatted correctly.
I suggest to install pre-commit and use that.
See: https://pre-commit.com/

This repo already has the correct pre-commit config there, all you need to do after you installed it is run pre-commit install, then run the --reset soft again and do a commit, this will check everything which is needed for the PR to be valid (in a format/clippy sense way at least)

@Lowaiz Lowaiz requested a review from BlackDex June 2, 2022 13:45
@dani-garcia dani-garcia merged commit 2f71a01 into dani-garcia:main Jun 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants