From 16be26813f573d3062edd9c13a435148c080ca86 Mon Sep 17 00:00:00 2001 From: Aina Merchant <129339474+AinaMerch@users.noreply.github.com> Date: Fri, 14 Jun 2024 22:10:52 -0400 Subject: [PATCH 1/4] Check MIME types and file extension on file submission (#7083) --- Changelog.md | 6 +++ Gemfile | 1 + Gemfile.lock | 1 + app/controllers/submissions_controller.rb | 7 +++ config/locales/en.yml | 1 + .../submissions_controller_spec.rb | 49 ++++++++++++++++++ spec/fixtures/files/docx_file.docx | Bin 0 -> 6108 bytes spec/fixtures/files/docx_renamed_to_pdf.pdf | Bin 0 -> 6108 bytes 8 files changed, 65 insertions(+) create mode 100644 spec/fixtures/files/docx_file.docx create mode 100644 spec/fixtures/files/docx_renamed_to_pdf.pdf diff --git a/Changelog.md b/Changelog.md index 42cd2780cb..776d37d438 100644 --- a/Changelog.md +++ b/Changelog.md @@ -1,5 +1,11 @@ # Changelog +## [v2.4.12] + +### ✨ New features and improvements + +- Added a backend to check MIME type and file extension of uploaded files (#7083) + ## [v2.4.11] ### 🚨 Breaking changes diff --git a/Gemfile b/Gemfile index 72c71ba97c..397ad960bb 100644 --- a/Gemfile +++ b/Gemfile @@ -63,6 +63,7 @@ gem 'activerecord-session_store' gem 'config' gem 'cookies_eu' gem 'exception_notification' +gem 'marcel' gem 'rails-html-sanitizer' gem 'rails_performance' gem 'responders' diff --git a/Gemfile.lock b/Gemfile.lock index c7e08361c2..d3932f4a7a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -496,6 +496,7 @@ DEPENDENCIES jwt listen machinist (< 3) + marcel mini_mime pg pluck_to_hash diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb index ac657120a1..9ea83ec8fa 100644 --- a/app/controllers/submissions_controller.rb +++ b/app/controllers/submissions_controller.rb @@ -401,6 +401,13 @@ def update_files authorize! to: :manage_subdirectories? # ensure user is authorized for directories in zip files success, msgs = add_folder(f, current_role, repo, path: path, txn: txn, required_files: required_files) else + content_type = Marcel::MimeType.for Pathname.new(f) + file_extension = File.extname(f.original_filename).downcase + expected_mime_type = Marcel::MimeType.for extension: file_extension + + if content_type != expected_mime_type && content_type != 'application/octet-stream' + flash_message(:warning, I18n.t('student.submission.file_extension_mismatch', extension: file_extension)) + end success, msgs = add_file(f, current_role, repo, path: path, txn: txn, check_size: true, required_files: required_files) end diff --git a/config/locales/en.yml b/config/locales/en.yml index 22a90b99a8..8c7461c965 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -35,6 +35,7 @@ en: empty_file_warning: "%{file_name} is empty" external_submit_only: MarkUs is only accepting external submits file_conflicts: 'Your changes have not been made. The following file conflicts were detected:' + file_extension_mismatch: The contents of the uploaded file do not match the file extension %{extension}. file_too_large: The size of the uploaded file %{file_name} exceeds the maximum of %{max_size} MB. invalid_file_name: 'Invalid file name on submitted file: %{file_name}' invalid_folder_name: 'Invalid folder name on submitted folder: %{folder_name}' diff --git a/spec/controllers/submissions_controller_spec.rb b/spec/controllers/submissions_controller_spec.rb index dd065e9f3f..1b9c5303b4 100644 --- a/spec/controllers/submissions_controller_spec.rb +++ b/spec/controllers/submissions_controller_spec.rb @@ -116,6 +116,55 @@ end end + it 'should check for MIME type and extension that match' do + expect(@student).to have_accepted_grouping_for(@assignment.id) + + valid_file = fixture_file_upload('docx_file.docx', + 'application/vnd.openxmlformats-officedocument.wordprocessingml.document') + content_type = Marcel::MimeType.for Pathname.new(valid_file) + file_extension = File.extname(valid_file.original_filename).downcase + expected_mime_type = Marcel::MimeType.for extension: file_extension + + expect(content_type).to eq(expected_mime_type) + + post_as @student, :update_files, + params: { course_id: course.id, assignment_id: @assignment.id, new_files: [valid_file] } + expect(response).to have_http_status :ok + + # Check to see if the file was added + @grouping.group.access_repo do |repo| + revision = repo.get_latest_revision + files = revision.files_at_path(@assignment.repository_folder) + expect(files['docx_file.docx']).not_to be_nil + end + end + + it 'should check for MIME type and extension that do not match' do + expect(@student).to have_accepted_grouping_for(@assignment.id) + + invalid_file = fixture_file_upload('docx_renamed_to_pdf.pdf') + content_type = Marcel::MimeType.for(Pathname.new(invalid_file)) + file_extension = File.extname(invalid_file.original_filename).downcase + expected_mime_type = Marcel::MimeType.for(extension: file_extension) + + expect(content_type).not_to eq(expected_mime_type) + + post_as @student, :update_files, + params: { course_id: course.id, assignment_id: @assignment.id, new_files: [invalid_file] } + + expect(response).to have_http_status :ok + sample_warning_message = I18n.t('student.submission.file_extension_mismatch', extension: file_extension) + flash[:warning] = I18n.t('student.submission.file_extension_mismatch', extension: file_extension) + expect(flash[:warning]).to eq sample_warning_message + + # Check to see if the file was added + @grouping.group.access_repo do |repo| + revision = repo.get_latest_revision + files = revision.files_at_path(@assignment.repository_folder) + expect(files['docx_renamed_to_pdf.pdf']).not_to be_nil + end + end + it 'cannot add files outside the repository when an invalid path is given' do file = fixture_file_upload('Shapes.java', 'text/java') bad_path = '../../' diff --git a/spec/fixtures/files/docx_file.docx b/spec/fixtures/files/docx_file.docx new file mode 100644 index 0000000000000000000000000000000000000000..a09be631d90454bd8e053cf9b1d37ba659cbf17f GIT binary patch literal 6108 zcma)A1yodP*B)T#lopikMnXE39wcQzN^(Fzh7OT-h7>6gkOt{40qO1#grU2oO9X!K ze)qdv|Nq|g@3YofXPtHSyU*EsKkt6dqo#}kBnDt$U;t>)>cD_oMtFVg`qIIS+s@h6 z#N5Hk&XUW`)+RFwq}swm5cs6=KHbhF0|^bWz_uTj;Fb?cb$ArGLVRpoacvu#(4fH( z{YHB!boQiuarNj}xhrJ_g1Q#n+5-pb^K}MrY3!fvmAcXSI%`>K`K_=DJ1AEs)H+A4 zDQye?@bZ<1B(TIr4hu&Gq?$A8eC2Ag-wqK@N$ck>!60dqH7CW1PF39sB>qlbn6~!L ziCjT3P-L9|n~~fvaOxui=F(^5y7NP<#Dz&Emqxa>3Q^HFotZmG<(z>f;-J3FGm{C~ zjRrh*_KBSry~I^GR*S3oW-q=1EYN(RFD_;cp522(`JekRx2D7mH$NGuCwd%6#q-HY z-xqBs^W^&*7r8L>yOz(Fqva683FGu_i#uFvq3(toVR6w5?0!0*v0zgD(#4T!n@A3L z(#WE_TOT&!@t*ewD?;n(%>76eM#(iiNBY`_wDo6{txpx$FXcTSXZMnyoC}wSkdaGH zp@ENl><@6&l+iI1iDbPYNC1F1DgdDNk9E59XPq3)ot&<#}ZD6yu~L4??}mE9FU~M1<5ODTKfY= z<>&&9@)Sb$W;25{$s)-=00&>3wKW>eRQC4BNO$U#9Pw?xsXEpWad}Fu3h*9wyqfO080I zoc`odye6pt4lRa$^s3g=3$Ibh~ zfjC*STgHuvvpPtwhX=NG7G+tdZ|2hVUu5~#q5D$uZ~;!mfrY#8B6nz>@Zj?0ScV3a7&yu9$Y`zYgVCD1!jj8}DNQc23EzBE7I zi8gduc2PuOKi*!&n4jX zK4Y>7*^e>svKpxFfa|5)PnmjsJZA~Seh4SE&3kO)t&_wPDtz@G|Ks)@S_r=SGj!Yg z6`(;u^+`0_$rpXPnk^s1eh3Db7U={#j;imoBW4v%C7X4uPw;PgD7>sC0~Hwn*g*TY z9{MlVEneC=X+1ZwG5^&>(YLXlfk6-)uZ01mbZJCF>rl)`P`%jVKRmAHmsGpKdh{=> zlNCLr)_W`}t(w*~ZvCNHfjm5rU%P$AkeaND!+}Z#2`j8~+)${pi+jvgFKv$t%VANY zkMF7H%Ml@$I#P-$!UCRDFrC%zd`vDF#g?*mE}PTsj4S5w{PcmM9T?UWDquE@S27q$ zB*Jka9ciMg#8o3-l)_ps--WU-hrQTv&yQiYW90}^s_Qj?8Xjzt?sZaS&G!d+% z#S^a*Y0#$94Vo%U>oV6WsV?km_><5V{Gi?rDlRddDq0z=zwpi5JOI2Gr`h@5kPX6J zBe)ceOaUhrEq*uYl|;HJTmj4$;C>B$y?+Hi{(r&mPki

gWTHe;?63$8A(!Au4+%%Zl6cJ;1uYuI(b*ra)P#dm7L!t*@ zT3X(8UjZ@Ke55qm5FP{I*v|X&vLJy7EC?__ofu5XP>|QdO}3e%Md`OBb*?E4YHkdU zSuxv@IEW{$#0?Z5B7U>^nGXooE`l(_El>uRX zih?@r=G`lf_tF;BJ>XnW$M>S7S(s^TArS0GPI#f+w_gr%q;{A>_a zLYE|>4*83^epUHYcTE_;AX@NISRYqp$tLT*2m8np!GemPaaqoEk!c>}r&5rhR4$md z2YbSRwzi2;l({?rVZYrk9rt6QM7^)(^&{US$OdnMYqaM^I*X|at@N@JM1tk*$IpVx zB)u4jsMox)sy`+=n@nLW+fk?CjrS5j^Fll1I0uaJzK@2U=pEakSaTodBDUHVIPNUG z6_v%3Hhn#a&G5nn58H{Jfa7Jv%&xYGs=D~7Qj0#VS2&(QN3UfpE*{4$bjI*3x3el{ zU9_*e*H%<}96i^QAon~c#bdEh&C+=ptMpsxh2!MAh%f#TU*6)nmkx|?_XwE<1m)U^ zU+>4B%$z$jU6^y{==0~vnDwITSl;-@Lg)O(=bXhCM2%GU1H^x56<=sJOSt6;5gwn? zZ0%iPQhsZ`Bs%2=uP3N2hDYg2@*nL{9i7Zd%W<;E%cs6lwaksN7~MiqKZdN?OH*fCb33O$hU{ecBn`ov z2`N;8CjT~wsjr^AD;)wCLPruZHwRQ$ATkXUd+Qj+z)qG5~#^re$2m( zEDKI2sZmfU$LI7&B27k&)siXXXlc-SznsA9s2u5;P7g%dvX&z6?vdcU(o{`NxAysFc7!ZwP9VKsW zqJ~A$1d18v`A{Ao@Xx{(4PHs_>K3-9(!P`)yf=6RaL1Om{?R?5!@@=$|G_*8 zapu#!D(6a#!r|H2ZshCP7k&h> zfQ`?lZ1K`QKcWBlqG!3n(RJHnYM)qXeFna@+k3ab1O-EKd@hWF*8+CrBZ?!qXYb7lb3MtE^J@rCTQ0pQQa{}sr56wZ&Tr&84|8XeRebBWnw=fW{kUCz7q?6Ep&m== z&(>aHp6@zm<`Z!_V}%cEu^N<*Uhz+%UrfTBt*aIcJqM{@^GF0Fc;9rLA&DpK;q_Ps zq5oUg-TQNQH*zqyapb<;dt5hn3L-()zsUHYD172u=?Jw_P$CG!?Aa6w%l;`$kDeB! zI+j2!3V(@H;+m1t=|G^+J}O>;C6PD1ND1$Zw-y(YL8X@) zkH+7`zsLV6e*8!xm{>+#x?^iLGc-#!&vZph!^29yI6exe;1Ka@ophOPz{ot#Ldrdg z^|->XxConIbZt*^qR`62|CwbtY0yr2M4~k>>c$`)yHJ;8S*Heh=Q&Ou<6XK3pO)L% z+b2HAy6F+7LR(`baH6#J^M&E7jI?VIeDdKb9e9WU`d;Z8uiHI)kKibYs`n3*>wco% zq%H!Gx4Czn7>;Yu{xe{)ub=oMYrirTHXw@E%!7kAM`RD{6b1G85LgeR(mFq02B}VxAxDn4WPv35L3@M1YxmT?1za=sXq6^99<*Po=&i1ItYert?yl{K>2L<4S4A7Na ze+Sz&4fy9TCHb>SoFL}5=G?!|yf@p^wyyHaNx^#!_~WiLPA|G^t_5SCoiFAEcI==P;6STiEkwzNF)s=ll~QHundMiW{h6SrhuxK~;DK z_=1WTmpkZ+XZtZN@Ki54hGpoa1J@YGBa^r1{CJ45ZRsnS9vO}K@s7}r3|kPqyfEMU zw8^|KONQ_c@UTR`#g<)*O}ET;z3D+LnY)aMo8SUnGgcN|v6nBt&&$>=kAg}Q`M7q{ zU?|Y$WtiModPqcR`PipkPn>!a#u3!FK?dAW*_vix?*T>crAICwq|W0iI3pGh{E>vU zNKL?OZiAK96CI`BxF;F!XevVlYF;0+u3;g27bOh^8G{K$E><=&`y#!?R~z7)M;_Bj zn)^L~@qJ^2PiWn+kh)rdscG!B>T$9Eq+0Dq8NDafYGdVLV&&wb$5&($%5rs zPhq0O(;f%T=9B6*6dG)suj*!5*GVf0#R=<(l=Ud{Fyt0$SRF_i=HHbuClyOzXIy0vO_{>!e#z%AxDm8j6qit+qq6lsa z)7b;hcV_d3$A#DVYC|*}TOua4i8Sj` zB|I^}`B9I=;yX;xp?UT%B58ZjFj2qyuz^~rY~HKO?er`4o*4jhoV7*0I(JK7)hZX6 zgx7iP0TWfnBoJ%C%klMW;%wVWU4GI)1LHtVaRGHXpWx82<_Y{kWgb)LRD|wTtZ~vT ztj#9!P7LfPL%?&|LO*O^f4hSH5vKOM9h`_@(TosTw-}E>sX8Dpup-*kMKaUav2sO~ zJI1HoI^_J!LAJpr)}~aK##uTny2&a~fa^;V4>Nj_M*L!Y=I0KjGJ%&e(=>_oO7>4T zK0_3Ud*R7H#4UMndnj?nWQF-g9r>pj!ioobp>jEAsxj&Xw0}X{@u{$Y>xU+iJ3$fFL_Iv8x~V`$ znXP;mn|6FhEobD$5T;KfX+sW|vYcCIgONjUH|wr$8$Gc_)5ooLYt$*Ulp*(VJRTlw z?dJrj8YQa&mt4ve>a6EUD|oNEv$F5@MiP)s%nX|(x0FG1WCcgQmc3uQ49?>3A0j@P z7GUht7fKfVxit)|t!`xL8>(yz#1eK&WymlzYOGK0LjFP!{WzDutIRMofFI#r7mpF-P8Pbb|4U3S?@53K0U+015Vtt3X zAyZmzHZEfKckja`*iPJmL2jwq(MF$zfFGa5Ls|2@H;KRM5$+s_iCXvDaX^pv77V8+ z3Y)%t-z%&nQ~o;kop!=z{w<3{9eU}*c@I8|vtSkDSp3Sz7P}GWQ6`;cGlUNED$hhh zTstw;uk0ztgx`{09Y!adsdCX&d||LjQ~}RfCMYQCOMa-%h@06{TG%4BG-3aR$z*S{dW*O_!N8(-W~63|*w+K~eNM}w!Be+xK}ITJEo9eE-UiZ_h_YydEA*Se zN~@O^z;P`TOy2*uaYJ-HZl1|r%LLaX*htIW-rUjPX4q6jf$W-ZbiqN9`b2On!| z14%D*D-8xwaQcDyKb|D{HgAh!MxY|+#Kec0pZC?BxHCLi72OHuv?%4(vepRK_HAlK zywT2-AS+5o*Cvq7Zff(1LD1)%ZCTWeGiS)eqi9?AB~n!|mC502wFgK?Gg;tsbkq%F z_LAkR&4zWHAX*#A-C7G^@u1`q->}7nb^BR89V`upF#z^^Qa@4(!-?ZzO=jmd=!MdqBq0_i`1zM_V~*Lgc+H{UYNrTsfg2b#K!IK zW^kX$PDaV%QCOEj6cHv zSFQNn`8F~B`@7>}{>S+?4gTHzHaWau&wrW!H6#2L`yaUU?6gkOt{40qO1#grU2oO9X!K ze)qdv|Nq|g@3YofXPtHSyU*EsKkt6dqo#}kBnDt$U;t>)>cD_oMtFVg`qIIS+s@h6 z#N5Hk&XUW`)+RFwq}swm5cs6=KHbhF0|^bWz_uTj;Fb?cb$ArGLVRpoacvu#(4fH( z{YHB!boQiuarNj}xhrJ_g1Q#n+5-pb^K}MrY3!fvmAcXSI%`>K`K_=DJ1AEs)H+A4 zDQye?@bZ<1B(TIr4hu&Gq?$A8eC2Ag-wqK@N$ck>!60dqH7CW1PF39sB>qlbn6~!L ziCjT3P-L9|n~~fvaOxui=F(^5y7NP<#Dz&Emqxa>3Q^HFotZmG<(z>f;-J3FGm{C~ zjRrh*_KBSry~I^GR*S3oW-q=1EYN(RFD_;cp522(`JekRx2D7mH$NGuCwd%6#q-HY z-xqBs^W^&*7r8L>yOz(Fqva683FGu_i#uFvq3(toVR6w5?0!0*v0zgD(#4T!n@A3L z(#WE_TOT&!@t*ewD?;n(%>76eM#(iiNBY`_wDo6{txpx$FXcTSXZMnyoC}wSkdaGH zp@ENl><@6&l+iI1iDbPYNC1F1DgdDNk9E59XPq3)ot&<#}ZD6yu~L4??}mE9FU~M1<5ODTKfY= z<>&&9@)Sb$W;25{$s)-=00&>3wKW>eRQC4BNO$U#9Pw?xsXEpWad}Fu3h*9wyqfO080I zoc`odye6pt4lRa$^s3g=3$Ibh~ zfjC*STgHuvvpPtwhX=NG7G+tdZ|2hVUu5~#q5D$uZ~;!mfrY#8B6nz>@Zj?0ScV3a7&yu9$Y`zYgVCD1!jj8}DNQc23EzBE7I zi8gduc2PuOKi*!&n4jX zK4Y>7*^e>svKpxFfa|5)PnmjsJZA~Seh4SE&3kO)t&_wPDtz@G|Ks)@S_r=SGj!Yg z6`(;u^+`0_$rpXPnk^s1eh3Db7U={#j;imoBW4v%C7X4uPw;PgD7>sC0~Hwn*g*TY z9{MlVEneC=X+1ZwG5^&>(YLXlfk6-)uZ01mbZJCF>rl)`P`%jVKRmAHmsGpKdh{=> zlNCLr)_W`}t(w*~ZvCNHfjm5rU%P$AkeaND!+}Z#2`j8~+)${pi+jvgFKv$t%VANY zkMF7H%Ml@$I#P-$!UCRDFrC%zd`vDF#g?*mE}PTsj4S5w{PcmM9T?UWDquE@S27q$ zB*Jka9ciMg#8o3-l)_ps--WU-hrQTv&yQiYW90}^s_Qj?8Xjzt?sZaS&G!d+% z#S^a*Y0#$94Vo%U>oV6WsV?km_><5V{Gi?rDlRddDq0z=zwpi5JOI2Gr`h@5kPX6J zBe)ceOaUhrEq*uYl|;HJTmj4$;C>B$y?+Hi{(r&mPki

gWTHe;?63$8A(!Au4+%%Zl6cJ;1uYuI(b*ra)P#dm7L!t*@ zT3X(8UjZ@Ke55qm5FP{I*v|X&vLJy7EC?__ofu5XP>|QdO}3e%Md`OBb*?E4YHkdU zSuxv@IEW{$#0?Z5B7U>^nGXooE`l(_El>uRX zih?@r=G`lf_tF;BJ>XnW$M>S7S(s^TArS0GPI#f+w_gr%q;{A>_a zLYE|>4*83^epUHYcTE_;AX@NISRYqp$tLT*2m8np!GemPaaqoEk!c>}r&5rhR4$md z2YbSRwzi2;l({?rVZYrk9rt6QM7^)(^&{US$OdnMYqaM^I*X|at@N@JM1tk*$IpVx zB)u4jsMox)sy`+=n@nLW+fk?CjrS5j^Fll1I0uaJzK@2U=pEakSaTodBDUHVIPNUG z6_v%3Hhn#a&G5nn58H{Jfa7Jv%&xYGs=D~7Qj0#VS2&(QN3UfpE*{4$bjI*3x3el{ zU9_*e*H%<}96i^QAon~c#bdEh&C+=ptMpsxh2!MAh%f#TU*6)nmkx|?_XwE<1m)U^ zU+>4B%$z$jU6^y{==0~vnDwITSl;-@Lg)O(=bXhCM2%GU1H^x56<=sJOSt6;5gwn? zZ0%iPQhsZ`Bs%2=uP3N2hDYg2@*nL{9i7Zd%W<;E%cs6lwaksN7~MiqKZdN?OH*fCb33O$hU{ecBn`ov z2`N;8CjT~wsjr^AD;)wCLPruZHwRQ$ATkXUd+Qj+z)qG5~#^re$2m( zEDKI2sZmfU$LI7&B27k&)siXXXlc-SznsA9s2u5;P7g%dvX&z6?vdcU(o{`NxAysFc7!ZwP9VKsW zqJ~A$1d18v`A{Ao@Xx{(4PHs_>K3-9(!P`)yf=6RaL1Om{?R?5!@@=$|G_*8 zapu#!D(6a#!r|H2ZshCP7k&h> zfQ`?lZ1K`QKcWBlqG!3n(RJHnYM)qXeFna@+k3ab1O-EKd@hWF*8+CrBZ?!qXYb7lb3MtE^J@rCTQ0pQQa{}sr56wZ&Tr&84|8XeRebBWnw=fW{kUCz7q?6Ep&m== z&(>aHp6@zm<`Z!_V}%cEu^N<*Uhz+%UrfTBt*aIcJqM{@^GF0Fc;9rLA&DpK;q_Ps zq5oUg-TQNQH*zqyapb<;dt5hn3L-()zsUHYD172u=?Jw_P$CG!?Aa6w%l;`$kDeB! zI+j2!3V(@H;+m1t=|G^+J}O>;C6PD1ND1$Zw-y(YL8X@) zkH+7`zsLV6e*8!xm{>+#x?^iLGc-#!&vZph!^29yI6exe;1Ka@ophOPz{ot#Ldrdg z^|->XxConIbZt*^qR`62|CwbtY0yr2M4~k>>c$`)yHJ;8S*Heh=Q&Ou<6XK3pO)L% z+b2HAy6F+7LR(`baH6#J^M&E7jI?VIeDdKb9e9WU`d;Z8uiHI)kKibYs`n3*>wco% zq%H!Gx4Czn7>;Yu{xe{)ub=oMYrirTHXw@E%!7kAM`RD{6b1G85LgeR(mFq02B}VxAxDn4WPv35L3@M1YxmT?1za=sXq6^99<*Po=&i1ItYert?yl{K>2L<4S4A7Na ze+Sz&4fy9TCHb>SoFL}5=G?!|yf@p^wyyHaNx^#!_~WiLPA|G^t_5SCoiFAEcI==P;6STiEkwzNF)s=ll~QHundMiW{h6SrhuxK~;DK z_=1WTmpkZ+XZtZN@Ki54hGpoa1J@YGBa^r1{CJ45ZRsnS9vO}K@s7}r3|kPqyfEMU zw8^|KONQ_c@UTR`#g<)*O}ET;z3D+LnY)aMo8SUnGgcN|v6nBt&&$>=kAg}Q`M7q{ zU?|Y$WtiModPqcR`PipkPn>!a#u3!FK?dAW*_vix?*T>crAICwq|W0iI3pGh{E>vU zNKL?OZiAK96CI`BxF;F!XevVlYF;0+u3;g27bOh^8G{K$E><=&`y#!?R~z7)M;_Bj zn)^L~@qJ^2PiWn+kh)rdscG!B>T$9Eq+0Dq8NDafYGdVLV&&wb$5&($%5rs zPhq0O(;f%T=9B6*6dG)suj*!5*GVf0#R=<(l=Ud{Fyt0$SRF_i=HHbuClyOzXIy0vO_{>!e#z%AxDm8j6qit+qq6lsa z)7b;hcV_d3$A#DVYC|*}TOua4i8Sj` zB|I^}`B9I=;yX;xp?UT%B58ZjFj2qyuz^~rY~HKO?er`4o*4jhoV7*0I(JK7)hZX6 zgx7iP0TWfnBoJ%C%klMW;%wVWU4GI)1LHtVaRGHXpWx82<_Y{kWgb)LRD|wTtZ~vT ztj#9!P7LfPL%?&|LO*O^f4hSH5vKOM9h`_@(TosTw-}E>sX8Dpup-*kMKaUav2sO~ zJI1HoI^_J!LAJpr)}~aK##uTny2&a~fa^;V4>Nj_M*L!Y=I0KjGJ%&e(=>_oO7>4T zK0_3Ud*R7H#4UMndnj?nWQF-g9r>pj!ioobp>jEAsxj&Xw0}X{@u{$Y>xU+iJ3$fFL_Iv8x~V`$ znXP;mn|6FhEobD$5T;KfX+sW|vYcCIgONjUH|wr$8$Gc_)5ooLYt$*Ulp*(VJRTlw z?dJrj8YQa&mt4ve>a6EUD|oNEv$F5@MiP)s%nX|(x0FG1WCcgQmc3uQ49?>3A0j@P z7GUht7fKfVxit)|t!`xL8>(yz#1eK&WymlzYOGK0LjFP!{WzDutIRMofFI#r7mpF-P8Pbb|4U3S?@53K0U+015Vtt3X zAyZmzHZEfKckja`*iPJmL2jwq(MF$zfFGa5Ls|2@H;KRM5$+s_iCXvDaX^pv77V8+ z3Y)%t-z%&nQ~o;kop!=z{w<3{9eU}*c@I8|vtSkDSp3Sz7P}GWQ6`;cGlUNED$hhh zTstw;uk0ztgx`{09Y!adsdCX&d||LjQ~}RfCMYQCOMa-%h@06{TG%4BG-3aR$z*S{dW*O_!N8(-W~63|*w+K~eNM}w!Be+xK}ITJEo9eE-UiZ_h_YydEA*Se zN~@O^z;P`TOy2*uaYJ-HZl1|r%LLaX*htIW-rUjPX4q6jf$W-ZbiqN9`b2On!| z14%D*D-8xwaQcDyKb|D{HgAh!MxY|+#Kec0pZC?BxHCLi72OHuv?%4(vepRK_HAlK zywT2-AS+5o*Cvq7Zff(1LD1)%ZCTWeGiS)eqi9?AB~n!|mC502wFgK?Gg;tsbkq%F z_LAkR&4zWHAX*#A-C7G^@u1`q->}7nb^BR89V`upF#z^^Qa@4(!-?ZzO=jmd=!MdqBq0_i`1zM_V~*Lgc+H{UYNrTsfg2b#K!IK zW^kX$PDaV%QCOEj6cHv zSFQNn`8F~B`@7>}{>S+?4gTHzHaWau&wrW!H6#2L`yaUU? Date: Wed, 26 Jun 2024 19:24:07 -0400 Subject: [PATCH 2/4] Do not overwrite max_mark in Result.get_total_extra_marks (#7125) Fixes bug in assignment summary view --- Changelog.md | 4 ++ app/models/result.rb | 1 - .../course_summaries_controller_spec.rb | 53 +++++++++++++++++++ 3 files changed, 57 insertions(+), 1 deletion(-) diff --git a/Changelog.md b/Changelog.md index 776d37d438..0eb0fe0d28 100644 --- a/Changelog.md +++ b/Changelog.md @@ -6,6 +6,10 @@ - Added a backend to check MIME type and file extension of uploaded files (#7083) +### 🐛 Bug fixes + +- Fix bug in grade display for marks summary (#7125) + ## [v2.4.11] ### 🚨 Breaking changes diff --git a/app/models/result.rb b/app/models/result.rb index 9c5a2892bb..0988df5762 100644 --- a/app/models/result.rb +++ b/app/models/result.rb @@ -129,7 +129,6 @@ def self.get_total_extra_marks(result_ids, max_mark: nil, user_visibility: :ta_v max_mark_hash[assessment_id] ||= Assignment.find(assessment_id)&.max_mark(user_visibility) assignment_max_mark = max_mark_hash[assessment_id] end - max_mark = max_mark_hash[assessment_id] extra_marks_hash[id] += (extra_mark * assignment_max_mark / 100).round(2) end end diff --git a/spec/controllers/course_summaries_controller_spec.rb b/spec/controllers/course_summaries_controller_spec.rb index 4babbcb2b7..35448abb2b 100644 --- a/spec/controllers/course_summaries_controller_spec.rb +++ b/spec/controllers/course_summaries_controller_spec.rb @@ -170,6 +170,59 @@ end end end + + context 'when there are percentage extra_marks' do + before do + assignments = create_list(:assignment_with_criteria_and_results, 3) + create(:grouping_with_inviter_and_submission, assignment: assignments[0]) + create_list(:grade_entry_form_with_data, 2) + create(:grade_entry_form) + create(:marking_scheme, assessments: Assessment.all) + assignments.first.criteria.first.update!(max_mark: 3.0) + assignments.second.criteria.first.update!(max_mark: 2.0) + assignments.third.criteria.first.update!(max_mark: 5.0) + assignments.each do |assignment| + create(:extra_mark, result: assignment.groupings.first.current_result) + end + + get_as instructor, :populate, params: { course_id: course.id }, format: :json + @response_data = response.parsed_body.deep_symbolize_keys + @data = @response_data[:data] + end + + it 'returns the correct grades' do + expect(@data.length).to eq Student.count + Student.find_each do |student| + expected = { + id: student.id, + id_number: student.id_number, + user_name: student.user_name, + first_name: student.first_name, + last_name: student.last_name, + section_name: student.section_name, + email: student.email, + hidden: student.hidden, + assessment_marks: GradeEntryForm.all.map do |ges| + total_grade = ges.grade_entry_students.find_by(role: student).get_total_grade + out_of = ges.grade_entry_items.sum(:out_of) + percent = total_grade.nil? || out_of.nil? ? nil : (total_grade * 100 / out_of).round(2) + [ges.id.to_s.to_sym, { + mark: total_grade, + percentage: percent + }] + end.to_h + } + student.accepted_groupings.each do |g| + mark = g.current_result.get_total_mark + expected[:assessment_marks][g.assessment_id.to_s.to_sym] = { + mark: mark, + percentage: (mark * 100 / g.assignment.max_mark).round(2).to_s + } + end + expect(@data.map { |h| h.except(:weighted_marks) }).to include expected + end + end + end end end From 94560f009f9c2d1e7a9760b9efc1f3540a8abe67 Mon Sep 17 00:00:00 2001 From: Samuel Maldonado Date: Wed, 26 Jun 2024 21:49:02 -0400 Subject: [PATCH 3/4] bugfix: do not display peer review marks in course grades summary (#7126) --- Changelog.md | 1 + .../course_summaries_controller.rb | 6 +++- app/helpers/course_summaries_helper.rb | 5 +-- .../course_summaries_controller_spec.rb | 32 +++++++++++++++++++ 4 files changed, 41 insertions(+), 3 deletions(-) diff --git a/Changelog.md b/Changelog.md index 0eb0fe0d28..fd24cac017 100644 --- a/Changelog.md +++ b/Changelog.md @@ -9,6 +9,7 @@ ### 🐛 Bug fixes - Fix bug in grade display for marks summary (#7125) +- Remove peer reviews from grade summary table (#7126) ## [v2.4.11] diff --git a/app/controllers/course_summaries_controller.rb b/app/controllers/course_summaries_controller.rb index 399f2f664d..59af5fc0c7 100644 --- a/app/controllers/course_summaries_controller.rb +++ b/app/controllers/course_summaries_controller.rb @@ -9,7 +9,11 @@ def index; end def populate table_data = get_table_json_data(current_role) - assessments = current_role.student? ? current_role.visible_assessments : current_course.assessments + if current_role.student? + assessments = current_role.visible_assessments.where(parent_assessment_id: nil) + else + assessments = current_course.assessments.where(parent_assessment_id: nil) + end marking_schemes = current_role.student? ? MarkingScheme.none : current_course.marking_schemes average, median, individual, assessment_columns, marking_scheme_columns, graph_labels = [], [], [], [], [], [] diff --git a/app/helpers/course_summaries_helper.rb b/app/helpers/course_summaries_helper.rb index 19dabb5657..06a2b50922 100644 --- a/app/helpers/course_summaries_helper.rb +++ b/app/helpers/course_summaries_helper.rb @@ -30,12 +30,13 @@ def get_table_json_data(current_role) target[:section_name] = target.delete('sections.name') target end - assignment_grades = students.joins(accepted_groupings: :current_result) .where('results.released_to_students': released) .order(:'results.created_at') .pluck('roles.id', 'groupings.assessment_id', 'results.id') - result_marks = Result.get_total_marks(assignment_grades.map(&:third)) + results = Result.where(id: assignment_grades.map(&:third)).where.missing(:peer_reviews).ids + assignment_grades.select! { |row| results.include?(row[2]) } + result_marks = Result.get_total_marks(results) assignment_grades.each do |role_id, assessment_id, result_id| max_mark = @max_marks[assessment_id] mark = result_marks[result_id] diff --git a/spec/controllers/course_summaries_controller_spec.rb b/spec/controllers/course_summaries_controller_spec.rb index 35448abb2b..cb0369df7a 100644 --- a/spec/controllers/course_summaries_controller_spec.rb +++ b/spec/controllers/course_summaries_controller_spec.rb @@ -171,6 +171,22 @@ end end + context 'when there are peer reviews' do + before do + assignments = create_list(:assignment_with_criteria_and_results, 3) + @pr_assignment = create(:assignment_with_peer_review_and_groupings_results, + parent_assessment_id: assignments[0].id) + create(:complete_result, grouping: @pr_assignment.groupings.first) + get_as instructor, :populate, params: { course_id: course.id }, format: :json + @response_data = response.parsed_body.deep_symbolize_keys + @assessments = @response_data[:assessments] + end + + it 'does not return the peer review mark' do + expect(@assessments.pluck(:id)).not_to include @pr_assignment.id # rubocop:disable Rails/PluckId + end + end + context 'when there are percentage extra_marks' do before do assignments = create_list(:assignment_with_criteria_and_results, 3) @@ -321,6 +337,22 @@ end end + context 'when there are peer reviews' do + before do + assignments = create_list(:assignment_with_criteria_and_results, 3) + @pr_assignment = create(:assignment_with_peer_review_and_groupings_results, + parent_assessment_id: assignments[0].id) + create(:complete_result, grouping: @pr_assignment.groupings.first) + get_as student, :populate, params: { course_id: course.id }, format: :json + @response_data = response.parsed_body.deep_symbolize_keys + @assessments = @response_data[:assessments] + end + + it 'does not return the peer review mark' do + expect(@assessments.pluck(:id)).not_to include @pr_assignment.id # rubocop:disable Rails/PluckId + end + end + context 'when display_median_to_students not set for any assignment' do it_behaves_like 'check_graph_data' end From f2cb1b6673551c7f4c7c16202b577e1272450a42 Mon Sep 17 00:00:00 2001 From: Donny Wong Date: Tue, 2 Jul 2024 09:56:30 -0400 Subject: [PATCH 4/4] update MarkUs version --- app/MARKUS_VERSION | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/MARKUS_VERSION b/app/MARKUS_VERSION index ee22365dac..de9aab602b 100644 --- a/app/MARKUS_VERSION +++ b/app/MARKUS_VERSION @@ -1 +1 @@ -VERSION=v2.4.11,PATCH_LEVEL=DEV +VERSION=v2.4.12,PATCH_LEVEL=DEV