Skip to content

Fix display course specific user teams#10804

Merged
mushtaqak merged 1 commit intomasterfrom
mushtaq/tnl3825-fix-show-user-course-specific-team
Dec 7, 2015
Merged

Fix display course specific user teams#10804
mushtaqak merged 1 commit intomasterfrom
mushtaq/tnl3825-fix-show-user-course-specific-team

Conversation

@mushtaqak
Copy link
Contributor

@mushtaqak
Copy link
Contributor Author

@awaisdar001 @Shrhawk can anyone of you do initial review please ?

@mushtaqak mushtaqak force-pushed the mushtaq/tnl3825-fix-show-user-course-specific-team branch from eed250b to bc8013a Compare December 3, 2015 16:21
@mushtaqak
Copy link
Contributor Author

@adampalay can you please review :)

Copy link
Contributor

Choose a reason for hiding this comment

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

what's this re doing? Is there a way to do this without regular expressions?

@adampalay
Copy link
Contributor

this looks great; just a comment about the test :)

@mushtaqak mushtaqak force-pushed the mushtaq/tnl3825-fix-show-user-course-specific-team branch from bc8013a to e513c94 Compare December 7, 2015 12:49
@mushtaqak mushtaqak force-pushed the mushtaq/tnl3825-fix-show-user-course-specific-team branch from e513c94 to 1d57ef2 Compare December 7, 2015 13:46
@mushtaqak
Copy link
Contributor Author

@dianakhuang @adampalay May you please review. Thanks.

@dianakhuang
Copy link
Contributor

👍 Great that the fix was simple. And the tests are good.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, I didn't realize we were expecting JSON here! Why don't you do:
team_count = json.loads(response.content)['teams']['count']
and then check team_count is what you expect it to be?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've been corrected — JSON isn't being returned here.

@adampalay
Copy link
Contributor

👍

@mushtaqak
Copy link
Contributor Author

@adampalay @dianakhuang Thanks. I will merge now :)

mushtaqak pushed a commit that referenced this pull request Dec 7, 2015
…se-specific-team

Fix display course specific user teams
@mushtaqak mushtaqak merged commit e49f90e into master Dec 7, 2015
@adampalay adampalay deleted the mushtaq/tnl3825-fix-show-user-course-specific-team branch December 7, 2015 16:56
robrap added a commit that referenced this pull request Jul 7, 2023
The original tests looked like helpers,
but we are guessing that they were supposed
to be tests.

Additionally, one test had a small bug as written.
It would be more resilient to actually parse the
JSON, but that work is being left for a later time.
For now, it works, but is not resilient.

The original PR can be found here:
#10804
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