Skip to content

Conversation

@cahrens
Copy link

@cahrens cahrens commented Aug 26, 2015

@dan-f WIP PR.

Copy link
Author

Choose a reason for hiding this comment

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

@dan-f How will this field get populated for teams with existing memberships?

@cahrens cahrens force-pushed the christina/composite-index branch from 9ab9d37 to edb9cab Compare August 27, 2015 17:50
@cahrens
Copy link
Author

cahrens commented Aug 27, 2015

Note that this will need a rebase-- my new PR is https://github.com/edx/edx-platform/pull/9503. And migration number 5 changed, so you will likely need to back up and redo migrations.

@peter-fogg peter-fogg force-pushed the dan-f/teams-performance-cache-team-size branch 2 times, most recently from ea04ef7 to 8404b2c Compare August 27, 2015 21:41
@peter-fogg
Copy link
Contributor

@cahrens I've removed the is_active field from teams, and added team_size with the appropriate indices.

Copy link
Author

Choose a reason for hiding this comment

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

How can we be sure that memberships will never be created in this way (this would not update the count, correct)?

Copy link
Contributor

Choose a reason for hiding this comment

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

This way should update the count, I believe, because the count gets updated on the save of the team membership.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct. I changed these lines to call add_user just because it makes sense to test membership creation in the way that the actual application code does it.

@cahrens
Copy link
Author

cahrens commented Aug 28, 2015

Nice work, @peter-fogg. I just have that one question (wondering if we should handle the addition in the create method of the membership itself). Otherwise 👍, and this is certainly in good enough shape to test with tomorrow.

Please let the devops person who deploys the Ami to the load testing environment know that migrations need to be done.

@peter-fogg peter-fogg force-pushed the dan-f/teams-performance-cache-team-size branch from 866b944 to 56846a2 Compare August 28, 2015 13:33
@peter-fogg
Copy link
Contributor

@cahrens I had originally thought that we couldn't prevent someone from creating a membership with CourseTeamMembership.objects.create, but it turns out we can check in the save method if there's a primary key set on the model. If so, it already existed in the database, but if not, it's being saved for the first time. This should ensure that team sizes are always correct. I've also fixed up some quality violations and squashed commits.

@peter-fogg
Copy link
Contributor

@dianakhuang Can you give a second review if you get a chance?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This is an odd change, just because it doesn't really do anything differently?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I'd been experimenting with some things and forgot to change that back. I'll fix it up.

@dianakhuang
Copy link
Contributor

Looks good to me. 👍

@cahrens
Copy link
Author

cahrens commented Aug 28, 2015

@peter-fogg Since there is no rush with trying to load test today, I'll wait until this PR merges to my branch before rebasing on master.

@peter-fogg peter-fogg force-pushed the dan-f/teams-performance-cache-team-size branch from 56846a2 to 9e98a32 Compare August 28, 2015 14:56
@peter-fogg
Copy link
Contributor

The only failing JS tests are the known flaky discussions tests, so I'll go ahead and merge this.

@peter-fogg peter-fogg merged this pull request into christina/composite-index Aug 28, 2015
@peter-fogg peter-fogg deleted the dan-f/teams-performance-cache-team-size branch August 28, 2015 15:34
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.

4 participants