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

Add teams eventing and tests. #9607

Merged
merged 1 commit into from
Sep 4, 2015
Merged

Conversation

dianakhuang
Copy link
Contributor

TNL-3185 & TNL-3186

Implements 3 of the events from the Teams Events Spec: edx.team.created, edx.team.learner_added, edx.team.learner_removed. Also includes unit and bok choy tests.

Testing

  • Unit, integration, acceptance tests as appropriate
  • Analytics

Reviewers

If you've been tagged for review, please check your corresponding box once you've given the 👍.

Post-review

  • Squash commits

@@ -1107,7 +1129,7 @@ def test_discussion_privileged_user_can_edit_team(self, role):

@attr('shard_5')
@ddt.ddt
class TeamPageTest(TeamsTabBase):
class TeamPageTest(TeamsTabBase, EventsTestMixin):
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't need to inherit from EventsTestMixin, since TeamsTabBase already has it.

@peter-fogg
Copy link
Contributor

Looks good to me, other than my one question about mixin inheritance. 👍

@dianakhuang dianakhuang force-pushed the diana/teams-event-tracking branch 2 times, most recently from 1bf4796 to 9bea1cd Compare September 4, 2015 14:35
@@ -905,13 +909,31 @@ def test_user_can_create_new_team_successfully(self):
And the number of teams should be updated on the topic card
And if I switch to "My Team", the newly created team is displayed
"""

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: why this blank line?

Another nit: document the expected event in the docstring.

"""Test cases for the team creation endpoint."""

def setUp(self): # pylint: disable=arguments-differ
Copy link
Contributor

Choose a reason for hiding this comment

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

Would the arguments-differ warning go away if you were to add *args, **kwargs to the method signature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but then I'd have to handle the 'Unused argument' errors from pylint.

@@ -1053,6 +1077,18 @@ def test_access(self, user, status):
memberships = self.get_membership_list(200, {'team_id': self.solar_team.team_id})
self.assertEqual(memberships['count'], 2)

add_method = 'joined_from_team_view' if user == 'student_enrolled_not_on_team' else 'added_by_another_user'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we expect 'added_by_another_user' for staff, course_staff, and community_ta? Aren't they adding themselves?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, for this test, every user is adding 'student_enrolled_not_on_team'.

@dan-f
Copy link
Contributor

dan-f commented Sep 4, 2015

👍

@dianakhuang dianakhuang force-pushed the diana/teams-event-tracking branch from 9bea1cd to a743f4a Compare September 4, 2015 14:52
@dianakhuang
Copy link
Contributor Author

Only failing bok choy test is the known-flaky video control test. Merging.

dianakhuang added a commit that referenced this pull request Sep 4, 2015
@dianakhuang dianakhuang merged commit deacfeb into master Sep 4, 2015
@dianakhuang dianakhuang deleted the diana/teams-event-tracking branch September 4, 2015 15:32
@dianakhuang
Copy link
Contributor Author

@catong Here's the first one that went in.

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.

3 participants