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

Allow to import public repositories on corporate site #3537

Merged
merged 4 commits into from
Jan 30, 2018

Conversation

humitos
Copy link
Member

@humitos humitos commented Jan 22, 2018

When the DEFAULT_PRIVACY_LEVEL is private we allow to import
public and private repositories from Bitbucket service.

This handles an issue under .com, https://github.com/readthedocs/readthedocs-corporate/issues/225

When the DEFAULT_PRIVACY_LEVEL is ``private`` we allow to import
public _and_ private repositories from Bitbucket service.
@humitos humitos force-pushed the humitos/bitbucket/public-repos branch from b3ac73d to f5e8987 Compare January 22, 2018 16:05
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

This is certainly something that we should have caught. Tests here would be a good addition.

@humitos
Copy link
Member Author

humitos commented Jan 22, 2018

I just pushed a test for this, but I will add a comment on the commit since we need to find out how to make it valid :)

@@ -270,6 +271,17 @@ def test_make_project_fail(self):
data, organization=self.org, privacy=self.privacy)
self.assertIsNone(repo)

@override_settings(DEFAULT_PRIVACY_LEVEL='private')
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the most important part of the test.

The problem we have here is how are the method defined and how they use this setting:

The test passes because it's a public repo under a public default privacy level.

What it should be the best way to handle this? Should we make privacy as None by default and access the settings object inside the create_repository method?

cc @agjohnson

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this probably makes the most sense. 👍

@RichardLitt RichardLitt added the Bug A bug label Jan 22, 2018
@humitos
Copy link
Member Author

humitos commented Jan 29, 2018

@agjohnson after the changes, this is ready to be merged. Please, take another look if you can.

@agjohnson
Copy link
Contributor

Looks great! 👍 on the tests for other providers as well. Make our tests more defensive is a smart idea.

@agjohnson agjohnson merged commit df8079a into master Jan 30, 2018
@agjohnson agjohnson deleted the humitos/bitbucket/public-repos branch January 30, 2018 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants