-
Notifications
You must be signed in to change notification settings - Fork 2k
fix(cohorts): Add migration for BehavioralCohortAnalysis #39788
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 files reviewed, no comments
Migration SQL ChangesHey 👋, we've detected some migrations on this PR. Here's the SQL output for each migration, make sure they make sense:
|
🔍 Migration Risk AnalysisWe've analyzed your migrations for potential risks. Summary: 1 Safe | 0 Needs Review | 0 Blocked ✅ SafeNo locks, backwards compatible
|
Original approach created a fake model with managed=False just to get an admin interface. This caused Django to track it in migrations, requiring a migration file that did nothing but add noise. Better approach: Custom admin view without any model. Just a form that triggers management commands. Registered directly in ee/urls.py. Changes: - Remove BehavioralCohortAnalysis model from behavioral_cohorts_admin.py - Remove 0886_behavioralcohortanalysis.py migration - Create analyze_behavioral_cohorts_view as standalone view function - Register URL in ee/urls.py under admin path - Update max_migration.txt to 0885
Without a registered model, the behavioral cohort analysis view doesn't appear in the Django admin sidebar automatically. Add custom link to admin index page so users can find it. Changes: - Add custom app.html template for behavioral cohort analysis section - Include in admin index.html - Add named URL pattern for reverse URL lookup
) | ||
|
||
|
||
class BehavioralCohortAnalysis(models.Model): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm removing the model and add a custom view. This should be better and get rid of the migration problem.
Prevent duplicate submissions on page refresh by redirecting after successful form submission.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced the fake model approach with a proper custom admin view. The original implementation used a managed=False model just to get an admin interface, which forced Django to track it in migrations despite not creating any tables.
New approach: Direct URL registration in ee/urls.py with a standalone view function. No model = no migration needed = no CI failures. Added a custom link on the admin index page for discoverability.
Amazing, thanks @webjunkie !! |
man thanks a lot!! thanks for the explanation! good to know for next time |
Problem
BehavioralCohortAnalysis
was added as a proxy model without a migration. Whenever we runmakemigrations
, Django will attempt to add it alongside new model changes.Changes
Add a migration file. A table won't be created since
managed = false
.