From 0a273a7fc7c41b6b0e1bcd844b09d776c1b0489b Mon Sep 17 00:00:00 2001 From: Dalai Felinto Date: Thu, 27 Jun 2024 17:01:59 +0200 Subject: [PATCH 1/7] WIP: Improve validation messages 1. Prevent duplicate XML error to show up if the __MACOSX error is already present. 2. Add a general "Use the command-line tools" message on the template if any error is present. 3. Use to highlight some elements of the error messages. This is marginally related to: #175. --- TODO: I could not find a way to also have for missing_wheel and forbidden_filepaths. My idea is that we could sanitize their parameters first, and then combine with the mark_safe() error message. Feel free to take the patch over and finalize it. --- extensions/templates/extensions/submit.html | 7 +++++++ files/forms.py | 10 +++++----- files/utils.py | 6 ++++-- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/extensions/templates/extensions/submit.html b/extensions/templates/extensions/submit.html index d77153f0..712c3eb4 100644 --- a/extensions/templates/extensions/submit.html +++ b/extensions/templates/extensions/submit.html @@ -59,6 +59,13 @@ {% include "common/components/field.html" with field=form.source label='File' classes="js-agree-with-terms-trigger js-submit-form-file-input" %} + + {% if form.errors %} +
+ To catch and prevent most of these errors, it is recommended to build your extension using the command-line tool. +
+ {% endif %} +
{% include "common/components/field.html" with field=form.agreed_with_terms classes="js-agree-with-terms-trigger js-agree-with-terms-checkbox" %} diff --git a/files/forms.py b/files/forms.py index 355b0944..f8e2e254 100644 --- a/files/forms.py +++ b/files/forms.py @@ -34,13 +34,13 @@ class FileForm(forms.ModelForm): 'The manifest file should be at the top level of the archive, or one level deep.' ), # TODO: surface TOML parsing errors? - 'invalid_manifest_toml': _('Could not parse the manifest file.'), - 'invalid_missing_init': _('An add-on should have an __init__.py file.'), - 'missing_or_multiple_theme_xml': _('A theme should have exactly one XML file.'), + 'invalid_manifest_toml': _('Manifest file contains invalid code.'), + 'invalid_missing_init': mark_safe(_('Add-on file missing: __init__.py.')), + 'missing_or_multiple_theme_xml': mark_safe(_('Themes can only contain one XML file.')), 'invalid_zip_archive': msg_only_zip_files, 'missing_manifest_toml': _('The manifest file is missing.'), - 'missing_wheel': _('A declared wheel is missing in the zip file, expected path: %(path)s'), - 'forbidden_filepaths': _('Archive contains forbidden files or directories: %(paths)s'), + 'missing_wheel': _('Python wheel missing: %(path)s'), # TODO %(path)s + 'forbidden_filepaths': _('Archive contains forbidden files or directories: %(paths)s'), #TODO %(paths)s } class Meta: diff --git a/files/utils.py b/files/utils.py index 4a03d911..e9ff8b8c 100644 --- a/files/utils.py +++ b/files/utils.py @@ -173,7 +173,10 @@ def validate_file_list(toml_content, manifest_filepath, file_list): """Check the files in in the archive against manifest.""" error_codes = [] type_slug = toml_content['type'] - if type_slug == 'theme': + found_forbidden_filepaths = find_forbidden_filepaths(file_list) + + # Special treatment for Mac, so the same problem (__MACOSX folders) doesn't lead to two errors showing. + if type_slug == 'theme' and '__MACOSX/' not in found_forbidden_filepaths: theme_xmls = filter_paths_by_ext(file_list, '.xml') if len(list(theme_xmls)) != 1: error_codes.append('missing_or_multiple_theme_xml') @@ -192,7 +195,6 @@ def validate_file_list(toml_content, manifest_filepath, file_list): error_codes.append( {'code': 'missing_wheel', 'params': {'path': expected_wheel_path}} ) - found_forbidden_filepaths = find_forbidden_filepaths(file_list) if found_forbidden_filepaths: error_codes.append( { -- 2.30.2 From dce6f2d66db3106cf893c1733fb078419a15e3f5 Mon Sep 17 00:00:00 2001 From: Dalai Felinto Date: Thu, 27 Jun 2024 17:13:33 +0200 Subject: [PATCH 2/7] Using alert-info for the command-line part --- extensions/templates/extensions/submit.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/templates/extensions/submit.html b/extensions/templates/extensions/submit.html index 712c3eb4..d7a79a2d 100644 --- a/extensions/templates/extensions/submit.html +++ b/extensions/templates/extensions/submit.html @@ -61,7 +61,7 @@
{% if form.errors %} -
+
To catch and prevent most of these errors, it is recommended to build your extension using the command-line tool.
{% endif %} -- 2.30.2 From 19b9d7f9674816680bcf2aa88b5f9d5c22991f33 Mon Sep 17 00:00:00 2001 From: Dalai Felinto Date: Thu, 27 Jun 2024 17:19:58 +0200 Subject: [PATCH 3/7] Use blockquote instead --- extensions/templates/extensions/submit.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/extensions/templates/extensions/submit.html b/extensions/templates/extensions/submit.html index d7a79a2d..847ff45c 100644 --- a/extensions/templates/extensions/submit.html +++ b/extensions/templates/extensions/submit.html @@ -61,9 +61,9 @@
{% if form.errors %} -
+
To catch and prevent most of these errors, it is recommended to build your extension using the command-line tool. -
+ {% endif %}
-- 2.30.2 From 55c0d45e1cf68a7d939c01738bf2ba7763db4fca Mon Sep 17 00:00:00 2001 From: Dalai Felinto Date: Thu, 27 Jun 2024 17:32:08 +0200 Subject: [PATCH 4/7] Update error message based on review --- extensions/templates/extensions/submit.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/templates/extensions/submit.html b/extensions/templates/extensions/submit.html index 847ff45c..e896a7bd 100644 --- a/extensions/templates/extensions/submit.html +++ b/extensions/templates/extensions/submit.html @@ -62,7 +62,7 @@ {% if form.errors %}
- To catch and prevent most of these errors, it is recommended to build your extension using the command-line tool. + Build your extension using the command-line tool to catch and prevent most of these errors.
{% endif %} -- 2.30.2 From 3a811a500211a707963ff05a7ba94b3206830d82 Mon Sep 17 00:00:00 2001 From: Dalai Felinto Date: Mon, 1 Jul 2024 14:40:13 +0200 Subject: [PATCH 5/7] From review: Unittests Keep in mind that the test is deliberately failing since the code still cannot handle in some of the error messages. --- extensions/tests/test_submit.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/extensions/tests/test_submit.py b/extensions/tests/test_submit.py index dd4d16b3..a2ea2174 100644 --- a/extensions/tests/test_submit.py +++ b/extensions/tests/test_submit.py @@ -109,19 +109,19 @@ EXPECTED_VALIDATION_ERRORS = { ], }, 'invalid-addon-no-init.zip': { - 'source': ['An add-on should have an __init__.py file.'], + 'source': ['Add-on file missing: __init__.py.'], }, 'invalid-addon-dir-no-init.zip': { - 'source': ['An add-on should have an __init__.py file.'], + 'source': ['Add-on file missing: __init__.py.'], }, 'invalid-no-manifest.zip': { 'source': ['The manifest file is missing.'], }, - 'invalid-manifest-toml.zip': {'source': ['Could not parse the manifest file.']}, - 'invalid-theme-multiple-xmls.zip': {'source': ['A theme should have exactly one XML file.']}, + 'invalid-manifest-toml.zip': {'source': ['Manifest file contains invalid code.']}, + 'invalid-theme-multiple-xmls.zip': {'source': ['Themes can only contain one XML file.']}, 'invalid-missing-wheels.zip': { 'source': [ - 'A declared wheel is missing in the zip file, expected path: addon/wheels/test-wheel-whatever.whl' + 'Python wheel missing: addon/wheels/test-wheel-whatever.whl' ] }, } -- 2.30.2 From bd4f4307581cbcdf8191d5bb6d892b9d09f277c6 Mon Sep 17 00:00:00 2001 From: Dalai Felinto Date: Mon, 1 Jul 2024 14:54:29 +0200 Subject: [PATCH 6/7] From review: new unittest for OSX --- .../files/invalid-theme-multiple-xmls-macosx.zip | Bin 0 -> 6548 bytes extensions/tests/test_submit.py | 3 ++- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 extensions/tests/files/invalid-theme-multiple-xmls-macosx.zip diff --git a/extensions/tests/files/invalid-theme-multiple-xmls-macosx.zip b/extensions/tests/files/invalid-theme-multiple-xmls-macosx.zip new file mode 100644 index 0000000000000000000000000000000000000000..2e8ccee1bed6b9a302f299c3d3912317e804324b GIT binary patch literal 6548 zcmbW6byO8^x5p2CIHX8>=xz`Yl#q}-ba#VvcXxwGgEWWk?jwzYAf3`FAs{Jv;dih1 z^{ut;efO-Hd7kymeE0s&p84l93J?TDT)<;6xUOpaHTa(cDu5iIuP-YsDzB`;rmBJg zKp80rG@E_cAI~oCXaG3G9RvX2uY-1Hiotq zW~NTgtj=~e)_)<92*PD6&zV)eL1U8XetBj3vc%<1QQypa4Mu3NucR z_&wNpwDhmd=0B|9{=Hdeb5k2rR`-7}_%9yczc=VWfgN-iMHCJnOF<#5Kr1xQUDt$i<`U5(BYfhP(n&Zdzk` zrRq>e;NN%1MvboWHedYkIkbK6g?@k{O8vEhD%q=~(aab`gTv3vu?0a|Wsr~#0#op& z@f)iIUX>TXkuVyTh{n$OsLJZT2ZxU?|K=Y6pm>=7cUX1+0FQ6_ z-?!>CGLYR`}CnGK;!{qL`15x$?4 z5=rTca0z6_-F<+6Enkg#y4n-KHQ?PG+T1bBu(6wek-aLxbJ^Ivel|5A1gBnX7V&GCu;j+{;fXoxjU3g))Z^-FfO@~3_C_AeDDo|IS#K2FY z1n#a4#fQAUPNN;WcmLzHUh!QHi8LPbWd-q1yVKiRL~@V}Fh2vI&?5`=S5QNmS=RKX z&)Goka21o>yIbbj@t)_5x7gZ1%WR_+sDtu*ZN@9hVZ%S$8(k|c2WD$h zpzE-3Y!;^ryycC3`n8{af!GNLC=BM)Vwfj|4BLcSLW3$nR|A_>ZQAp{+Uc&1F}yZ^ zLD%nxRM%1(ZLftxd@rkao^(>gs$ONp6t05B4KoUI`&ZEm6Y|XHeUT$4#fd%#STJCA z1Bz&U3ZXKD9KxnM{!;VF%yV|lg^gh`&%4yg{I0)Wu=&+s(_35f`OKODxpiYOv>Jf@ zaTB?>CNUL^@$yX1pVSgBH2Nk5I?|6&NAs}Ii;_nrf+V@)Vuxvo04>v@m#!gU}ArpRXmQPcT~J1&~&V%-G654+_M%f;n(5>~EMBo!9OwMoA zueO*u;1gb%-GzY7nq$1@VC0z`f?=3KaQ&_&VYU&{TqGW?)!JGookrKh8jkk0ouUqF z3DD!%GXnA%%^eWZ*EhL$FgQem*J2rCuOnaUXQ+_U*&EkWxRwv9WhR%)Wk-MMNgiT_ zIiTLnXb%FQ+J}wf3M(-LTFV+b=r3Zcu_Sn8g^i+urI_-=dlwD7M6Y0Zw&?i}UyljW z5%N+!-gZ%=I<@^6xAnPo@@Pczn}C;NT+~t+C=TXljm1(y!&;o@+Svq=V3Iv32a-uK zAc|8};|=_F7^;<4W%5#a=!(UY_F-(Dq%d#cGM9`}s@E@vsNBB61FTgbcUV3kU)}ln zi5C9rD6M3|Db*R(Vc*gKsw6ja*A zI;pgJR#n5aIH(c?U07Fye#UYYubEL()>MrsYSPoCuSiSVL$!$`;Kw9wfZOn%!j_G1 zWL4*S5LOkjry0nv+QTa4;zb8QYt}-#2URH?Q7WUOPcD;~lFp{D%;!bZgb2F74XJ{r zg&;!0w#}s;q>}=MsIOlLFi6SMbcM~4f=ZDX{YFtk`AroN-;-%{TFts;)aLGiBQIrE z#Ai=xzaUmQe$AE152ir=^0cf2*FnTmYtKBoa4l6xA~js`r&p z%&8~^!!;?vIhSw{BE5vx9`!BHT#kd(UZ!qd^7_!?vRDjvnJ&MWu;v0aq#NsRNVFiD z=oLa@MD3`#&io0q73R-=-Nq|nWfJmmif;*5D5P50(dAC}=9Dp-i3*|gaBou+k`7Gw zbs#PS$AWRu!3HBs!GRg{$`m!+@Sq4UcN6+}d8QQKjoIhL4zy_yE=Adz3Q1M{d}GLA(qml zR%syI1NqClid-k8wT4?75{emd+=_#s%~knvV823#5c05?yUQysf)26gmNa_j!UQv zLE|!k%OA2G$dnB}TEW-geA9Xs0-q(%(yXPfAjhQw9(Yeii;Xd7h!nQ1{91nWlYHM0 zXb{zm*j@SHS1wWOMn*W=C!WAq_yb1-YC0Nv}(I zD{CB`jWJ(2yAF@|YV94wW-EwPrW6KTqV#Ay3lWobFJkVJp>iQM<{g2@cO7vvtZ>ma z9NoRpy4Wa7HyjJ(Tf~)eev&dwkxANKLo}tky|$D3Wb`&E;I5lOO=Y{MbEX#ag7~l| z?+EV9ak7`WJsq>eg6fN6nv)d9*hb!QcH?xahGjd=DBA75c8}af1P(*tt)`AbpDKZ1 zYvE`B+3aUr2bDprn_l$}^lz?{0puoMXr7A>isZ3Wqv2#yqGhQgOYZq2Zd%j75-bFm zo4p}S_1>})LBAoCD9v&aST#D(goRNGWw!`wF?=@8v{KAiEXF};`2b^! zTmaijs5`0jhz_7I4|Puo5)~Jq4DBnj-tc={ok)oCa>StX3u;c5ulM*cj>o}7Ld(bx z8k|}LH$x@w5&Dt(N4#&hY&*2wtWG#-P5D;MWOfI6eoi}|yMdf`S-dP{3%%bSZ9wq} zjid<}0(%p8qSb#&KgtIbQU-dR`(NY$IM!#%C@ zrx$K=wsAbq8*0mA8&x%?_Cb)Kp(7M*^1<8eeLFkDk|D4FtDGGkhQY9|8Ef#`*gxQh z5zTRPJBK;>UEdZZ8L;}3gQg_IQJ&IK(x?-fgz!YauhDlHGBHOef-uxFgM4ztn2A9# zPmsGaozOwA$A~?JPC27}rjD(Ys}FzWB8?dj;Lb@6MLKazsokWCkUM5d#T{Gy`nrvG$*ro?*(lGU~IY%*b>vkk@9TGWo zX7DXoVMDiW!JVEm-u~dMZOmpgZfD>_9XVl4$K+B3^q8e8rK`z$=IidN!Ilv3vFw3e z4@!8+RQ?J``cs4r-G+;>xX1W=SBFu0|#Ai7qS9HTS-ZSi)B8 zyGeMwIf>ZQ0(mi_W_zpyj22s~FPvkeqH_3j)IMO0AV;{bHt)#-^JL6TQqW7i_Ii%V z?Z(j@;t(UYieGo8Y;T6^YDby+FygF7!auMQNt9Wjo+RsDS!{U!6wt^~o1s2-p>ARE38i_ilj6pBh$%$7JPF;%m@U zrO#`!nB&!`QPj1$DlQ1C?X9s~OfrYOXyqk;RYQwR!XqVvb|Q3!qPy*wqbWv0B2gXt zDZK;7C0x9qAm9_VH@7N6wg|3Yr^s{wXmD?5K-^0h_f3|kA*rEA3=lrulgw^oc%=ML zFEUDEt3t!*-D0-(zHXgcpfo~#Bi?=et8jJgK6`yDEFpn>$$;T77ydHI)ij8L{)OX&ijGgF0IAwZo*Kc?<@UpWwD*PcD*1uXNmV&ta62m56j!^ky-v;l zQ#7@>%ITOsMl@LEn61rtf0ME|EnSbdbG7WJEw04;0Mh00Fe-7dB%eMLr{U9Yt#oFr zJ*n)o6*O6{N8k6k=-9YBywjDXV48yDKUNQFX{<;T({89`IkJaaJcPF17JS;Aivosl z_|!>d+Xye>PtG*vz>=X~ErOpuy&_5wY<f0{9;$u(jwpvUmaE%^ z1>(4=EV#7RytE-7t~N~>&x2p_?d?nbPtBE!VKqcEhx5&H7-`9Y<4vT;C7bBVcTyB) z;oT%7TkiC4$m6LsRf?TNL)se8bYTq{_i&hJo}IsfOesT(V4gG4N6#{QRRz>Ac<|uH z&MRp8?cM0LV7Yq9sH5L@Q1hvZ*9Mv_z63WU?BWDnl{img6CrXENdp{m&P>Q}W14g6 zWGEG8>5B!xZxZ9+G>jzPX{2_2Ob{~yTTncF_@RO_if=~v^1MsGJwpVh@|fF{6kt_# zY1m~QX}_3`YRHUfut|55jZ%-7_REiJaI`x?M=5gLZKRB?_PtYHnlQ;k-MUtsFx6!U zt^Yu)GSvI51mq+pIavNC;xm2%&5t=BE!G$zn;xfMJ|l4+Sx80*K{OwzaVelPz9(A6 zsGvyR=@)oN&IWF_(NE($8U~a_p4OiRWLvs0PSTORbx#IBZ6HR(spEXuE~gGo2Y?O>c6t)1>5Ao_NZ(gZBZvi@?sAf0$iY&5HCdGnq>jiIOlcIwHjwK%b|aM!_HQ@ z?*i@?H^s{Q6<-Q`{OCQw8+8WxVbX0Sz9L$*yrOP_JIkWwa1?hNKI6254+q9RF`Eg~ z@)_GTJL4&%zRi_ zD0^X2wx*+chbORLt2UQ-Uy!ssZPLhLDIjXYSwBcn*2jKFnZ+7KKm!StmS*)BIO z_{&7ObuzN4irI@qK6TCDGrJ7?l?{Yi#!eN!oi><2>Go`c6$G|kzOmJ0?K|Yfqx;eP zg~f$blOmtq`oTBSn-G!d%JT*qhM_0|;rXc!@VfM5U>lANXCQaS_N=<-7+OME0LikF z0?x#z&ryeXh{knsi*K_Dpw?b%&*Q=9Lk@z-RRnaLJB~QMxq%|Mm20vagyZWY5fy3? z3$-nhNtfOQi7XY@hePw2Ekx0R6RI6{JG4s`;l56w7exlbvyF@gxnA>Xn`o2mQadwC}1bWLKOXQjJBGCRsb`qc2tsH-GX8TIE<~A5Rqptj7w(eO9^7V&3GXm8gqyz40!CBd@ zgly})Cjki#siyrLp|7(YvQ1zqErjoCDUbILpWBlqCeBL?>jyeu1@fZlMhEfA*?%|U zr~EX-X5#_ZZ-{R~;lL4lGx{#qsnq?wZ^AHyofIC6X5=Ml2tZ^TET6#JJBG$d8Z1RD zysaXvnP|e=zg`<|csx!e$IULx+HaiNpEdzDLjsLLnBEfQeQ|m&2L$=wG7y|&vb&z^ z^@EZ1R_YlIiSs@UH#cCN!$%KH!oACM;)nDUle6}70J2?1v_B3pQ_a0=yI;(o80S0n>`;F)lIMAN`Z{X2BjJv@ z^g{0Jr2hbumOydPUTpkV4H(P}0sbL{qOd{N1HsBWC{z>57`dm$R^Vga-28-IJb zxb_sG(=)lrR7$<&W?@R=#5NJh-R%|> z{}00i+zLOA{vWXZzh!^p`ajwBBd-4)&;W@)W&cBX|J%gxY=1K8M_&FrpaEQPf0$s^ z|9d3+`}e@Vpyz*Pcx2muQr}0m{X6Odb{{hQN2L4D6p#4$&n`XU U1PS@^R-}h*^`W**g2&l^0j|A`Z2$lO literal 0 HcmV?d00001 diff --git a/extensions/tests/test_submit.py b/extensions/tests/test_submit.py index a2ea2174..687726bd 100644 --- a/extensions/tests/test_submit.py +++ b/extensions/tests/test_submit.py @@ -119,9 +119,10 @@ EXPECTED_VALIDATION_ERRORS = { }, 'invalid-manifest-toml.zip': {'source': ['Manifest file contains invalid code.']}, 'invalid-theme-multiple-xmls.zip': {'source': ['Themes can only contain one XML file.']}, + 'invalid-theme-multiple-xmls-macosx.zip': {'source': ['Archive contains forbidden files or directories: __MACOSX/']}, 'invalid-missing-wheels.zip': { 'source': [ - 'Python wheel missing: addon/wheels/test-wheel-whatever.whl' + 'Python wheel missing: addon/wheels/test-wheel-whatever.whl' ] }, } -- 2.30.2 From 4fe07c2204ae9f5f4f8ccf41d397e53798ba5739 Mon Sep 17 00:00:00 2001 From: Dalai Felinto Date: Mon, 1 Jul 2024 15:18:18 +0200 Subject: [PATCH 7/7] From review: Move logic around --- files/utils.py | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/files/utils.py b/files/utils.py index e9ff8b8c..1224b865 100644 --- a/files/utils.py +++ b/files/utils.py @@ -172,13 +172,21 @@ def find_forbidden_filepaths(file_list): def validate_file_list(toml_content, manifest_filepath, file_list): """Check the files in in the archive against manifest.""" error_codes = [] - type_slug = toml_content['type'] - found_forbidden_filepaths = find_forbidden_filepaths(file_list) - # Special treatment for Mac, so the same problem (__MACOSX folders) doesn't lead to two errors showing. - if type_slug == 'theme' and '__MACOSX/' not in found_forbidden_filepaths: + found_forbidden_filepaths = find_forbidden_filepaths(file_list) + if found_forbidden_filepaths: + error_codes.append( + { + 'code': 'forbidden_filepaths', + 'params': {'paths': ', '.join(found_forbidden_filepaths)}, + } + ) + type_slug = toml_content['type'] + if type_slug == 'theme': theme_xmls = filter_paths_by_ext(file_list, '.xml') - if len(list(theme_xmls)) != 1: + # Special treatment for Mac, so the same problem (__MACOSX folders) + # doesn't lead to two errors showing. + if len(list(theme_xmls)) != 1 and '__MACOSX/' not in found_forbidden_filepaths: error_codes.append('missing_or_multiple_theme_xml') elif type_slug == 'add-on': # __init__.py is expected to be next to the manifest @@ -195,13 +203,6 @@ def validate_file_list(toml_content, manifest_filepath, file_list): error_codes.append( {'code': 'missing_wheel', 'params': {'path': expected_wheel_path}} ) - if found_forbidden_filepaths: - error_codes.append( - { - 'code': 'forbidden_filepaths', - 'params': {'paths': ', '.join(found_forbidden_filepaths)}, - } - ) return error_codes -- 2.30.2