From fb0d3d69c227d04d9467482dd572af78cfa4a7fa Mon Sep 17 00:00:00 2001 From: Jimmy Zelinskie Date: Tue, 24 Feb 2015 17:50:54 -0500 Subject: [PATCH] changes to reflect PR comments (not finished) --- data/database.py | 4 +- data/model/legacy.py | 11 +-- endpoints/api/repository.py | 8 +- endpoints/api/user.py | 6 +- static/js/controllers/repolist.js | 137 ------------------------------ static/js/pages/repo-list.js | 23 +++-- test/test_api_security.py | 6 +- test/test_api_usage.py | 9 +- 8 files changed, 32 insertions(+), 172 deletions(-) delete mode 100644 static/js/controllers/repolist.js diff --git a/data/database.py b/data/database.py index 81377d980..8b1310da9 100644 --- a/data/database.py +++ b/data/database.py @@ -303,8 +303,8 @@ class Repository(BaseModel): class Star(BaseModel): - user = ForeignKeyField(User, index=True, related_name="stars") - repository = ForeignKeyField(Repository, index=True, related_name="stargazers") + user = ForeignKeyField(User, index=True) + repository = ForeignKeyField(Repository, index=True) created = DateTimeField(default=datetime.now) class Meta: diff --git a/data/model/legacy.py b/data/model/legacy.py index f7f5c51f9..ccf89d3c2 100644 --- a/data/model/legacy.py +++ b/data/model/legacy.py @@ -2550,17 +2550,12 @@ def unstar_repository(user, repository): """ Unstars a repository. """ try: star = (Star - .select() - .join(Repository) - .switch(Star) - .join(User) - .where(Repository.id == repository.id, User.id == user.id) - .get()) + .delete() + .where(Star.repository == repository.id, Star.user == user.id) + .execute()) except Star.DoesNotExist: raise DataModelException('Star not found.') - star.delete_instance() - def get_user_starred_repositories(user, limit=None, page=None): """ Retrieves all of the repositories a user has starred. """ diff --git a/endpoints/api/repository.py b/endpoints/api/repository.py index 1e7e14d41..3fe5301d3 100644 --- a/endpoints/api/repository.py +++ b/endpoints/api/repository.py @@ -110,11 +110,13 @@ class RepositoryList(ApiResource): def get(self, args): """Fetch the list of repositories under a variety of situations.""" username = None - if get_authenticated_user() and args['private']: - username = get_authenticated_user().username + if get_authenticated_user(): starred_repos = model.get_user_starred_repositories(get_authenticated_user()) star_lookup = set([repo.id for repo in starred_repos]) + if args['private']: + username = get_authenticated_user().username + response = {} repo_count = None @@ -133,7 +135,7 @@ class RepositoryList(ApiResource): 'description': repo_obj.description, 'is_public': repo_obj.visibility.name == 'public', } - if get_authenticated_user() and args['private']: + if get_authenticated_user(): repo['is_starred'] = repo_obj.id in star_lookup return repo diff --git a/endpoints/api/user.py b/endpoints/api/user.py index 3284e4371..c19ebff28 100644 --- a/endpoints/api/user.py +++ b/endpoints/api/user.py @@ -709,9 +709,9 @@ class StarredRepositoryList(ApiResource): """ List all starred repositories. """ page = args['page'] limit = args['limit'] - starred_repos = list(model.get_user_starred_repositories(get_authenticated_user(), - page=page, - limit=limit)) + starred_repos = model.get_user_starred_repositories(get_authenticated_user(), + page=page, + limit=limit) def repo_view(repo_obj): return { 'namespace': repo_obj.namespace_user.username, diff --git a/static/js/controllers/repolist.js b/static/js/controllers/repolist.js deleted file mode 100644 index 50243603e..000000000 --- a/static/js/controllers/repolist.js +++ /dev/null @@ -1,137 +0,0 @@ -function RepoListCtrl($scope, $sanitize, Restangular, UserService, ApiService) { - $scope.namespace = null; - $scope.page = 1; - $scope.publicPageCount = null; - - // When loading the UserService, if the user is logged in, create a list of - // relevant namespaces for later collecting relevant repositories. - UserService.load(function() { - var user = UserService.currentUser(); - if (!user.anonymous) { - $scope.namespaces = [user]; - for (var i = 0; i < user.organizations.length; i++) { - $scope.namespaces.push(user.organizations[i]); - } - //loadStarredRepos(); - //loadRepos(); - } - }); - - // If someone signs in on this page, we have to watch the user and re-load - // their repositories after they've signed in to actually have any content - // on the page. - $scope.$watch(function(scope) { return scope.user }, - function(user) { - if (!user.anonymous) { - $scope.namespaces = [user]; - for (var i = 0; i < user.organizations.length; i++) { - $scope.namespaces.push(user.organizations[i]); - } - loadStarredRepos(); - loadRepos(); - } - }); - - $scope.toggleStar = function(repo) { - if (repo.is_starred) { - unstarRepo(repo); - } else { - starRepo(repo); - } - } - - var starRepo = function(repo) { - var data = { - 'namespace': repo.namespace, - 'repository': repo.name - }; - ApiService.createStar(data).then(function(result) { - updateReposAfterStar(repo); - }, ApiService.errorDisplay('Could not star repository')); - }; - - var unstarRepo = function(repo) { - var data = { - 'repository': repo.namespace + '/' + repo.name - }; - ApiService.deleteStar(null, data).then(function(result) { - updateReposAfterUnstar(repo); - }, ApiService.errorDisplay('Could not unstar repository')); - }; - - // Finds a repository within the list of namespaces attached to $scope. - var findRepoInList = function(repoNamespace, repoName) { - var namespaceIndex = $scope.namespaces.map(function (n) { - return n.username || n.name; - }).indexOf(repoNamespace); - - var namespace = $scope.namespaces[namespaceIndex] - - var repoIndex = namespace.repositories.value.map(function (r) { - return r.namespace + '/' + r.name; - }).indexOf(repoNamespace + '/' + repoName); - - return repoIndex != -1 ? namespace.repositories.value[repoIndex] : null; - } - - // Add a starred repository to the list starred repository list and make - // sure it appears starred elsewhere on the page. - var updateReposAfterStar = function(repository) { - $scope.starred_repositories.value.push(repository); - - var repo = findRepoInList(repository.namespace, repository.name); - if (repo != null) { - repo.is_starred = true; - } - } - - // Remove a repository from the starred repository list and make sure that - // it doesn't appear starred elsewhere on the page. - var updateReposAfterUnstar = function(repository) { - // Remove from the starred listings - var index = $scope.starred_repositories.value.map(function(r) { - return r.namespace + '/' + r.name; - }).indexOf(repository.namespace + '/' + repository.name); - $scope.starred_repositories.value.splice(index, 1); - - // Set repo from the normal listings to unstarred. - var repo = findRepoInList(repository.namespace, repository.name); - if (repo != null) { - repo.is_starred = false; - } - }; - - var loadStarredRepos = function() { - if (!$scope.user || $scope.user.anonymous) { - return; - } - - $scope.starred_repositories = ApiService.listStarredReposAsResource().get(function(resp) { - return resp.repositories.map(function(repo) { - repo.is_starred = true; - return repo; - }); - }); - }; - - // Iterate over all of the $scope.namespaces and collect their respective - // repositories. - var loadRepos = function() { - if ($scope.namespaces.length == 0 || $scope.user.anonymous) { - return; - } - - for (var i = 0; i < $scope.namespaces.length; i++) { - var namespace = $scope.namespaces[i]; - var namespaceName = namespace.username || namespace.name; - var options = { - 'public': false, - 'sort': true, - 'namespace': namespaceName, - }; - namespace.repositories = ApiService.listReposAsResource().withOptions(options).get(function(resp) { - return resp.repositories; - }); - } - }; -} diff --git a/static/js/pages/repo-list.js b/static/js/pages/repo-list.js index bb3087b71..efe78eaeb 100644 --- a/static/js/pages/repo-list.js +++ b/static/js/pages/repo-list.js @@ -30,8 +30,6 @@ for (var i = 0; i < user.organizations.length; i++) { $scope.namespaces.push(user.organizations[i]); } - //loadStarredRepos(); - //loadRepos(); } }); @@ -39,16 +37,17 @@ // their repositories after they've signed in to actually have any content // on the page. $scope.$watch(function(scope) { return scope.user }, - function(user) { - if (!user.anonymous) { - $scope.namespaces = [user]; - for (var i = 0; i < user.organizations.length; i++) { - $scope.namespaces.push(user.organizations[i]); - } - loadStarredRepos(); - loadRepos(); - } - }); + function(user) { + if (!user.anonymous) { + $scope.namespaces = [user]; + for (var i = 0; i < user.organizations.length; i++) { + $scope.namespaces.push(user.organizations[i]); + } + loadStarredRepos(); + loadRepos(); + } + } + ); $scope.toggleStar = function(repo) { if (repo.is_starred) { diff --git a/test/test_api_security.py b/test/test_api_security.py index 7f047c270..df01fe8c9 100644 --- a/test/test_api_security.py +++ b/test/test_api_security.py @@ -175,13 +175,13 @@ class TestUserStarredRepository(ApiTestCase): self._run_test('DELETE', 401, None, None) def test_delete_freshuser(self): - self._run_test('DELETE', 400, 'freshuser', None) + self._run_test('DELETE', 204, 'freshuser', None) def test_delete_reader(self): - self._run_test('DELETE', 400, 'reader', None) + self._run_test('DELETE', 204, 'reader', None) def test_delete_devtable(self): - self._run_test('DELETE', 400, 'devtable', None) + self._run_test('DELETE', 204, 'devtable', None) class TestUserNotification(ApiTestCase): diff --git a/test/test_api_usage.py b/test/test_api_usage.py index 6dacb72ff..77d3364fc 100644 --- a/test/test_api_usage.py +++ b/test/test_api_usage.py @@ -224,6 +224,7 @@ class TestLoggedInUser(ApiTestCase): assert json['anonymous'] == False assert json['username'] == READ_ACCESS_USER + class TestUserStarredRepositoryList(ApiTestCase): def test_get_stars_guest(self): self.getJsonResponse(StarredRepositoryList, expected_code=401) @@ -240,9 +241,6 @@ class TestUserStarredRepositoryList(ApiTestCase): }, expected_code=401) - def test_unstar_repo_guest(self): - self.deleteResponse(StarredRepository, params=dict(repository='public/publicrepo'), expected_code=401) - def test_star_and_unstar_repo_user(self): self.login(READ_ACCESS_USER) json = self.getJsonResponse(StarredRepositoryList) @@ -257,8 +255,11 @@ class TestUserStarredRepositoryList(ApiTestCase): assert json['namespace'] == 'public' assert json['repository'] == 'publicrepo' - self.deleteResponse(StarredRepository, params=dict(repository='public/publicrepo'), expected_code=204) + self.deleteResponse(StarredRepository, params=dict(repository='public/publicrepo'), + expected_code=204) + json = self.getJsonResponse(StarredRepositoryList) + assert json['repositories'] == [] class TestUserNotification(ApiTestCase):