Repository: repobee/repobee

Issue:
API.ensure_teams_and_members should just take teams as input
As there now is an `apimeta.Teams` data structure, that may be passed directly to the `API.ensure_teams_and_members` function.

Discussion:
codecov-io: # [Codecov](https://codecov.io/gh/repobee/repobee/pull/217?src=pr&el=h1) Report
> Merging [#217](https://codecov.io/gh/repobee/repobee/pull/217?src=pr&el=desc) into [master](https://codecov.io/gh/repobee/repobee/commit/d95d223127759b3a28bcc0e92cf270ddb0e4a1e4?src=pr&el=desc) will **decrease** coverage by `0.13%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/repobee/repobee/pull/217/graphs/tree.svg?width=650&token=5vOKgkphZe&height=150&src=pr)](https://codecov.io/gh/repobee/repobee/pull/217?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #217      +/-   ##
==========================================
- Coverage   91.34%   91.21%   -0.14%     
==========================================
  Files          19       19              
  Lines        1306     1309       +3     
  Branches      234      237       +3     
==========================================
+ Hits         1193     1194       +1     
- Misses         93       95       +2     
  Partials       20       20
```


| [Impacted Files](https://codecov.io/gh/repobee/repobee/pull/217?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [repobee/apimeta.py](https://codecov.io/gh/repobee/repobee/pull/217/diff?src=pr&el=tree#diff-cmVwb2JlZS9hcGltZXRhLnB5) | `92.2% <100%> (ø)` | :arrow_up: |
| [repobee/command.py](https://codecov.io/gh/repobee/repobee/pull/217/diff?src=pr&el=tree#diff-cmVwb2JlZS9jb21tYW5kLnB5) | `95.23% <100%> (-0.03%)` | :arrow_down: |
| [repobee/github\_api.py](https://codecov.io/gh/repobee/repobee/pull/217/diff?src=pr&el=tree#diff-cmVwb2JlZS9naXRodWJfYXBpLnB5) | `92.53% <100%> (+0.09%)` | :arrow_up: |
| [repobee/gitlab\_api.py](https://codecov.io/gh/repobee/repobee/pull/217/diff?src=pr&el=tree#diff-cmVwb2JlZS9naXRsYWJfYXBpLnB5) | `78.94% <100%> (+0.18%)` | :arrow_up: |
| [repobee/cli.py](https://codecov.io/gh/repobee/repobee/pull/217/diff?src=pr&el=tree#diff-cmVwb2JlZS9jbGkucHk=) | `95.41% <0%> (-0.84%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/repobee/repobee/pull/217?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/repobee/repobee/pull/217?src=pr&el=footer). Last update [d95d223...8451896](https://codecov.io/gh/repobee/repobee/pull/217?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/repobee/apimeta.py b/repobee/apimeta.py
index fb49f48..5483e77 100644
--- a/repobee/apimeta.py
+++ b/repobee/apimeta.py
@@ -118,7 +118,7 @@ class APISpec:
     def __init__(self, base_url, token, org_name, user):
         _not_implemented()
 
-    def ensure_teams_and_members(self, member_lists, permission):
+    def ensure_teams_and_members(self, teams, permission):
         _not_implemented()
 
     def get_teams(self):
@@ -127,7 +127,7 @@ class APISpec:
     def create_repos(self, repos):
         _not_implemented()
 
-    def get_repo_urls(self, master_repo_names, org_name, students):
+    def get_repo_urls(self, master_repo_names, org_name, teams):
         _not_implemented()
 
     def get_issues(self, repo_names, state, title_regex):
@@ -142,7 +142,7 @@ class APISpec:
     def add_repos_to_review_teams(self, team_to_repos, issue):
         _not_implemented()
 
-    def get_review_progress(self, review_team_names, students, title_regex):
+    def get_review_progress(self, review_team_names, teams, title_regex):
         _not_implemented()
 
     def delete_teams(self, team_names):
diff --git a/repobee/command.py b/repobee/command.py
index 4b8d79f..e6e5d15 100644
--- a/repobee/command.py
+++ b/repobee/command.py
@@ -92,8 +92,7 @@ def _add_students_to_teams(
     Returns:
         all teams associated with the students in the students list.
     """
-    member_lists = {str(team): team.members for team in teams}
-    return api.ensure_teams_and_members(member_lists)
+    return api.ensure_teams_and_members(teams)
 
 
 def _create_student_repos(
diff --git a/repobee/github_api.py b/repobee/github_api.py
index 88a88e3..b7c5716 100644
--- a/repobee/github_api.py
+++ b/repobee/github_api.py
@@ -157,6 +157,17 @@ class GitHubAPI(apimeta.API):
                 if team.name in team_names
             )
 
+    def get_teams(self):
+        return [
+            apimeta.Team(
+                name=t.name,
+                members=[m.login for m in t.get_members()],
+                id=t.id,
+                implementation=t,
+            )
+            for t in self._org.get_teams()
+        ]
+
     def delete_teams(self, team_names: Iterable[str]) -> None:
         """Delete all teams that match any of the team names. Skip any team
         name for which no team can be found.
@@ -200,20 +211,19 @@ class GitHubAPI(apimeta.API):
         return existing_users
 
     def ensure_teams_and_members(
-        self,
-        member_lists: Mapping[str, Iterable[str]],
-        permission: str = "push",
+        self, teams: Iterable[apimeta.Team], permission: str = "push"
     ) -> List[apimeta.Team]:
         """Create teams that do not exist and add members not in their
         specified teams (if they exist as users).
 
         Args:
-            member_list: A mapping of (team_name, member_list).
+            teams: A list of teams specifying student groups.
 
         Returns:
             A list of Team namedtuples of the teams corresponding to the keys
             of the member_lists mapping.
         """
+        member_lists = {team.name: team.members for team in teams}
         raw_teams = self._ensure_teams_exist(
             [team_name for team_name in member_lists.keys()],
             permission=permission,
@@ -341,20 +351,20 @@ class GitHubAPI(apimeta.API):
         self,
         master_repo_names: Iterable[str],
         org_name: Optional[str] = None,
-        students: Optional[List[apimeta.Team]] = None,
+        teams: Optional[List[apimeta.Team]] = None,
     ) -> List[str]:
         """Get repo urls for all specified repo names in organization. Assumes
         that the repos exist, there is no guarantee that they actually do as
         checking this with the REST API takes too much time.
 
-        If the `students` argument is supplied, student repo urls are
+        If the `teams` argument is supplied, student repo urls are
         computed instead of master repo urls.
 
         Args:
             master_repo_names: A list of master repository names.
             org_name: Organization in which repos are expected. Defaults to the
                 target organization of the API instance.
-            students: A list of student groups.
+            teams: A list of teams specifying student groups.
 
         Returns:
             a list of urls corresponding to the repo names.
@@ -367,8 +377,8 @@ class GitHubAPI(apimeta.API):
             )
         repo_names = (
             master_repo_names
-            if not students
-            else util.generate_repo_names(students, master_repo_names)
+            if not teams
+            else util.generate_repo_names(teams, master_repo_names)
         )
         return [
             self._insert_auth(url)
@@ -521,7 +531,7 @@ class GitHubAPI(apimeta.API):
             )
 
     def get_review_progress(
-        self, review_team_names, students, title_regex
+        self, review_team_names, teams, title_regex
     ) -> Mapping[str, List[tuples.Review]]:
         """Get the peer review progress for the specified review teams and
         students. Only issues matching the title regex will be considered peer
@@ -535,8 +545,7 @@ class GitHubAPI(apimeta.API):
 
         Args:
             review_team_names: Names of review teams.
-            students: The reviewing students (supposedly also the ones being
-                reviewed, but not necessarily)
+            teams: A list of teams specifying student groups.
             title_regex: If an issue title matches this regex, the issue is
                 considered a potential peer review issue.
         Returns:
@@ -544,16 +553,16 @@ class GitHubAPI(apimeta.API):
             assigned_repos is a :py:class:`~repobee.tuples.Review`.
         """
         reviews = collections.defaultdict(list)
-        teams = self._get_teams_in(review_team_names)
-        for team in teams:
+        review_teams = self._get_teams_in(review_team_names)
+        for review_team in review_teams:
             with _try_api_request():
-                LOGGER.info("processing {}".format(team.name))
-                reviewers = set(m.login for m in team.get_members())
-                repos = list(team.get_repos())
+                LOGGER.info("processing {}".format(review_team.name))
+                reviewers = set(m.login for m in review_team.get_members())
+                repos = list(review_team.get_repos())
                 if len(repos) != 1:
                     LOGGER.warning(
                         "expected {} to have 1 associated repo, found {}."
-                        "Skipping...".format(team.name, len(repos))
+                        "Skipping...".format(review_team.name, len(repos))
                     )
                     continue
 
diff --git a/repobee/gitlab_api.py b/repobee/gitlab_api.py
index 420cdba..ffd6165 100644
--- a/repobee/gitlab_api.py
+++ b/repobee/gitlab_api.py
@@ -10,7 +10,7 @@ GitLabAPI are mostly high-level bulk operations.
 
 .. moduleauthor:: Simon Larsén
 """
-from typing import List, Iterable, Mapping, Optional
+from typing import List, Iterable, Optional
 from socket import gaierror
 import collections
 import contextlib
@@ -101,9 +101,7 @@ class GitLabAPI(apimeta.API):
             )
 
     def ensure_teams_and_members(
-        self,
-        member_lists: Mapping[str, Iterable[str]],
-        permission: str = "push",
+        self, teams: Iterable[apimeta.Team], permission: str = "push"
     ) -> List[apimeta.Team]:
         """Create teams that do not exist and add members not in their
         specified teams (if they exist as users).
@@ -115,6 +113,7 @@ class GitLabAPI(apimeta.API):
             A list of Team namedtuples of the teams corresponding to the keys
             of the member_lists mapping.
         """
+        member_lists = {team.name: team.members for team in teams}
         raw_teams = self._ensure_teams_exist(
             [str(team_name) for team_name in member_lists.keys()],
             permission=permission,
@@ -169,7 +168,15 @@ class GitLabAPI(apimeta.API):
         return [self._User(m.id, m.username) for m in group.members.list()]
 
     def get_teams(self):
-        return self._gitlab.groups.list(id=self._group.id)
+        return [
+            apimeta.Team(
+                name=t.name,
+                members=[m.username for m in t.members.list()],
+                id=t.id,
+                implementation=t,
+            )
+            for t in self._gitlab.groups.list(id=self._group.id)
+        ]
 
     def _add_to_team(self, members: Iterable[str], team):
         """Add members to a team.
@@ -283,20 +290,20 @@ class GitLabAPI(apimeta.API):
         self,
         master_repo_names: Iterable[str],
         org_name: Optional[str] = None,
-        students: Optional[List[apimeta.Team]] = None,
+        teams: Optional[List[apimeta.Team]] = None,
     ) -> List[str]:
         """Get repo urls for all specified repo names in organization. Assumes
         that the repos exist, there is no guarantee that they actually do as
         checking this with the REST API takes too much time.
 
-        If the `students` argument is supplied, student repo urls are
+        If the `teams` argument is supplied, student repo urls are
         computed instead of master repo urls.
 
         Args:
             master_repo_names: A list of master repository names.
             org_name: Organization in which repos are expected. Defaults to the
                 target organization of the API instance.
-            students: A list of student groups.
+            teams: A list of teams specifying student groups.
 
         Returns:
             a list of urls corresponding to the repo names.
@@ -308,14 +315,14 @@ class GitLabAPI(apimeta.API):
                 "{}/{}.git".format(group_url, repo_name)
                 for repo_name in master_repo_names
             ]
-            if not students
+            if not teams
             else [
                 "{}/{}/{}.git".format(
                     group_url,
-                    student,
-                    util.generate_repo_name(str(student), master_repo_name),
+                    team,
+                    util.generate_repo_name(str(team), master_repo_name),
                 )
-                for student in students
+                for team in teams
                 for master_repo_name in master_repo_names
             ]
         )


Test patch:
diff --git a/tests/test_command.py b/tests/test_command.py
index f857c44..cf0df02 100644
--- a/tests/test_command.py
+++ b/tests/test_command.py
@@ -162,7 +162,7 @@ def students():
 
 @pytest.fixture
 def ensure_teams_and_members_mock(api_mock, students):
-    api_mock.ensure_teams_and_members.side_effect = lambda member_lists: [
+    api_mock.ensure_teams_and_members.side_effect = lambda teams: [
         apimeta.Team(name=str(student), members=[student], id=id)
         for id, student in enumerate(students)
     ]
@@ -290,16 +290,11 @@ class TestSetupStudentRepos:
         expected_clone_calls = [
             call(url, cwd=str(tmpdir)) for url in master_urls
         ]
-        expected_ensure_teams_arg = {
-            str(student): [str(student)] for student in students
-        }
 
         command.setup_student_repos(master_urls, students, api_mock)
 
         git_mock.clone_single.assert_has_calls(expected_clone_calls)
-        api_mock.ensure_teams_and_members.assert_called_once_with(
-            expected_ensure_teams_arg
-        )
+        api_mock.ensure_teams_and_members.assert_called_once_with(students)
         api_mock.create_repos.assert_called_once_with(repo_infos)
         git_mock.push.assert_called_once_with(push_tuples)
 
diff --git a/tests/test_github_api.py b/tests/test_github_api.py
index 9081486..113186a 100644
--- a/tests/test_github_api.py
+++ b/tests/test_github_api.py
@@ -312,28 +312,48 @@ def api(happy_github, organization, no_teams):
 
 class TestEnsureTeamsAndMembers:
     @staticmethod
-    def assert_equal_teams(teams_and_members, teams):
-        for team in teams:
-            members = {mem.login for mem in team.get_members()}
-            assert members == set(teams_and_members[team.name])
+    def assert_teams_equal(actual_teams, expected_teams):
+        """Assert that the teams are equal, except for the implementation and
+        id.
+        """
+        assert len(actual_teams) == len(expected_teams)
+        for actual, expected in zip(
+            sorted(actual_teams), sorted(expected_teams)
+        ):
+            assert (actual.name, sorted(actual.members)) == (
+                expected.name,
+                sorted(expected.members),
+            )
+
+    @pytest.fixture
+    def team_wrappers(self, teams_and_members):
+        """Wrap the teams_and_members dictionaries into apimeta.Team classes.
+
+        TODO: Remove this when this test suite is rewritten.
+        """
+        return [
+            apimeta.Team(name=team_name, members=members)
+            for team_name, members in teams_and_members.items()
+        ]
 
-    def test_no_previous_teams(self, api, teams_and_members, no_teams):
+    def test_no_previous_teams(self, api, team_wrappers, no_teams):
         """Test that ensure_teams_and_members works as expected when there are
         no previous teams, and all users exist. This is a massive end-to-end
         test of the function with only the lower level API's mocked out.
         """
-        api.ensure_teams_and_members(teams_and_members)
-        self.assert_equal_teams(teams_and_members, api.org.get_teams())
+        expected_teams = list(team_wrappers)
+        api.ensure_teams_and_members(expected_teams)
+        self.assert_teams_equal(api.get_teams(), expected_teams)
 
     def test_all_teams_exist_but_without_members(
-        self, api, teams_and_members, teams
+        self, api, team_wrappers, teams
     ):
         """Test that ensure_teams_and_members works as expected when all of
         the teams already exist, but have no members in them.
         """
-        api.ensure_teams_and_members(teams_and_members)
-
-        self.assert_equal_teams(teams_and_members, api.org.get_teams())
+        expected_teams = list(team_wrappers)
+        api.ensure_teams_and_members(expected_teams)
+        self.assert_teams_equal(api.get_teams(), expected_teams)
 
     @pytest.mark.parametrize(
         "unexpected_exc",
@@ -344,7 +364,7 @@ class TestEnsureTeamsAndMembers:
         ],
     )
     def test_raises_on_non_422_exception(
-        self, api, organization, teams_and_members, unexpected_exc
+        self, api, organization, team_wrappers, unexpected_exc
     ):
         """Should raise if anything but a 422 http error is raised when
         creating the team.
@@ -356,17 +376,17 @@ class TestEnsureTeamsAndMembers:
         organization.create_team.side_effect = raise_
 
         with pytest.raises(exception.UnexpectedException):
-            api.ensure_teams_and_members(teams_and_members)
+            api.ensure_teams_and_members(team_wrappers)
 
     def test_running_twice_has_no_ill_effects(
-        self, api, no_teams, teams_and_members
+        self, api, no_teams, team_wrappers
     ):
         """Tests that add_to_team is not called if all members are already
         in it."""
-        api.ensure_teams_and_members(teams_and_members)
-        api.ensure_teams_and_members(teams_and_members)
-
-        self.assert_equal_teams(teams_and_members, api.org.get_teams())
+        expected_teams = list(team_wrappers)
+        api.ensure_teams_and_members(expected_teams)
+        api.ensure_teams_and_members(expected_teams)
+        self.assert_teams_equal(api.get_teams(), expected_teams)
 
 
 class TestCreateRepos:
@@ -502,7 +522,7 @@ class TestGetRepoUrls:
         # assume works correctly when called with just repo names
         expected_urls = api.get_repo_urls(expected_repo_names)
 
-        actual_urls = api.get_repo_urls(master_repo_names, students=students)
+        actual_urls = api.get_repo_urls(master_repo_names, teams=students)
 
         assert len(actual_urls) == len(students) * len(master_repo_names)
         assert sorted(expected_urls) == sorted(actual_urls)
diff --git a/tests/test_gitlab_api.py b/tests/test_gitlab_api.py
index d04e8be..0663e0c 100644
--- a/tests/test_gitlab_api.py
+++ b/tests/test_gitlab_api.py
@@ -246,62 +246,57 @@ def team_names():
 
 
 class TestEnsureTeamsAndMembers:
-    def test_with_no_pre_existing_groups(self, api_mock, team_names):
+    def test_with_no_pre_existing_groups(self, api_mock):
         """Test that all groups are created correctly when the only existing
         group is the target group.
         """
         # arrange
         api = repobee.gitlab_api.GitLabAPI(BASE_URL, TOKEN, TARGET_GROUP)
-        allocations = {tn: [tn] for tn in team_names}
-        expected_group_names = list(allocations.keys())
+        expected_team_names = [team.name for team in constants.STUDENTS]
         assert (
-            expected_group_names
-        ), "pre-test assert, expected group names should be non-empty"
+            expected_team_names
+        ), "pre-test assert, expected team names should be non-empty"
 
         # act
-        api.ensure_teams_and_members(allocations)
+        api.ensure_teams_and_members(constants.STUDENTS)
 
         # assert
-        actual_groups = api.get_teams()
-        assert sorted([g.name for g in actual_groups]) == sorted(
-            expected_group_names
+        actual_teams = api.get_teams()
+        assert sorted([g.name for g in actual_teams]) == sorted(
+            expected_team_names
         )
-        for group in actual_groups:
-            if group.name != TARGET_GROUP:
-                group_member_names = [m.username for m in group.members.list()]
-                assert group_member_names == [group.name]
+        for team in actual_teams:
+            if team.name != TARGET_GROUP:
+                assert team.members == [team.name]
             else:
-                assert not group.members.list()
+                assert not team.members
 
     def test_with_multi_student_groups(self, api_mock):
         # arrange
         api = repobee.gitlab_api.GitLabAPI(BASE_URL, TOKEN, TARGET_GROUP)
         num_students = len(constants.STUDENTS)
-        allocations = {
-            str(
-                repobee.apimeta.Team(members=g1.members + g2.members)
-            ): g1.members
-            + g2.members
+        teams = [
+            repobee.apimeta.Team(members=g1.members + g2.members)
             for g1, g2 in zip(
                 constants.STUDENTS[: num_students // 2],
                 constants.STUDENTS[num_students // 2 :],
             )
-        }
-        expected_groups = dict(allocations)
+        ]
+        expected_teams = list(teams)
 
         # act
-        api.ensure_teams_and_members(allocations)
+        api.ensure_teams_and_members(teams)
 
         # assert
-        actual_groups = api.get_teams()
-        assert len(actual_groups) == len(expected_groups)
-        assert sorted([g.name for g in actual_groups]) == sorted(
-            expected_groups.keys()
+        actual_teams = api.get_teams()
+        assert len(actual_teams) == len(expected_teams)
+        assert sorted([t.name for t in actual_teams]) == sorted(
+            t.name for t in expected_teams
         )
-        for group in actual_groups:
-            member_names = [m.username for m in group.members.list()]
-            expected_member_names = expected_groups[group.name]
-            assert sorted(member_names) == sorted(expected_member_names)
+        for actual_team, expected_team in zip(
+            sorted(actual_teams), sorted(expected_teams)
+        ):
+            assert sorted(actual_team.members) == sorted(expected_team.members)
 
     def test_run_twice(self, team_names):
         """Running the function twice should have the same effect as
@@ -309,27 +304,25 @@ class TestEnsureTeamsAndMembers:
         """
         # arrange
         api = repobee.gitlab_api.GitLabAPI(BASE_URL, TOKEN, TARGET_GROUP)
-        allocations = {tn: [tn] for tn in team_names}
-        expected_group_names = list(allocations.keys())
+        expected_team_names = [t.name for t in constants.STUDENTS]
         assert (
-            expected_group_names
-        ), "pre-test assert, expected group names should be non-empty"
+            expected_team_names
+        ), "pre-test assert, expected team names should be non-empty"
 
         # act
-        api.ensure_teams_and_members(allocations)
-        api.ensure_teams_and_members(allocations)
+        api.ensure_teams_and_members(constants.STUDENTS)
+        api.ensure_teams_and_members(constants.STUDENTS)
 
         # assert
-        actual_groups = api.get_teams()
-        assert sorted([g.name for g in actual_groups]) == sorted(
-            expected_group_names
+        actual_teams = api.get_teams()
+        assert sorted([g.name for g in actual_teams]) == sorted(
+            expected_team_names
         )
-        for group in actual_groups:
-            if group.name != TARGET_GROUP:
-                group_member_names = [m.username for m in group.members.list()]
-                assert group_member_names == [group.name]
+        for team in actual_teams:
+            if team.name != TARGET_GROUP:
+                assert team.members == [team.name]
             else:
-                assert not group.members.list()
+                assert not team.members
 
 
 @pytest.fixture
@@ -407,7 +400,7 @@ class TestGetRepoUrls:
 
         # act
         actual_urls = api.get_repo_urls(
-            master_repo_names, students=constants.STUDENTS
+            master_repo_names, teams=constants.STUDENTS
         )
 
         # assert
@@ -449,7 +442,7 @@ class TestCreateRepos:
         no pre-existing projects. Should just succeed.
         """
         expected_urls = api.get_repo_urls(
-            master_repo_names, students=constants.STUDENTS
+            master_repo_names, teams=constants.STUDENTS
         )
 
         actual_urls = api.create_repos(repos)
@@ -460,7 +453,7 @@ class TestCreateRepos:
         """Running create_repos twice should have precisely the same effect as
         runing it once."""
         expected_urls = api.get_repo_urls(
-            master_repo_names, students=constants.STUDENTS
+            master_repo_names, teams=constants.STUDENTS
         )
 
         api.create_repos(repos)