Small refactor and Compatibility with DHIS2 2.41#55
Conversation
…ig singleton and version control for the tables
…string to number to allow more operations
… added the strict sql mode renaming the sql files
|
@nshandra I have changed the way SQL failures are identified so that .sql files are renamed to make them strict, calling them x_strict.sql instead of searching for errors in the log. This also requires updating the Docker setup: EyeSeeTea/d2-docker#140. |
|
when difficult to replicate the scenario, please reviewers, do only a code review, to identify any code quality or architecture issue you could identify |
tokland
left a comment
tokland
left a comment
There was a problem hiding this comment.
A minor in-line comments. I was about to comment that long SQL can be extracted to files (so we can format it properly) but it's some work for small value, let's keep as is.
| for org_unit in orgunits: | ||
| path_query = " (path like '%{}%' and uid <> '{}') or ".format( | ||
| org_unit, org_unit) | ||
| path_query = path_query[:-3] |
There was a problem hiding this comment.
That [:-3] is very cryptic, I guess it's removing the trailing "or ". For that, you can use str.join, like this:
path_query = " or ".join([
f"(path like '%{orgunit}%' and uid <> '{orgunit}')"
for orgunit in orgunits
])(I use also f-strings, python +3.6)
There was a problem hiding this comment.
Hi Arnau, thanks. I wouldn’t change it now because it’s copy-pasted from an older file and I’m not entirely sure what it does — it’s old code. And there are already quite a few changes in this PR.
nshandra
left a comment
nshandra
left a comment
There was a problem hiding this comment.
Looks good. Minor styling and leftover issues.
| pre_api_version=args.pre_api | ||
| if args.post_api is not None: | ||
| post_api_version=args.post_api |
There was a problem hiding this comment.
Minor nitpick, should be:
pre_api_version = args.pre_api
There was a problem hiding this comment.
Good call on the style issues, but I spoke with Arnau about defining and applying a standard style to all the project files once there are no more pull requests.
| if sql_data_elements != "": | ||
| has_rule = True | ||
| if sql_datasets != "": | ||
| sql_query = sql_query + """ |
There was a problem hiding this comment.
Styling: Trailing whitespace after """"
There was a problem hiding this comment.
Just a note in case this comes up later—
maybe it’s not the case here, but the spaces inside the SQL triple quotes (""") are intentional to avoid accidentally merging words in the queries.
| DELETE FROM datavalue where dataelementid in (select dataelementid from datasetelement | ||
| where datasetid in (select datasetid from dataset where uid in {datasets})); | ||
| """.format(datasets=datasets)) | ||
| pass |
There was a problem hiding this comment.
I have fixed this one
| def delete_all_event_programs_from_lists(programs, f): | ||
| programs = convert_to_sql_format(programs) | ||
| delete_all_event_programs(programs, f) | ||
|
|
||
| def delete_all_event_programs(programs, f): |
There was a problem hiding this comment.
Styling: 2 blank lines between defs
| from src.preprocess.sql_generation.versioned_table_names import * | ||
|
|
||
|
|
||
|
|
||
|
|
||
| def generate_delete_tracker_rules(trackers, data_elements, org_units, org_unit_descendants, |
There was a problem hiding this comment.
Styling: should be 2 blank lines
| if anonimize_org_units: | ||
| write(f, """ | ||
| update {programinstance} as rand set organisationunitid=(select organisationunitid | ||
| from organisationunit where length(path)=(select length(path) from organisationunit where rand.organisationunitid= organisationunitid) ORDER BY RANDOM() limit 1) where {programinstanceid} | ||
| in ( select psi.{programinstanceid} from programstageinstance psi | ||
| inner join programstage ps on ps.programstageid=psi.programstageid | ||
| inner join program p on p.programid=ps.programid where p.uid in {program}); | ||
| """.format(program=sql_event_program, programstageinstance=get_event_table_name(),programinstanceid=get_enrollment_identifier_name(), programinstance=get_enrollment_table_name())) |
There was a problem hiding this comment.
programstageinstance arg is unused.
There was a problem hiding this comment.
I have fixed this one
| ")".replace("(or", " ") | ||
|
|
||
| def fix_final_query(sql_query): | ||
| """This method is required now to create the sql with the ; and remove possible unnecessary and;""" | ||
| sql_query = sql_query + ";" | ||
| sql_query = remove_spaces_before_semicolon(sql_query) | ||
| sql_query = sql_query.replace("and;", ";") | ||
| return sql_query | ||
|
|
||
| def remove_spaces_before_semicolon(s): | ||
| return re.sub(r'(\S)\s+;', r'\1;', s) No newline at end of file |
There was a problem hiding this comment.
Styling: 2 blank lines between defs
|
|
||
|
|
||
| def write(f, text): | ||
| # print(text) |
There was a problem hiding this comment.
Originally it wasn’t commented out, but Jordi asked me to comment it out back some years ago. It can actually be removed, although it’s a good spot to check the logs if debugging is needed.
There was a problem hiding this comment.
I have fixed this one
|
|
||
| from enum import Enum | ||
|
|
||
| class TableKey(Enum): | ||
| USER = "userinfo" | ||
| EVENT = "event" | ||
| ENROLLMENT = "enrollment" | ||
| TRACKER = "tracker" | ||
| EVENT_COMMENT = "event_comment" | ||
| ENROLLMENT_COMMENT = "enrollment_comment" No newline at end of file |
| } | ||
|
|
||
| def _get_tables_for_version(): |
There was a problem hiding this comment.
Styling: 2 blank lines between defs
4 participants
https://app.clickup.com/t/8697kr4wf
This branch fixes all the sqls in 2.41 and make it multiversion.
Functions have been split into dedicated files, compatibility with DHIS2 2.41 has been added, and the handling of versioned tables has been improved.
Also some little related bugs deleting organizational units and programs and small removing user from usergroups errors have been fixed to make it work in 2.41, and also password hiding in logs has been ensured.