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.

Sign up for GitHub

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

Jump to bottom

Refactor: Django admin always enabled #1881

Merged
merged 19 commits into from Feb 23, 2024
Merged

Refactor: Django admin always enabled #1881

merged 19 commits into from Feb 23, 2024

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented Feb 8, 2024 •
edited by thekaveman

Prerequisite of finishing #1856

@angela-tran angela-tran self-assigned this Feb 8, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 8, 2024 •
edited

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)		Lines missing
  benefits
  settings.py
  urls.py
  benefits/core
  admin.py
Project Total  

This report was generated by python-coverage-comment-action

@github-actions github-actions bot added back-end Django views, sessions, middleware, models, migrations etc. documentation [auto] Improvements or additions to documentation deployment-dev [auto] Changes that will trigger a deploy if merged to dev tests Related to automated testing (unit, UI, integration, etc.) infrastructure Terraform, Azure, etc. and removed deployment-dev [auto] Changes that will trigger a deploy if merged to dev labels Feb 8, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 8, 2024

Preview url: https://benefits-1881--cal-itp-previews.netlify.app

1 similar comment
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 8, 2024

Preview url: https://benefits-1881--cal-itp-previews.netlify.app

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 8, 2024

Preview url: https://benefits-1881--cal-itp-previews.netlify.app

@thekaveman thekaveman added this to the Admin tool: foundation milestone Feb 12, 2024
@thekaveman thekaveman self-assigned this Feb 12, 2024
@thekaveman thekaveman force-pushed the refactor/always-django-admin branch 2 times, most recently from f20faf3 to 0eebbd3 Compare February 14, 2024 00:20
@github-actions GitHub Actions
Copy link

Preview url: https://benefits-1881--cal-itp-previews.netlify.app

1 similar comment
@github-actions GitHub Actions
Copy link

Preview url: https://benefits-1881--cal-itp-previews.netlify.app

angela-tran and others added 11 commits February 13, 2024 16:21
@angela-tran @thekaveman
refactor(admin): admin interface is always enabled
4625f25
@angela-tran @thekaveman
test(admin): update unit test assertion
d9b2c28
@angela-tran @thekaveman
feat: add script for resetting the database
2174ac8
@thekaveman
refactor(settings): consolidate template processors
0e0f37c 
remove duplicate django.contrib.messages.context_processors.messages
@thekaveman
test(admin): assert redirects to login page
84956df
@thekaveman
test(admin): pre_login_user success and failure
eeffe2e
@thekaveman
chore(settings): remove unused variable
f394a3c
@thekaveman
chore(config): add sample SUPERUSER env vars
6f9ba82 
these have to be present in the .env file to reset the DB locally
@thekaveman
feat(devcontainer): startup with the reset_db script
409391e 
reset_db.sh reuses the existing init.sh for other initialization
@thekaveman
refactor(migrations): remove data migration, use local fixtures
c2b074f 
dumped the existing (prior to this deletion) data using Django manage.py,
excluding some model types that are defined and recreated by other migrations:

   python manage.py dumpdata \
    --exclude auth.permission \
    --exclude auth.user \
    --exclude contenttypes.contenttype > fixtures.json

then cleaned up the labels/names of our sample data for consistency

updates db_reset.sh to load these fixtures after migrations are run

updates the Cypress tests to use the new fixture location for sample data
@thekaveman
fix(tests): container startup script specifically for Cypress
4bac379
@github-actions GitHub Actions
Copy link

Preview url: https://benefits-1881--cal-itp-previews.netlify.app

@thekaveman
chore(terraform): remove env vars used in data migration
aeaa3aa 
these are no longer relevant as we'll use the admin interface directly
to configure
@github-actions GitHub Actions
Copy link

Preview url: https://benefits-1881--cal-itp-previews.netlify.app

@thekaveman thekaveman mentioned this pull request Feb 15, 2024
Closed
22 tasks
@thekaveman thekaveman marked this pull request as ready for review February 15, 2024 22:41
@thekaveman thekaveman requested a review from a team as a code owner February 15, 2024 22:41
@thekaveman
Copy link
Member

@angela-tran @machikoyasuda this is ready for review.

@angela-tran I know you opened the original PR but I'd still like your review since I made a ton of changes.

The full list of post-merge steps (and other things I did) is outlined in #1856

@github-actions GitHub Actions
Copy link

Preview url: https://benefits-1881--cal-itp-previews.netlify.app

@thekaveman
refactor(rest_db): allow more local customization
b71b525 
* devs may or may not want to reset their local DB
* devs may want to change which DB file is targeted
* devs may want to change which fixture file is loaded

update docs to reflect these changes
@thekaveman
feat(git): ignore fixtures except the included sample
0a562fb
@github-actions GitHub Actions
Copy link

Preview url: https://benefits-1881--cal-itp-previews.netlify.app

machikoyasuda
machikoyasuda previously approved these changes Feb 16, 2024
View reviewed changes
Copy link
Member

@machikoyasuda machikoyasuda left a comment

Choose a reason for hiding this comment

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

got admin up and running on my machine

angela-tran
angela-tran commented Feb 16, 2024
View reviewed changes
Copy link
Member Author

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

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

Thank you @thekaveman for finishing out this PR. The code changes look good to me.

I noticed some documentation that is out-of-date:
docs/deployment/README.md

no external database...
...loaded via data migrations

and

docs/development/models-migrations.md

@thekaveman
docs(deployment): update language around config database
94312c0 
note the use of Key Vault for secrets, Django admin for non-secrets
@thekaveman
refactor(migrations): update helper script and docs
e37ed91 
now that we have the Admin interface, we don't want to regrenerate the existing migration
rather, we need to generate new migrations each time to reflect model changes into the DB
@github-actions GitHub Actions
Copy link

Preview url: https://benefits-1881--cal-itp-previews.netlify.app

@thekaveman
Copy link
Member

@angela-tran thanks for those notes. You reminded me that we also need to update the bin/makemigrations.sh helper to account for not having the data migration anymore, and needing to generate new schema migrations each time, rather than updating the existing.

@github-actions GitHub Actions
Copy link

Preview url: https://benefits-1881--cal-itp-previews.netlify.app

angela-tran
angela-tran commented Feb 23, 2024
View reviewed changes
Copy link
Member Author

@angela-tran angela-tran left a comment

Choose a reason for hiding this comment

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

Great catch on the migrations helper script, and thanks for the documentation updates.

I noticed one typo to be fixed, but other than that this PR is approved ✅

docs/development/i18n.md Outdated Show resolved Hide resolved
@thekaveman @angela-tran
docs: fix typo in Django URL
0f8ab20 
Co-authored-by: Angela Tran <angela@compiler.la>
@github-actions GitHub Actions
Copy link

Preview url: https://benefits-1881--cal-itp-previews.netlify.app

@thekaveman thekaveman merged commit 6d0a87c into dev Feb 23, 2024
13 checks passed
@thekaveman thekaveman deleted the refactor/always-django-admin branch February 23, 2024 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thekaveman thekaveman thekaveman approved these changes

@machikoyasuda machikoyasuda machikoyasuda left review comments

Assignees

@thekaveman thekaveman

@angela-tran angela-tran

Labels
back-end Django views, sessions, middleware, models, migrations etc. documentation [auto] Improvements or additions to documentation infrastructure Terraform, Azure, etc. tests Related to automated testing (unit, UI, integration, etc.)
Projects
Digital Services
Status: Done
Milestone
Admin tool: foundation
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@angela-tran @thekaveman @machikoyasuda