From 9196817abb1564729cd337b95607429df8881dc5 Mon Sep 17 00:00:00 2001 From: Adam Palay Date: Wed, 15 Apr 2015 17:40:08 -0400 Subject: [PATCH 1/3] Revert "Decrease the number of inserts and updates needed by DjangoKeyValueStore" This reverts commit 88b918747628a5f6020625e603d4a7184c08583e. --- lms/djangoapps/courseware/model_data.py | 56 ++++++++++--------- .../courseware/tests/test_model_data.py | 29 +++++----- 2 files changed, 42 insertions(+), 43 deletions(-) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index 4f2dc29e4f..8a3bcb7007 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -284,7 +284,7 @@ class FieldDataCache(object): def find_or_create(self, key): ''' - Find a model data object in this cache, or create a new one if it doesn't + Find a model data object in this cache, or create it if it doesn't exist ''' field_object = self.find(key) @@ -293,26 +293,28 @@ class FieldDataCache(object): return field_object if key.scope == Scope.user_state: - field_object = StudentModule( + field_object, __ = StudentModule.objects.get_or_create( course_id=self.course_id, student_id=key.user_id, module_state_key=key.block_scope_id, - state=json.dumps({}), - module_type=key.block_scope_id.block_type, + defaults={ + 'state': json.dumps({}), + 'module_type': key.block_scope_id.block_type, + }, ) elif key.scope == Scope.user_state_summary: - field_object = XModuleUserStateSummaryField( + field_object, __ = XModuleUserStateSummaryField.objects.get_or_create( field_name=key.field_name, usage_id=key.block_scope_id ) elif key.scope == Scope.preferences: - field_object = XModuleStudentPrefsField( + field_object, __ = XModuleStudentPrefsField.objects.get_or_create( field_name=key.field_name, module_type=BlockTypeKeyV1(key.block_family, key.block_scope_id), student_id=key.user_id, ) elif key.scope == Scope.user_info: - field_object = XModuleStudentInfoField( + field_object, __ = XModuleStudentInfoField.objects.get_or_create( field_name=key.field_name, student_id=key.user_id, ) @@ -378,39 +380,39 @@ class DjangoKeyValueStore(KeyValueStore): """ saved_fields = [] - # field_objects maps id(field_object) to a the object and a list of associated fields. - # We use id() because FieldDataCache might return django models with no primary key - # set, but will return the same django model each time the same key is passed in. - dirty_field_objects = defaultdict(lambda: (None, [])) - for key in kv_dict: - # Check key for validity - if key.scope not in self._allowed_scopes: - raise InvalidScopeError(key) + # field_objects maps a field_object to a list of associated fields + field_objects = dict() + for field in kv_dict: + # Check field for validity + if field.scope not in self._allowed_scopes: + raise InvalidScopeError(field) - field_object = self._field_data_cache.find_or_create(key) - # Update the list dirtied field_objects - _, dirty_names = dirty_field_objects.setdefault(id(field_object), (field_object, [])) - dirty_names.append(key.field_name) + # If the field is valid and isn't already in the dictionary, add it. + field_object = self._field_data_cache.find_or_create(field) + if field_object not in field_objects.keys(): + field_objects[field_object] = [] + # Update the list of associated fields + field_objects[field_object].append(field) # Special case when scope is for the user state, because this scope saves fields in a single row - if key.scope == Scope.user_state: + if field.scope == Scope.user_state: state = json.loads(field_object.state) - state[key.field_name] = kv_dict[key] + state[field.field_name] = kv_dict[field] field_object.state = json.dumps(state) else: # The remaining scopes save fields on different rows, so # we don't have to worry about conflicts - field_object.value = json.dumps(kv_dict[key]) + field_object.value = json.dumps(kv_dict[field]) - for field_object, names in dirty_field_objects.values(): + for field_object in field_objects: try: # Save the field object that we made above - field_object.save(force_update=field_object.pk is not None) + field_object.save() # If save is successful on this scope, add the saved fields to # the list of successful saves - saved_fields.extend(names) + saved_fields.extend([field.field_name for field in field_objects[field_object]]) except DatabaseError: - log.exception('Error saving fields %r', names) + log.exception('Error saving fields %r', field_objects[field_object]) raise KeyValueMultiSaveError(saved_fields) def delete(self, key): @@ -425,7 +427,7 @@ class DjangoKeyValueStore(KeyValueStore): state = json.loads(field_object.state) del state[key.field_name] field_object.state = json.dumps(state) - field_object.save(force_update=field_object.pk is not None) + field_object.save() else: field_object.delete() diff --git a/lms/djangoapps/courseware/tests/test_model_data.py b/lms/djangoapps/courseware/tests/test_model_data.py index 2ecf717702..770391b142 100644 --- a/lms/djangoapps/courseware/tests/test_model_data.py +++ b/lms/djangoapps/courseware/tests/test_model_data.py @@ -109,11 +109,7 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): # There should be only one query to load a single descriptor with a single user_state field with self.assertNumQueries(1): - self.field_data_cache = FieldDataCache( - [mock_descriptor([mock_field(Scope.user_state, 'a_field')])], - course_id, - self.user - ) + self.field_data_cache = FieldDataCache([mock_descriptor([mock_field(Scope.user_state, 'a_field')])], course_id, self.user) self.kvs = DjangoKeyValueStore(self.field_data_cache) @@ -133,7 +129,7 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): "Test that setting an existing user_state field changes the value" # We are updating a problem, so we write to courseware_studentmodulehistory # as well as courseware_studentmodule - with self.assertNumQueries(2): + with self.assertNumQueries(3): self.kvs.set(user_state_key('a_field'), 'new_value') self.assertEquals(1, StudentModule.objects.all().count()) self.assertEquals({'b_field': 'b_value', 'a_field': 'new_value'}, json.loads(StudentModule.objects.all()[0].state)) @@ -142,7 +138,7 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): "Test that setting a new user_state field changes the value" # We are updating a problem, so we write to courseware_studentmodulehistory # as well as courseware_studentmodule - with self.assertNumQueries(2): + with self.assertNumQueries(3): self.kvs.set(user_state_key('not_a_field'), 'new_value') self.assertEquals(1, StudentModule.objects.all().count()) self.assertEquals({'b_field': 'b_value', 'a_field': 'a_value', 'not_a_field': 'new_value'}, json.loads(StudentModule.objects.all()[0].state)) @@ -151,7 +147,7 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): "Test that deleting an existing field removes it from the StudentModule" # We are updating a problem, so we write to courseware_studentmodulehistory # as well as courseware_studentmodule - with self.assertNumQueries(2): + with self.assertNumQueries(3): self.kvs.delete(user_state_key('a_field')) self.assertEquals(1, StudentModule.objects.all().count()) self.assertRaises(KeyError, self.kvs.get, user_state_key('not_a_field')) @@ -188,7 +184,7 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): # Scope.user_state is stored in a single row in the database, so we only # need to send a single update to that table. # We also are updating a problem, so we write to courseware student module history - with self.assertNumQueries(2): + with self.assertNumQueries(3): self.kvs.set_many(kv_dict) for key in kv_dict: @@ -232,7 +228,7 @@ class TestMissingStudentModule(TestCase): # We are updating a problem, so we write to courseware_studentmodulehistory # as well as courseware_studentmodule - with self.assertNumQueries(2): + with self.assertNumQueries(6): self.kvs.set(user_state_key('a_field'), 'a_value') self.assertEquals(1, len(self.field_data_cache.cache)) @@ -285,7 +281,7 @@ class StorageTestBase(object): self.kvs = DjangoKeyValueStore(self.field_data_cache) def test_set_and_get_existing_field(self): - with self.assertNumQueries(1): + with self.assertNumQueries(2): self.kvs.set(self.key_factory('existing_field'), 'test_value') with self.assertNumQueries(0): self.assertEquals('test_value', self.kvs.get(self.key_factory('existing_field'))) @@ -302,14 +298,14 @@ class StorageTestBase(object): def test_set_existing_field(self): "Test that setting an existing field changes the value" - with self.assertNumQueries(1): + with self.assertNumQueries(2): self.kvs.set(self.key_factory('existing_field'), 'new_value') self.assertEquals(1, self.storage_class.objects.all().count()) self.assertEquals('new_value', json.loads(self.storage_class.objects.all()[0].value)) def test_set_missing_field(self): "Test that setting a new field changes the value" - with self.assertNumQueries(1): + with self.assertNumQueries(4): self.kvs.set(self.key_factory('missing_field'), 'new_value') self.assertEquals(2, self.storage_class.objects.all().count()) self.assertEquals('old_value', json.loads(self.storage_class.objects.get(field_name='existing_field').value)) @@ -351,7 +347,7 @@ class StorageTestBase(object): # Each field is a separate row in the database, hence # a separate query - with self.assertNumQueries(len(kv_dict)): + with self.assertNumQueries(len(kv_dict)*3): self.kvs.set_many(kv_dict) for key in kv_dict: self.assertEquals(self.kvs.get(key), kv_dict[key]) @@ -359,8 +355,8 @@ class StorageTestBase(object): def test_set_many_failure(self): """Test that setting many regular fields with a DB error """ kv_dict = self.construct_kv_dict() - for key in kv_dict: - with self.assertNumQueries(1): + with self.assertNumQueries(6): + for key in kv_dict: self.kvs.set(key, 'test value') with patch('django.db.models.Model.save', side_effect=[None, DatabaseError]): @@ -369,6 +365,7 @@ class StorageTestBase(object): exception = exception_context.exception self.assertEquals(len(exception.saved_field_names), 1) + self.assertEquals(exception.saved_field_names[0], 'existing_field') class TestUserStateSummaryStorage(StorageTestBase, TestCase): From 9e525d6b5c98dc8059a1addd4de7abd2889b63b1 Mon Sep 17 00:00:00 2001 From: Marco Morales Date: Thu, 2 Apr 2015 16:02:30 -0400 Subject: [PATCH 2/3] added images to the bulk email directory to include Youtube as a social icon option --- lms/static/images/bulk_email/YoutubeIcon.png | Bin 0 -> 1964 bytes .../images/bulk_email/YoutubeIcon_gray.png | Bin 0 -> 1961 bytes 2 files changed, 0 insertions(+), 0 deletions(-) create mode 100644 lms/static/images/bulk_email/YoutubeIcon.png create mode 100644 lms/static/images/bulk_email/YoutubeIcon_gray.png diff --git a/lms/static/images/bulk_email/YoutubeIcon.png b/lms/static/images/bulk_email/YoutubeIcon.png new file mode 100644 index 0000000000000000000000000000000000000000..0eda866d6e90300c2b1507b81946d2e51468eb4e GIT binary patch literal 1964 zcmV;d2UGZoP)A1_eZpa>YZ!WxkIqt;UvK)sj-zv8(Rw+Z-l;VE5?h!ulUO!eE z6Ju4fC7|Hc!m6Boq|npd3vkSS&jd^6MF}TZK`ZT<>ssmsIKHT4y=8|47_{JKOUifs za;l^%Rs)z;TY*AJ>@m=8o;qdo;(RKG=K@YPG@OlP3Oh}SOW5(t{T+2 zLhAf$Mu1lu5#Sr3c+_Cfb*9`2O2P83prsO^ud;pSNmk`YrhGS3en-Q;7YBSplk=PGHSP+bhKO7E}LM&`7_P4y)2z z8+iPGpkAQ1s3W8-mDp`&2DqC*P^U9hpkW51^Eps=!?iZ6avP|S)GyVhLHy6^PwYF* z4DhNKirVj~{i0Qg!@v76qwjmRm$`%v*4yMm+b^xBY@n!aNIZj%5*lU)Lo042a^2lyl@rz_N1#C}gb0Do@o0H=`h z=ZSr7kN^v`XKkBfC8%9<1h|uww*#ev-UW3d_E}6&iY_Mf0C4>|Pd!lqkd1+C=G0La z7RJ`I8q*DQmejBCS#`mq>z3mSJ8kPuxS8R zpw5H-B+Aj&3LB;z<}=Te?=v01i#YZ89OJ-~L8CAeOI56;u@Ch->b`Fj`Y<1T+KWC$ z6u>gjIxQehqi&IU=#y^M*N^adS~$R&7PB5!Kvrng?N!|$hmP^U_N{RhlkvZ!vwHg8|msCxf65&%erh;b$XvzLe~FYOM;O2i5dbp+IF-)SAJBjcqyR62+J_lnx(I;t0ywpRsXvS< zR}(Ckp_Y0BL{{YqVo&1$RIs~v6N6KWIoV%oYnThl2YruP4Z6#~*BK%J&TJsS$xL}) zP7r7{2jEpqE~n=n2i;B{)d~%KcKi9EpEFwoz}XE1IDwSi^xhp-Wh1dK=K!S7;Pmeo zvFz&aShqz`Cyy+jY(bA12FTt#O|*&*wrd2IA|7`uu!C1i&h7{$vcW2j~H!_MAEi z81I-zma}@3bCO8CTH8=;st@7|eqQhkGU?@$a8-P}7K727e2jsg*F}XjpN)dJ@I;Yu z;Z0EhaJ5K)9-qxF=7Sf@Y@HC2T%^9=rK1Vqb=1PE5t1Kx8uTgd=ScCJXGI3Ew&>e< z^!0Q&^8>^A2FnUue>;p%2Wv1wHWxeWRH8oG=xik4i#Mv5$b9BR@_lR2hr{Soc(PV? zfG#h>za>9X<`{2#A)e=tduR%_+U-C^XXrJX+A>U#ioMLQBVcHKRz6tYvv)yew z>~gNxAZF_CO4k3C^6~&St!+xdz+87v!_j^~zIzh1ZyV@bg5Bkiayfe1T^Ew_T>I2! y3wK^{iA}}Vs^ejpj+3g1Q%BpAo~jrgr~d)xN{-t2W2AnOI|1~Egh{?I{N58C1ds+IyGYdv9h*0Uq^aQVGm1MdbZVSxsYnL z-n3XOx0EcmfXB^LtL3WMnz}iCd0xB_p2?n{U^W>`O(vtuY&N^hCez(lC6LL(#AMLd zSnat}8o*n(bx#MqL0_z(1tx<&<Qj;xoB(As8eC}^S^0P1Ro1zp z@(LV9DJdxmPQU<-E}P9(cDkyz%N>Xghhw_}7&O4jnp$U9H^40lV9)?7>YdJxO$G20 z=pw4xOPzm7oy%GT=xjoOAAuf34FO%_$}2#9u)H%USpjUQjdYj3%K&Rq--GQx&rurO(&3^KpmMaI1>KMKD9~>TpjcPA z+jt||3yJMf&<(-4`#||18)ysm*nFK^pfXS?s-BdU6ML;z0WwV`u;!rcePa8RtN%M_ zBp>JiTVK}k2>wA2gW93?k+R{$zFx}!iwFcZoVf`a=B0N&4eIVK)COg3LAy!)D{L6# z<+BD5`*&IfSnYpx)A#eZc&N`6ME1fLZkuY*FaVY>af7V z^FE?6T|lQu{dy6wMGNL9r6n(jBj>G-1b{5m^aRm1AJLm>pibPcaUAFmLH$jjI2NP; zJqy}pvRGW8$>h6sMFPNJRe=}26XS&b7O*NKF{B^ynF$J@0;m8gfC^yq0IWxqfc_$4 z!;suYY7_UFCFJ`w2kOL%vS@>r=Kg51Ss1!&Z1{1D{lG%LWd#F18@ zjsYq<$cwy}oXgs4+n}KxaTz0mu~OMNTE>9t~w@g*n<)G5{V2 zwIPngVa{G4=h}n3$Pc(cYR3@Y#)oA9+`~B$48SEpUf^tUZdDTiTqgryj-Z`NAc4$G zuKqiuEH2CdX3GGW?Z>G^uKvJATp$H_A=EP(fv?R1aJC<(KH=&Qr8t;BNo*`be*kC&_bcqg`7&4rz<5aj&$I0dXs64g-+)@3$h#Ty(E_7UPrG|@ z*#qg%n+RZb2m#K*darQ4@NM7}830#?7@+$h6@LaW4s0zL2d)x~1Kq#C3EVi42l$l? zfO(AsIG*bnb8$}V@&oWX?xlN;OKV$!d+?4E+b`I@(Hy}N!7aQ(2Ec=U04DhX81LnC z=??QWA9G<|Grj!dcx4ED-6;d$8TYdfya43)!wnZ~pgzD2aM{J@5nDRe{gBwM2`;#p zgu~k4K4X#3EK1Xv^MlQwkH`SXGU5mO0LYTQpCoE8s1u9p9kWR|t1mgHH>u~g#pA8| z6h7eRO@5IH%O_w|oQ22ic+218#m{T9mNj3GfOX*sGS`K3BLLuHnE)@+%`O^*cOSQP zLP~NGTbJmdL3kaoZ48V2z!M+~?&s*kkF&xASUdD>9Qt}9Tr*VbG5%JJvx6nLLN*Hr z&%=rOVM3%(+{JsV*OU9q@#OoKpby39Q%JH_8$5vV`fXQKp~JC5QjT;Tu4{-?hU2}> z*iLz?ccr=gc0U62a&_H9x{6v?)T(^P8kJ-o9KcMQ?c~YxwRgv#D+|_5Qjb9n0-(X* zT4b|ZJq_&o?R#4p%$C(=_fyQ3d_L72o+Ev1dBG12ri40!y|9MJf~zk$5=p{cO>0rUkJav?taHi v&^Sg{Ua5G;Ew+Q_?_0Xdr!)100000NkvXXu0mjfb4-_N literal 0 HcmV?d00001 From 987ea8551e058d8ff8d09a2eed6d0ce276b391dd Mon Sep 17 00:00:00 2001 From: Adam Palay Date: Thu, 16 Apr 2015 10:08:13 -0400 Subject: [PATCH 3/3] fix quality violation --- lms/djangoapps/courseware/tests/test_model_data.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_model_data.py b/lms/djangoapps/courseware/tests/test_model_data.py index 770391b142..99e2f538ab 100644 --- a/lms/djangoapps/courseware/tests/test_model_data.py +++ b/lms/djangoapps/courseware/tests/test_model_data.py @@ -109,7 +109,9 @@ class TestStudentModuleStorage(OtherUserFailureTestMixin, TestCase): # There should be only one query to load a single descriptor with a single user_state field with self.assertNumQueries(1): - self.field_data_cache = FieldDataCache([mock_descriptor([mock_field(Scope.user_state, 'a_field')])], course_id, self.user) + self.field_data_cache = FieldDataCache( + [mock_descriptor([mock_field(Scope.user_state, 'a_field')])], course_id, self.user + ) self.kvs = DjangoKeyValueStore(self.field_data_cache) @@ -347,7 +349,7 @@ class StorageTestBase(object): # Each field is a separate row in the database, hence # a separate query - with self.assertNumQueries(len(kv_dict)*3): + with self.assertNumQueries(len(kv_dict) * 3): self.kvs.set_many(kv_dict) for key in kv_dict: self.assertEquals(self.kvs.get(key), kv_dict[key])