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

Move organizations models #6776

Merged
merged 19 commits into from Jun 9, 2020
Merged

Move organizations models #6776

merged 19 commits into from Jun 9, 2020

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Mar 11, 2020

This doesn't migrate the whole app, only their models and its dependencies the more minimal possible (querysets, managers, utils, constants). The app is named readthedocs.organizations, django will take the last component to name the app (organizations). Thanks to that we don't need to do any kind of migration on .com, django will still "think" it is the same app.

This requires some PRs to be merged on .com first. I was able to run tests successfully and try it locally on .com and .org.

@stsewd stsewd added the Status: blocked label Mar 11, 2020
@stsewd
Copy link
Member Author

stsewd commented Mar 11, 2020

Looks like there is a missing migration, we don't check migrations on .com, but it shouldn't be a problem. I can fix it con .com first.

@stsewd stsewd removed the Status: blocked label Mar 17, 2020
@stsewd stsewd requested a review from a team Mar 17, 2020
@humitos
Copy link
Member

humitos commented Apr 21, 2020

This doesn't migrate the whole app, only their models and its dependencies the more minimal possible (querysets, managers, utils, constants).

Is there anything blocking us to move the whole app?

The app is named readthedocs.organizations, django will take the last component to name the app (organizations).

Just for the records, https://docs.djangoproject.com/en/3.0/ref/applications/#django.apps.AppConfig.label

humitos
humitos previously approved these changes Apr 21, 2020
Copy link
Member

@humitos humitos left a comment

This is good. I'm not sure if it needs a deeper look, but overall it's OK.

I'd may not install the app just yet until we have everything migrated here.

@@ -145,6 +145,7 @@ def INSTALLED_APPS(self): # noqa

# our apps
'readthedocs.projects',
'readthedocs.organizations',
Copy link
Member

@humitos humitos Apr 21, 2020

Choose a reason for hiding this comment

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

I assume that installing this app won't have any effect in .org until we register the URLs. It may be better to that time before adding it to the INSTALLED_APPS.

Copy link
Member Author

@stsewd stsewd Apr 21, 2020

Choose a reason for hiding this comment

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

It doesn't have any effect on .org, but we need it so django can manage the models. We can install the app only on .com maybe.

@humitos
Copy link
Member

humitos commented Apr 21, 2020

Also, please don't merge this until we have some time testing it locally first.

@humitos
Copy link
Member

humitos commented Apr 21, 2020

What's the best way to review this PR? Is there any change or is just copy and paste from the corporate code?

@stsewd
Copy link
Member Author

stsewd commented Apr 21, 2020

Is there anything blocking us to move the whole app?

Templates and logic

@stsewd
Copy link
Member Author

stsewd commented Apr 21, 2020

What's the best way to review this PR? Is there any change or is just copy and paste from the corporate code?

You need to pull both PRs, from the .org and .com. Check that everything in .com works as expected, and the database doesn't require any changes/migrations.

@humitos humitos self-requested a review May 11, 2020
@humitos humitos dismissed their stale review May 11, 2020

I need to test this PR locally before merging

@humitos humitos requested a review from a team May 11, 2020
Copy link
Member

@ericholscher ericholscher left a comment

Tested this locally and it looks good on community 👍

@ericholscher
Copy link
Member

ericholscher commented Jun 9, 2020

Definitely would be useful to get the admin over, but not a huge issue if we aren't actively creating organizations.

@stsewd stsewd merged commit 49b03f2 into master Jun 9, 2020
2 checks passed
@stsewd stsewd deleted the move-organizations-models branch Jun 9, 2020
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.

None yet

3 participants