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

Feat/946/default admin #964

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Feat/946/default admin #964

wants to merge 20 commits into from

Conversation

g-pechorin
Copy link

Pull Request Contents

♻️ Refactor

Description

Adds a management command, is invoked on setup, which creates a default admin user for the carrot system iff there are 0 users in the database at present.

Related Issues or other material

  • Closes #

closes #946

Screenshots, example outputs/behaviour etc.

✅ Added/updated tests?

  • This PR contains relevant tests
    • Or doesn't need to per the below explanation

This functionality relies on reading the django database, performing a single comparison - a test seems redundant.

  • I've completed all actions and tasks detailed in the PR Template

Copy link

github-actions bot commented Mar 5, 2025

Tests Skipped Failures Errors Time
25 1 💤 6 ❌ 0 🔥 3.939s ⏱️

Copy link
Contributor

@Karthi-DStech Karthi-DStech left a comment

Choose a reason for hiding this comment

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

Hi Peter,

The code looks great! I just have a small suggestion and let me know your thoughts.

Looking forward from you.

Thanks and regards,
Karthik

g-pechorin and others added 6 commits March 12, 2025 18:19
adjusted the log messages for "add user" and "don't add user"
the "create default super user" test expectation needed to be updated to account for the new log message which includes the password
@Karthi-DStech
Copy link
Contributor

Karthi-DStech commented Mar 13, 2025

Thanks for your changes @g-pechorin. I can approve your PR once I get the request access.

@Karthi-DStech Karthi-DStech self-requested a review March 13, 2025 13:12
# log the user's details
self.stdout.write(
self.style.SUCCESS(
f"Superuser successfully created with Username='{self.settings.SUPERUSER_DEFAULT_NAME}', Password='{self.settings.SUPERUSER_DEFAULT_PASSWORD}'"
Copy link
Contributor

Choose a reason for hiding this comment

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

Security concerns were raised by @brian-kim31 when password and username are exposed on the console.
Maybe you can hide the password somehow, only showing a few last letters for the purpose of tracking, for example?

Copy link
Author

Choose a reason for hiding this comment

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

how about first-letter *** last-letter? so with Password123! it'd show P**********!?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create super user on app install
4 participants