Make matching group ID condition be more specific - #14748
Conversation
| for user_partition in user_partitions: | ||
| for group in user_partition['groups']: | ||
| if str(group['id']) in xblock_display_name: | ||
| if _(u'Group ID {group_id}').format(group_id=group['id']) == xblock_display_name: |
There was a problem hiding this comment.
@cahrens do you think this check would be sufficient ?
There was a problem hiding this comment.
I added this because xblock_display_name is actually retried from xblock which has same translated version, that is displayed to user (as a translated version), that is why I am checking this.
There was a problem hiding this comment.
This is better, but there are still issues with the general move functionality with regard to split_test instances. I documented those in https://openedx.atlassian.net/browse/TNL-6754.
It's also theoretically possible that someone could create a unit/vertical/sequential with "Group ID XXXX", and you would improperly replace it, but it does some like a very remote chance. Though remember that XXXX may be something just like 0, as in "Group ID 0" or "Group ID 1" (as you are iterating through all user partitions). You could further reduce the likelihood of incorrect matches by only looking at user_partitions that are related to the scheme "random" (RandomUserPartitionScheme), which is what the split_test module uses; those UserPartitions are likely to have the large integer group_id values that you've been seeing.
There was a problem hiding this comment.
@muzaffaryousaf @CAR So, are we going with this solution for now?
There was a problem hiding this comment.
I think you should at least only iterate over content experiment user partitions:
from xmodule.split_test_module import get_split_user_partitions
5fd6fa8 to
d7f8e6c
Compare
| for group in user_partition['groups']: | ||
| if str(group['id']) in xblock_display_name: | ||
| return group['name'] | ||
| if user_partition['scheme'] == 'random': |
There was a problem hiding this comment.
@cahrens user_partition dict has a scheme key, so I am using it to only iterate random scheme user partitions now. @muzaffaryousaf FYI
There was a problem hiding this comment.
You can avoid having to know about the scheme name by using the helper method get_split_user_partitions in split_test_module.py.
for user_partition in get_split_user_partitions(user_partitions)
There was a problem hiding this comment.
@cahrens We can't use get_split_user_partitions method because it requires UserPartition as argument, here we have dict containing user partitions and their info.
I can get partitions but that would be an extra call because create_xblock_info method already calculates and uses user_partitions that we are passing to our method get_group_display_name(user_partitions, xblock_display_name)
There was a problem hiding this comment.
Oh, ok, stick with what you have here then. 👍
|
jenkins run js |
1 similar comment
|
jenkins run js |
|
@cahrens @muzaffaryousaf quick review ? |
|
I will squash and rebase after I get thumbs-ups :) |
|
jenkins run bokchoy |
|
Don't forget to squash before you merge. |
c2b437e to
1fb9a65
Compare
There was a problem hiding this comment.
note to reviewers: Used schemes=['random'] here instead of doing it again in get_group_display_name method.
@cahrens FYI
There was a problem hiding this comment.
I don't think you can/should assume that every time "is_concise" is True, you only want the random user_partitions. If that's the case, the property is very poorly named.
There was a problem hiding this comment.
is_concise is used when we want the concise version of the xblock info (in-short has very few key-values), and it is only used for the moment in Move modal, and group name to id issue also appears when we have concise version, otherwise user_partitions won't be used here.
There was a problem hiding this comment.
OK, I understand now where user_partitions is used.
I think the clearest thing is to not pass user_partitions into get_group_display_name, and instead have that method fetch the user_partitions it cares about (the random scheme ones).
I'd also recommend making the method name and doc clear that it is dealing with groups within the random UserPartition only. All UserPartitions have groups. Perhaps "get_random_group_display_name".
There was a problem hiding this comment.
Good Idea, I will change that :)
|
@muzaffaryousaf @cahrens Final review ? |
|
👍 Thanks for iterating, @mushtaqak! |
c6e51a2 to
6b9f853
Compare
|
EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Friday, March 31, 2017. |
|
EdX Release Notice: This PR has been deployed to production. (We had an infrastructure issue, so this message was posted manually.) |
5 participants
https://openedx.atlassian.net/browse/TNL-6731