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

fix: migration 0550 #28135

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: migration 0550 #28135

wants to merge 1 commit into from

Conversation

orian
Copy link
Contributor

@orian orian commented Jan 31, 2025

Problem

Importing DJango models directly in a migration code cases problems (some fields do not exist)

Changes

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Does this work well for both Cloud and self-hosted?

How did you test this code?

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR fixes critical issues with Django model imports in migration files by ensuring proper usage of the apps registry for accessing historical model versions during migrations.

  • Modified /posthog/migrations/0550_migrate_action_webhooks_to_destinations.py to correctly pass apps parameter to migration function
  • Updated /posthog/management/commands/migrate_action_webhooks.py to use apps.get_model() instead of direct model imports
  • Added hog_function_init parameter in convert_to_hog_function() for proper dependency injection
  • Test file /posthog/management/commands/test/test_migrate_action_webhooks.py needs fixing as it's missing apps definition and still uses direct imports

3 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

@@ -58,7 +58,7 @@ def test_migrate_large_number_of_actions_across_teams(self):
)

# Run the migration
migrate_all_teams_action_webhooks()
migrate_all_teams_action_webhooks(apps)
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: The 'apps' parameter is undefined. This will cause a NameError when running the test. The test needs to properly mock or obtain the apps registry.

Suggested change
migrate_all_teams_action_webhooks(apps)
from django.apps import apps
migrate_all_teams_action_webhooks(apps)

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.

1 participant