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, readthedocs/readthedocs-corporate#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 Jan 22, 2018
Copy link
Contributor

@agjohnson agjohnson left a comment

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

@humitos
Copy link
Member Author

@humitos 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

@humitos humitos Jan 22, 2018

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

@agjohnson agjohnson Jan 29, 2018

Yeah, this probably makes the most sense. 👍

@humitos
Copy link
Member Author

@humitos 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

@agjohnson agjohnson commented Jan 30, 2018

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
1 check passed
@agjohnson agjohnson deleted the humitos/bitbucket/public-repos branch Jan 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants