From 2197edf8099ba3302e146800ad24d8bd9498ca75 Mon Sep 17 00:00:00 2001 From: Zandre Engelbrecht Date: Wed, 3 Jan 2024 12:41:54 +0200 Subject: [PATCH 1/9] add util func for to validate case property names --- corehq/apps/data_dictionary/util.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/corehq/apps/data_dictionary/util.py b/corehq/apps/data_dictionary/util.py index 5b4d3dc19e25..371530c852bd 100644 --- a/corehq/apps/data_dictionary/util.py +++ b/corehq/apps/data_dictionary/util.py @@ -1,6 +1,7 @@ from collections import defaultdict from itertools import groupby from operator import attrgetter +import re from django.core.exceptions import ValidationError from django.utils.translation import gettext @@ -360,3 +361,9 @@ def is_case_type_deprecated(domain, case_type): return case_type_obj.is_deprecated except CaseType.DoesNotExist: return False + + +def is_case_property_name_valid(case_prop_name): + pattern = '^[a-zA-Z][a-zA-Z0-9-_]*$' + match_obj = re.match(pattern, case_prop_name) + return match_obj is not None From d65cfa63e0241da8f42354e3961b41782e746466 Mon Sep 17 00:00:00 2001 From: Zandre Engelbrecht Date: Wed, 3 Jan 2024 12:42:04 +0200 Subject: [PATCH 2/9] add unit test for util func --- .../apps/data_dictionary/tests/test_util.py | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/corehq/apps/data_dictionary/tests/test_util.py b/corehq/apps/data_dictionary/tests/test_util.py index 22bb22b27dcd..b263a712675b 100644 --- a/corehq/apps/data_dictionary/tests/test_util.py +++ b/corehq/apps/data_dictionary/tests/test_util.py @@ -14,7 +14,8 @@ get_column_headings, map_row_values_to_column_names, is_case_type_deprecated, - get_data_dict_deprecated_case_types + get_data_dict_deprecated_case_types, + is_case_property_name_valid, ) @@ -191,3 +192,20 @@ def test_get_data_dict_deprecated_case_types(self): deprecated_case_types = get_data_dict_deprecated_case_types(self.domain) self.assertEqual(len(deprecated_case_types), 1) self.assertEqual(deprecated_case_types, {self.deprecated_case_type_name}) + + def test_is_case_property_name_valid(self): + valid_names = [ + 'foobar', + 'f00bar32', + 'foo-bar_32', + ] + invalid_names = [ + 'foo bar', + '32foobar', + 'foob@r', + '_foobar', + ] + for valid_name in valid_names: + self.assertTrue(is_case_property_name_valid(valid_name)) + for invalid_name in invalid_names: + self.assertFalse(is_case_property_name_valid(invalid_name)) From dfc67b36ab31e3ff6eeabf5df0819a80d85578a4 Mon Sep 17 00:00:00 2001 From: Zandre Engelbrecht Date: Wed, 3 Jan 2024 12:42:20 +0200 Subject: [PATCH 3/9] append relevant error if validation fails on case property name --- corehq/apps/data_dictionary/bulk.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/corehq/apps/data_dictionary/bulk.py b/corehq/apps/data_dictionary/bulk.py index 44b57dcde970..f0790114025e 100644 --- a/corehq/apps/data_dictionary/bulk.py +++ b/corehq/apps/data_dictionary/bulk.py @@ -21,6 +21,7 @@ save_case_property, get_column_headings, map_row_values_to_column_names, + is_case_property_name_valid, ) FHIR_RESOURCE_TYPE_MAPPING_SHEET = "fhir_mapping" @@ -165,6 +166,11 @@ def _process_sheets(domain, workbook, allowed_value_info): row_vals['deprecated']) data_type_display = row_vals['data_type_display'] if data_validation_enabled else None + if not is_case_property_name_valid(name): + errors.append(_('Invalid case property "{}" in "{}" sheet. The case property name should start ' + 'with a letter, and only contain letters, ' + 'numbers, "-", and "_"').format(name, case_type)) + # Fall back to value from file if data_type_display is not found in the map. # This allows existing error path to report accurately the value that isn't found, # and also has a side-effect of allowing older files (pre change to export From ef1519e242c1df0061a1949faa512435eb259341 Mon Sep 17 00:00:00 2001 From: Zandre Engelbrecht Date: Wed, 3 Jan 2024 12:42:30 +0200 Subject: [PATCH 4/9] extend unit test for broken import --- .../tests/data/broken_data_dictionary.xlsx | Bin 8058 -> 8046 bytes .../tests/test_import_export.py | 5 +++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/corehq/apps/data_dictionary/tests/data/broken_data_dictionary.xlsx b/corehq/apps/data_dictionary/tests/data/broken_data_dictionary.xlsx index dcc5605e6ddd19d1779cd9c492e66d158e46be05..572d5d2b0ca480a7cd2c0fa44c8deae524631ae1 100644 GIT binary patch delta 5358 zcmZ8l2Q*w=*B+f2(Ps4Cqebt%_a2?#)s0RNZ4iWr5+h0wy+#Y81qo4t2%<+Xqj%Bk zFUkL{H+j!m_paxz`|NY>-TOTIoU<>acBSxi)X~r(0Bme*z!QB&<9B$BDCj>ytssgH z>TeW-w(}doDF_&0ybt_cU0RJS5|)G@!FyHFhoe%Q=b-?8V0lr;kZV_pLHdqbFn^4E zx##w%5Kn*AUR_p1;*fJeg@3wgrS@*?nz^`WC#@xDld3u~qBflpH)x`rbCJ!uIsNkZ z$+`8Wm>zu!Lm&B@CRWbbgdem^jHKLrpN&NmLwnORI1J`sfp0Xt%;`iInC+{RJay?X z8va@9)w4=|{S3Gn`9b;wsE`KbpNTz1E|@40wJh|0Xw&%4gsNYV6Pym$8~^@j#BW6% zLkxB7eYRmNsCgI(QfdvtP(@Fb(0>|1Y5)yR@td2)7-V>&ZG%C?eR+t+^dM{|MJG?I zNZF4j18ErngAl%e|5zrvnAT_7@?klPrQ3MUgR9GYVz7@N5`;VgW21<_ZQUou z?Ai>aYo-!2U(^s}Bc5{O>5BQ}blG(LNcVn?D6yFqECZcwVCW6|ENb#{UOa8ia{3oU zCPeR0FV(*@+4*%P+;O`q5hmf4B|H$T8Gb(+au%|6L3q%K#$`Ps2inP2f3KGYJ`RbiD3DFc0Set@GQNk%a z+k!&7DvpG*Al?n^(LvswA$LYHIzEx(5+}R!>M4oqGyQG5RK^wpdw(12C`mm(>ZxFu z!qE4SFN>!?XlBz+O9solZApyiviNJMf~61EyF2rx-VTUbnuqz|2aM9+R2_8-7YPf+ zo$t)$k&1E!M(w!^U0tF1a%>P)dn{H4#+bNavo(ra;?% z@Xxo4o-kJE5KyI)TVbX6a*OU0@zm` zB6#vJbSdU#a|_Euis>fq_MK#7m!6DkpT3k@K3~KI6V{lkjf%N)u`x(raz*(h)21FX zipNGaH0RwJe*;nuN1;vGl}+stjqwE+Ynf*w9@#}ohDMu#*MnmO#W z%&o%kkV@5Gwfe~(n)utkVeEwhrY|zCMJWnB-lZ%pS|i0BPv!?~-V}}FbgVjBbQFGd z?D+b)nQTT#8nJo{&{4<2*5RJV6G8_7%I^X0p8rfizp@P9uPkFY?KUk!5WHuE9Z>c7 zKrn14&vVl?4Dn!mgJhkOOOJ%`c|NmKbMM4n;(O!zNEUq1$Cv#1>E3txNDn#70=W++ zuhKb$jDms_deQHvPN&~4d!C0Z1x^vD#sGTlZ}>-Uo}D*upp3!?$oOek5(8Epx*-j+ zIq(;-6@}9rD)N=m`rZ0yO+WRj`B3ixuD&HK>>o=#1_60X*?O0+{RyAWzcL0X%kSW= zj^teUOBGTvBV11*>JB%b*L`{G2_G!b4&c3ieBy)ojCpAqCvVGV;4F3NC`>)dFyHsp zfx8H&Opp@&zFi&lM>2LpO31u@^nO@jk*ZUV{a18#+=^V`?;9IFK1%mYQufgA^@5!l8g-U7?9rVL~CrkLSbMm&wK^j4fnF3I z6!ak7k4{gw-62E(7@V-LVb$lye>-h+VZ9ls#u!fs&g{Pe>)w1mckk8hVKpGgljLa| z>*q#P?uN_QVX(cVh=D!#7(&XWsB{?aq8g}%S6`Rd%2Gv5$j011&59I1C!u5*bW%!T zm`;s=Iw%lD2}D!@O-pO*9IjXbjq?}_0^*B_mxi4v0>9Z>RTZQ|)Q|zmF*SIuX)CTC z)4jF*b4125$dL~hfG7?XH+AwcR?2*7?ec@F;V*0>3L%7Ko3JFtBJ6c`1S4hMM+3tu z+Abr4%&~R}oXvzaoFbWAwq{mUsULg!%|8DSknCqA{1DqARrZI&<7Gw@sSRZB&M08!=79sm9e)eo`sI*=;o06GY?A&~G9x ze;2A6f1;5=)&|Q?bT~(mdviXs-`iGH@*)Alx*xd_nBE?Oi8TS94UFFEvpVMv^`Ku}CxHjH3f*pwWws+cSB5<^nPR|sc{)h~Qca4w(_MY-YX=V0 z_eECS9)ZD?n$8Khqwnoejn8D?J+Qy=@p;T2lfUuRKpqBuxB}+QlpkFtG~XsI={C{x z=As(p@S?-r7kX2VOVxN39)J4;HIw}kI%!v;SAl9DSg2@%waWfb=LHUItm4=t&mo%V zhOOaNEn6cJcVa*Tl2GZ+B|A{tj8Msc!qD{%e!{UfiShmHJ#rrODHkReeJrhlUn8{J z-yZ&yyaV`59r7T7B8%sV>zeRLNAp&#KF`hR8=C2$Gh4;ZK!Dk&5I+k%kp zJq>FK?Ctl;x$+8Jotgz5Zj;kLgd9_wSgR=HqkRB}N9d4i38))cTyL7TDFG|Hid?)x5p-d z_cEvLtj(%X%oV>0Jpbe$ME!XY*?sCof3#JWL(X_YEKv2l=Uy=VF1eean)sv9gn)-IVa~s(+3ciR)Bh%PSq?<10*rGaG`F!* zhnPt71ZuPSuCx5GEes)UML0YCNX4X>=R}uYGQRPzMu`@zU$3TR{ulIFsa7guezxbSA`A#&Zuy zX-(Jd`(6_v<$z+buIzHhG9xFz$c0MNlCr@qlZLIPky}yESbRuS54T_gpTeVjh8by6 zihNb`l{~|B*||5=?C10|lqTg29s4C+yw&KJIUNUkVv-6#3@&k1xdR{& zj`{vnkJC2|ZZkL+#V8ONiqC>y08@4fkG_nsl02C7z?dJi7f~LG>bYLDQh&j^KWR*7 z_iajI2yF2AdF$bjT-*AgZj(Ic3}dT`7cUt$*MXzxNXSnn>$@TK85*iDW z-gkj{Ph0+6psetikg})%029`KXB&K$gbPM%ffX$*bGhO+ykB8CCXHs30l9-2y``S(z z$0&J%;?3g<88Z|X52az9NwV_ILwuxtYLiOs22RQ+oB4#T z`yL{=cpZkD7p!NG4-hWO_acwpE``=0XRuc&(jpl=FpnNq+B}NFPdKNt^Mt^8UPsv` z!`YY_#on|qpG^e1`#*=nuO_|0x3`@%-0_~T z|K17%G!&;_baoj|5@~sm-khg>C;b_T4_m5=o+tnS@-7{l-v_-UZ%-B+%40-F6yYN~%&?@?uuT}!hVp`6xJ7y{1P*aR zN?r9Fsvp&+seSD5C1ZMK^aZATk7cQh6N;;k&M1T21uBp(Z4%sqh*Pq6LDReILO9XsK3^Dvoh01ZabE^M~J{nI<0As~ z&jMq?E-r&uogFqRd@=FD{xIkKE4nXCUqC3dqdxdpDb_=F0h9PalZ!W#h)s5N> zMo~YW3bpvPY9pomQWHY6OmPPuHf zq$q!Jcan<5=NrDNW2yQX7_m_jez%{~7V~FZP~c`%IKPA8NG13iGr=lS#T{{7EBGQpN9Vj D)^>=V delta 5351 zcmZXY1yoeq*T84!&LKq_q(O2(X;48zP|^XU9CAQ9y)+VvFr>87A>BxWlyna=q$r@I zbbO%itxwh>4-cfP$fH|E!Uo2=5-P;N z4={d9VVK|lkz^@3em{*NWkaUo-~uSbk~o>~rXHTxj7*VhS===XLG4>VsJA8}q)^z$ zcOqhygIIN9s+W9xL$lcXv`g{o<%}t8ptnUaF{8r?WPZNW`sy77$9UM3 ze2ivHU~=%$wC=cR7J$qLSbHx-PdKCrj`KD9`PVrdN=s_ z?z5-wkKb*1FPG16`6!*w6aeB4PfJGnxJQ+qlhcN$u;t+O>sZ|tR{{z*8lOW=ocisCsJoH1Gxg`q*rXB`E1z4J;6$|_;y3ib*yuJAPRkM>_ z%^lAKrr<*u)z1LC#YJ=r4a}_g7DhV< zYnVA$=EFbr%n)o_sNqP6Dwp#`Hc|=amJYNXkm-|lSniU2b|Xd<=bl$l-9*}V61lj~ zLg3a1M39CF43|Rw=73{h_a{` z2e~f?L`+pcAUgXc!||D_No(U7)5g|X6DoDb5Qur339+qt{8!I4#WjwV{pOJykC`6K zv??upm8xE>?f!Ad_VQ^yX^6k%9xuem?lz7b z@>IGWwVo!Y7@5p?_8u2EieZ4s=Apq{i8v|KbM?T}@*tYFNVGxXw%?e2)cz3Lntk)i z;i{+RT+)F>H&EbiL0wY2H_-32v)h4m=S(9q_&IR2NR@Cduj$nhx)(6;!9b<=XMMR* zl3q(6aq8GnHagYhs0q9N<-RdZOcUOF@?^!ryB4cde$RDA?V#kf@hgNew+Q@`zGKh3 zX|C;O9Qmz|rkxfKWfWbfuKplIq5}D>q(vK5vItFqodCuHLC<}8tR83iVhcJeFNZYM z)+oEygHTfA4!Vzk>cQMix))WI!)4H&j{#d^LP7NR39#%PZvEtCsZxT>o$@PAk4k?; z6BeKl{ec?Qqem7)pnhfRHx`y}Xr8ltef!pNJ8_JjV=!LrQS4L%`IMwiye^1z^fqo) zow0WQK&GtWF8xwyyC-@;+rMmm=83aQmypYgf!S9Lt@9OU!8cDxAKb^qyTJeW^%@ok zluZNz{he(t(+D6e;9+m~K@WvMLCJj4C1iWX>x;G+DCMQ18p5n~vs8qC(=e+gc_!2_ zrW)yUMv@<4`PeGJY(c^anj*PMv_hvTE3?{$63LeAPGb70fHoHf_pZ2o0lZ<)6A#nG zm9$yq%9AeIDleX$qxL{+D)2};Qp|hOyCUY{DCQv*pv&p4_XDA@8{By*w6g{F8HAm9 zt_e=$9i~tMr?za9JGDBSVPNZCN;lFT0r*I)JcP1?*(Y^dVRAU_Tf?H3GReKFuF(Jw zlQhxox2?*JDK`j2vvLyTjO80FJSk^WwtRHAOO(Sy?zs-yXD>?Wr!|IJYK)D(KlCdj zd}Py(4M+&SDwR#IKQxI;a57Ci#(+emP-N1jQCaj&^!xD6)_De%Smi|p9*O1&4Ccm~ zOY;e9mbrWZ!Vxs)>kAC$3mX}aZ8hPJ`m9Z<%=ILzkpPzlQ>}%Sn2jFYMMyhF2Yd`8 z-TGu6S8>=Ta1ljBE>k)t%0fI{l=LLbbdUWLU<1c`YS6rVjZ7V@SG%0D%dEvG!{&sM zWzfg^Cxn)gAlbY%sPwk4U|ADj7BJj$U<}@IRlq8M1e@AkBs4rnRC&p~rkRp7DtLHe zZ4XK->ggi3k7FP%bdcYG>t}9B2TVxw4$r=vIoT=5y>O*!t;l&I?&xha`Se>F(L}vA zfT43Zkl*w4JWGP1v;7WhQ8)bQ&pRDfzMWEIC#sfQRL`Gya}J?ctWiQPDtyo$7`X06 zNPjK?2_to@YU3!*&PcwB7JLwzttS3u0kzKF&d@t%vv}2T6QB5ivN}D$Nfg>>MzcOukc(;`sKFuUWWiLum|93 zRrG=+Ng+29aOEZ8J}fqUH_IG}Z*RN|)zK`GnA(AfO!>)wo)vZ$@pq-YGt3^|Rx2C6 zwH0KpK^9kIhTbOMYnnfdu*n=( zW#q?)%<6KMZ>Z|TYk;D5M3t+u4V<#0^67WON6-f!^(50VvRCqgquy0@S<;n>O02sH z&x!+tUoOB`(eKY%7(0Xs0vY~K^ne=uU3gQnX!OJj_GzL>_fnsDzWHFpG_ zasj*j$D?Bs6_xm+C*&^vjZ-6AU;F#%Vnlj!S7SVJS*fUee%yUGbbvRAmE$9E&@7$9 zRO0&&M$}zlMCBoeePC7t1}iKYRufBaFA)^2Mmg(TF=vZo42b7C@#^>asl3`( zOQvl}fBo_d#4_lM*t2Ni=2=<-~PvGTT30W+iiv-M#YYR$)C8La7H!`E0Dqvk_A{8dHc4dDx-#?hFjel=w- z_HsY83B8(V@W1A7lxSUIgll-h_c&_H>#>Cz@;5n8b!sNyVQDO?hq<%oyuQNYH>Ga9 z;bQ!RD1r`D+&a7WPMNiH7`e~8I)y!bEVT-d2Pt02k*x!-0Y(e^*(7!BWQqZ(Pf?4P zevpcz5P0GRI^;!Ols*ahGp1*s~m_6p!6QRCyk5*-|W@QS1lNHCG5N$!kl{Sd)?uckbW+D4Tw; zoVuGvR8bEV1`bN(1P0Wkj>0+u2RgNX0mH)xJvn%w%F-;p`>KG4wLfCk*hgW6rd7=B zK5g)e4H!~r(E1HA4aVe1GjrX$`NU~sx>xUWTuP-3s1ZXM&&4@pg!!hR9OWodFQcL*!DCa()=Fcr70Y3m+kh?%i{nqugtH z>nN;l^)d#2pOZ&*;}dvVQb(KDFVs3Ma0Zt9sGy>7qOt36!EtUlLg~py4*e8ED%`^* za$uHA#yz#w_LjfkIp$Ty@ZcyB#Jc2#PQw540@4b?1)zk8LN-sRf(DIdsg!RLsd!mU z8r5fpy1DC`nVB6I+{6Cf%rtH{W&Z%wURo3?D62HOzCKAiFQ)^|{j_K8Dg)+7kj_1O z>gCmVa_EVnrN*eJQJFM%FK7LGXw{2=B+hBI2%96}!A0tDu0j~s^nk$R3!A?B^z26p zf`rjsLV!t1y2&SUN%eWB`}Iizf;X(fOz1x|n80A_sH!cuvmeV55SK5gM?q`;Ip2v1 zS{23gbkGwOh)L(%3-f1DgOHGJ2eBGqrMb7Px0|Tw->JY>-s*mppG&*WbHp-x&7g?2 zeakA`{0Y8WM*BIFl(Z57wv!a!=?Cl;3Oq!7KLX&}D5>rj^wXPJJoFV6L|FAn5OePUF9jMLsw7;he?yyVpMxh0c#2tAjD&OI&P@3jp zT?+)k}bd93a_Y5k!xC-+EQh}{t+fA|DDo0b2;B|aewi(BJY=1d<=b%dqp)2p<6Wy@ecxx zP$E28jAW3aYAaW46O-#y*%N(MV%-C#Ozt}KhNj7ypWDhy1=FRojXRCGo!1PxrglRb zQDx>EXLqVLyBNG^rwA?36g}zn)rXsY@Dpcy;MdLyCl9cYzyN^`aQ#2KVTkK)@Z1iXwcmK5sG5e$AgX(L^c^x(0gObt*g`?ADJb}_yY&J=(6rEh4o^^$8 z^>E#z+l`9;Wopz#MI2dQv2Z?>Cv)q?PSU5j7Rx;bb2{D`dGKDlvPxGgk{GSfskRIm ziC#C<>#u+0P-=FHvFcopU(p?J(6{7b2Zz{Z z7ByH-yp{zeB&Z?W5ZiR!j#YN-(#x8L!sawT4sF4mXFZ;<*7|`0k^Kv+b|FrdFlXVc zcQ@5zB8H^`bILUTb-HI@ z5NP>wHU2#l;QTH@Ad_iXfM_GlHX%sJ<{8!fC4So)F!e|rAN^7=P9W0buBtbV*ke>t zTuXC}9M*ejy;a^pZ<#4ObX5~;p%wmdC|p>2i|?v_>8 z=9IU?^*#AOP1e>!3VTJzhmjv<6>j*@_i^gdqug0LO3_bHqK0iqAWrsrqMx%u7wtYc z5<|?9hFOX-w_(d9uY3oZZZd&OCr4}LD3{X0qZ&wu*bT0LIB+gbAEmEVIF$cO|6Bw!yfF(iQ_&VwjE!Ly^ z!xdh=Q6sOqg(?|N&1@@KW_L7AoA(xJ8xNQGA?jOI6 z(vAHp*dPn(ZvdCxd%mj2=PPClrZn%zAvI=WxyzAC^?mAIr^= zUTsKqF%tjW&0`Sgs!+)Aa*K>3Bk_v@ztf&*^L@c#7vb(Nk1+-CKlHBT?ajf+7xnRD zq>7019&lI2jfEB)ktt5c2>3;YwiC8h?qv9^vEjb4kN_SG>8v18>QICWCIiQ$x=Cy= zxmXTGedgVCF~Of4F#VaOg$cR(Za)lRm2gIZA4=C8sX1d z2Cq}y=K;pRzNXZz85AK8iGW+x3O#6$L|Lx*%kq+0XkTTcFnXigz{H88&v0e< Ee-Vp`9{>OV diff --git a/corehq/apps/data_dictionary/tests/test_import_export.py b/corehq/apps/data_dictionary/tests/test_import_export.py index ae9aa9753483..861d22945121 100644 --- a/corehq/apps/data_dictionary/tests/test_import_export.py +++ b/corehq/apps/data_dictionary/tests/test_import_export.py @@ -82,8 +82,9 @@ def test_broken_import(self): 'Case property \"mistake\" referenced in \"caseType1-vl\" sheet that does not exist in ' '\"caseType1\" sheet. Row(s) affected: 5, 6', "Error in \"caseType1\" sheet, row 5: " - "{'data_type': [\"Value 'Nonsense' is not a valid choice.\"]}" - + "{'data_type': [\"Value 'Nonsense' is not a valid choice.\"]}", + "Invalid case property \"32foobar\" in \"caseType1\" sheet. The case property name should start " + "with a letter, and only contain letters, numbers, \"-\", and \"_\"", } message_str = str(messages[0]) soup = BeautifulSoup(message_str, features="lxml") From 8a890e2c84054ab44869bdd42e12a473bdda0918 Mon Sep 17 00:00:00 2001 From: Zandre Engelbrecht Date: Wed, 3 Jan 2024 13:21:29 +0200 Subject: [PATCH 5/9] rename util func --- corehq/apps/data_dictionary/tests/test_util.py | 8 ++++---- corehq/apps/data_dictionary/util.py | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/corehq/apps/data_dictionary/tests/test_util.py b/corehq/apps/data_dictionary/tests/test_util.py index b263a712675b..aff407993a2f 100644 --- a/corehq/apps/data_dictionary/tests/test_util.py +++ b/corehq/apps/data_dictionary/tests/test_util.py @@ -15,7 +15,7 @@ map_row_values_to_column_names, is_case_type_deprecated, get_data_dict_deprecated_case_types, - is_case_property_name_valid, + is_case_type_or_prop_name_valid, ) @@ -193,7 +193,7 @@ def test_get_data_dict_deprecated_case_types(self): self.assertEqual(len(deprecated_case_types), 1) self.assertEqual(deprecated_case_types, {self.deprecated_case_type_name}) - def test_is_case_property_name_valid(self): + def test_is_case_type_or_prop_name_valid(self): valid_names = [ 'foobar', 'f00bar32', @@ -206,6 +206,6 @@ def test_is_case_property_name_valid(self): '_foobar', ] for valid_name in valid_names: - self.assertTrue(is_case_property_name_valid(valid_name)) + self.assertTrue(is_case_type_or_prop_name_valid(valid_name)) for invalid_name in invalid_names: - self.assertFalse(is_case_property_name_valid(invalid_name)) + self.assertFalse(is_case_type_or_prop_name_valid(invalid_name)) diff --git a/corehq/apps/data_dictionary/util.py b/corehq/apps/data_dictionary/util.py index 371530c852bd..52e9715b06c4 100644 --- a/corehq/apps/data_dictionary/util.py +++ b/corehq/apps/data_dictionary/util.py @@ -363,7 +363,7 @@ def is_case_type_deprecated(domain, case_type): return False -def is_case_property_name_valid(case_prop_name): +def is_case_type_or_prop_name_valid(case_prop_name): pattern = '^[a-zA-Z][a-zA-Z0-9-_]*$' match_obj = re.match(pattern, case_prop_name) return match_obj is not None From afda814df54f64d20b71628662028b13c8de71f0 Mon Sep 17 00:00:00 2001 From: Zandre Engelbrecht Date: Wed, 3 Jan 2024 13:21:57 +0200 Subject: [PATCH 6/9] move where validation occurs + refactor text --- corehq/apps/data_dictionary/bulk.py | 6 ------ corehq/apps/data_dictionary/util.py | 3 +++ 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/corehq/apps/data_dictionary/bulk.py b/corehq/apps/data_dictionary/bulk.py index f0790114025e..44b57dcde970 100644 --- a/corehq/apps/data_dictionary/bulk.py +++ b/corehq/apps/data_dictionary/bulk.py @@ -21,7 +21,6 @@ save_case_property, get_column_headings, map_row_values_to_column_names, - is_case_property_name_valid, ) FHIR_RESOURCE_TYPE_MAPPING_SHEET = "fhir_mapping" @@ -166,11 +165,6 @@ def _process_sheets(domain, workbook, allowed_value_info): row_vals['deprecated']) data_type_display = row_vals['data_type_display'] if data_validation_enabled else None - if not is_case_property_name_valid(name): - errors.append(_('Invalid case property "{}" in "{}" sheet. The case property name should start ' - 'with a letter, and only contain letters, ' - 'numbers, "-", and "_"').format(name, case_type)) - # Fall back to value from file if data_type_display is not found in the map. # This allows existing error path to report accurately the value that isn't found, # and also has a side-effect of allowing older files (pre change to export diff --git a/corehq/apps/data_dictionary/util.py b/corehq/apps/data_dictionary/util.py index 52e9715b06c4..6d1bc23ab66a 100644 --- a/corehq/apps/data_dictionary/util.py +++ b/corehq/apps/data_dictionary/util.py @@ -207,6 +207,9 @@ def save_case_property(name, case_type, domain=None, data_type=None, """ if not name: return gettext('Case property must have a name') + if not is_case_type_or_prop_name_valid(name): + return gettext('Invalid case property name. It should start with a letter, and only contain letters, ' + 'numbers, "-", and "_"') try: prop = CaseProperty.get_or_create( From 0e6a823edadf2c666ee70cf4f362794fa5ab3201 Mon Sep 17 00:00:00 2001 From: Zandre Engelbrecht Date: Wed, 3 Jan 2024 13:22:14 +0200 Subject: [PATCH 7/9] validate case type name --- corehq/apps/data_dictionary/util.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/corehq/apps/data_dictionary/util.py b/corehq/apps/data_dictionary/util.py index 6d1bc23ab66a..02e81667cda6 100644 --- a/corehq/apps/data_dictionary/util.py +++ b/corehq/apps/data_dictionary/util.py @@ -210,6 +210,9 @@ def save_case_property(name, case_type, domain=None, data_type=None, if not is_case_type_or_prop_name_valid(name): return gettext('Invalid case property name. It should start with a letter, and only contain letters, ' 'numbers, "-", and "_"') + if not is_case_type_or_prop_name_valid(case_type): + return gettext('Invalid case type name. It should start with a letter, and only contain letters, ' + 'numbers, "-", and "_"') try: prop = CaseProperty.get_or_create( From 8aac252e59f8bc974a0250e9eefdf1465e58e888 Mon Sep 17 00:00:00 2001 From: Zandre Engelbrecht Date: Wed, 3 Jan 2024 13:22:26 +0200 Subject: [PATCH 8/9] update unit test error text --- corehq/apps/data_dictionary/tests/test_import_export.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/corehq/apps/data_dictionary/tests/test_import_export.py b/corehq/apps/data_dictionary/tests/test_import_export.py index 861d22945121..d56a6bc87b77 100644 --- a/corehq/apps/data_dictionary/tests/test_import_export.py +++ b/corehq/apps/data_dictionary/tests/test_import_export.py @@ -83,8 +83,8 @@ def test_broken_import(self): '\"caseType1\" sheet. Row(s) affected: 5, 6', "Error in \"caseType1\" sheet, row 5: " "{'data_type': [\"Value 'Nonsense' is not a valid choice.\"]}", - "Invalid case property \"32foobar\" in \"caseType1\" sheet. The case property name should start " - "with a letter, and only contain letters, numbers, \"-\", and \"_\"", + 'Error in "caseType1" sheet, row 6: Invalid case property name. It should start with a ' + 'letter, and only contain letters, numbers, "-", and "_"', } message_str = str(messages[0]) soup = BeautifulSoup(message_str, features="lxml") From 25f160925f811738194bd8f1bfb8731163eb2b23 Mon Sep 17 00:00:00 2001 From: Zandre Engelbrecht Date: Wed, 3 Jan 2024 13:28:11 +0200 Subject: [PATCH 9/9] change where case type name validation is done --- corehq/apps/data_dictionary/bulk.py | 8 ++++++++ corehq/apps/data_dictionary/util.py | 3 --- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/corehq/apps/data_dictionary/bulk.py b/corehq/apps/data_dictionary/bulk.py index 44b57dcde970..dae1d466ecc6 100644 --- a/corehq/apps/data_dictionary/bulk.py +++ b/corehq/apps/data_dictionary/bulk.py @@ -21,6 +21,7 @@ save_case_property, get_column_headings, map_row_values_to_column_names, + is_case_type_or_prop_name_valid, ) FHIR_RESOURCE_TYPE_MAPPING_SHEET = "fhir_mapping" @@ -137,7 +138,14 @@ def _process_sheets(domain, workbook, allowed_value_info): domain, worksheet) errors.extend(_errors) continue + case_type = worksheet.title + if not is_case_type_or_prop_name_valid(case_type): + errors.append( + _('Invalid sheet name "{}". It should start with a letter, and only contain ' + 'letters, numbers, "-", and "_"').format(case_type) + ) + continue column_headings = [] for (i, row) in enumerate(itertools.islice(worksheet.iter_rows(), 0, None), start=1): diff --git a/corehq/apps/data_dictionary/util.py b/corehq/apps/data_dictionary/util.py index 02e81667cda6..6d1bc23ab66a 100644 --- a/corehq/apps/data_dictionary/util.py +++ b/corehq/apps/data_dictionary/util.py @@ -210,9 +210,6 @@ def save_case_property(name, case_type, domain=None, data_type=None, if not is_case_type_or_prop_name_valid(name): return gettext('Invalid case property name. It should start with a letter, and only contain letters, ' 'numbers, "-", and "_"') - if not is_case_type_or_prop_name_valid(case_type): - return gettext('Invalid case type name. It should start with a letter, and only contain letters, ' - 'numbers, "-", and "_"') try: prop = CaseProperty.get_or_create(