Organization Validation for Service Inventory - #1668
Open
JVickery-TBS wants to merge 46 commits into
Open
Conversation
- Started working on reference tables for pd data.
- Continued script to generate service reference data. - Continued schema for ref tables. - Improved pd subcommand for loading ref data.
- Finalized script to generate service ref data. - Added more recombinant schema key/values.
- Made JSONB index. - Added choices_filter_query. - Added recombinant org_name to the temp ds table. - Started writing the database validation.
wardi
reviewed
Apr 10, 2026
- Finalized ref value checks and errors in the db func. - Fixed up some other logic things.
- Started working on fiscal year function.
- Finalized fiscal year db function.
wardi
reviewed
Apr 17, 2026
wardi
reviewed
Apr 17, 2026
wardi
reviewed
Apr 18, 2026
wardi
reviewed
Apr 18, 2026
Member
|
just those minor things otherwise LGTM |
- Added change log file.
- Pyright fixes. - Flake8 fixes.
- Made service name fields computed ones. - Modified filter scripts for service name lookups.
- Filter script typo.
- Filter script typo.
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
- Created new workflow to semi-automate stuffs.
- Removed service name computed fields.
- Trigger type cast.
Contributor
Author
|
TODO: update any tests for test_service.py and its make tests if needed. |
- Add db table and logic for reference data file hashes during load.
# Conflicts: # ckanext/canada/helpers.py # test-core.ini ### RESOLVED.
fixes for loading service.csv
…o feature/ds-reference-tables
- Added test coverage for service inventory fiscal year function. - Added test coverage for service inventory org based fields.
- Flake8 fixes.
- Org validation for make service test.
- Org validation for make service test.
- Org validation for make service test.
- Trying to fix `Prep Test Coverage Split File`
- Trying to fix `Prep Test Coverage Split File`
# Conflicts: # ckanext/canada/tests/filters.py # ckanext/canada/tests/test_service.py ### RESOLVED.
- Rebuild service inventory ref data. - Add fixme comment for overwritting dict assignment.
- Support multiple org names for one umd.
- Do not count new lines in max char func. - Added test coverage.
- Added functionality for the new recombinant CLI markers. - Added choice suffix to service inventory program_id. - Added test coverage. - Added support for old choice values in the recombinant webforms. - If `service_migration` marker is set, invalid program_ids will be suffixed with -INV.
- Renamed the datastore_user temp table to be more accurate as an app_context table.
- Better handle legacy choices. - Better handle choice suffixing. - Handle insecure firefox preventing window.confirm
- Added change log files.
- Pyright fixes. - Flake8 fixes.
- Pyright fixes. - Flake8 fixes.
wardi
reviewed
Jul 31, 2026
| suffix_fr = '' | ||
| if id.endswith('-INV'): | ||
| suffix_en = ' (Invalid for Organization EN)' | ||
| suffix_fr = ' (Invalid for Organization FR)' |
Member
There was a problem hiding this comment.
Do we need a FIXME to remember to get translated strings here?
Comment on lines
+124
to
+125
| row['program_name_en'].append('"%s%s"' % (PROGRAM_IDS[id]['en'], suffix_en)) | ||
| row['program_name_fr'].append('"%s%s"' % (PROGRAM_IDS[id]['fr'], suffix_fr)) |
Member
There was a problem hiding this comment.
Maybe a little cursed, but you are allowed to use the same types of quotes inside f-strings that you use outside.
Suggested change
| row['program_name_en'].append('"%s%s"' % (PROGRAM_IDS[id]['en'], suffix_en)) | |
| row['program_name_fr'].append('"%s%s"' % (PROGRAM_IDS[id]['fr'], suffix_fr)) | |
| row['program_name_en'].append(f'"{PROGRAM_IDS[id]['en']}{suffix_en}"') | |
| row['program_name_fr'].append(f'"{PROGRAM_IDS[id]['fr']}{suffix_fr}"') |
|
|
||
| REQUEST_HEADERS = {'User-Agent': 'CKAN/open-gov/service/gen_ref_data'} | ||
|
|
||
| WHITE_SPACE_SUBS = re.compile(r'\xa0|\t|\n|\s+') |
Member
There was a problem hiding this comment.
I think you mean
Suggested change
| WHITE_SPACE_SUBS = re.compile(r'\xa0|\t|\n|\s+') | |
| WHITE_SPACE_SUBS = re.compile(r'[\xa0\s]+') |
\t and \n are covered by \s and you want to allow any number of repeats of all of them, not only \s
| org_id_abbr_map = {} | ||
| with requests.get(org_variants_uri, stream=True) as response: | ||
| response.encoding = 'utf-8-sig' | ||
| c = csv.DictReader(response.iter_lines(decode_unicode=True)) |
Member
There was a problem hiding this comment.
if we've manually set the encoding do we need detect_unicode=True? If so that's a strange API
| contextual_flags else 'FALSE', | ||
| org_name=literal_string(org_name) if org_name else None, | ||
| flag_definitions=(',' if contextual_flags else '') + | ||
| ', '.join('%s boolean' % m for m in contextual_flags), |
Member
There was a problem hiding this comment.
we'd want an identifier() here wrapping column names, but as we discussed it would be even better to use a text[] field for flags
| BEGIN | ||
| IF value IS NOT NULL AND value <> '' AND LENGTH(value) > max_chars THEN | ||
| IF value IS NOT NULL AND value <> '' AND LENGTH(REGEXP_REPLACE( | ||
| value, E'[\r\n]', '', 'g')) > max_chars THEN |
Member
There was a problem hiding this comment.
as discussed we want to include all characters when computing length
| table_name = Column(Unicode, primary_key=True) | ||
| last_sync = Column(DateTime, nullable=False, | ||
| default=datetime.datetime.now(datetime.timezone.utc)) | ||
| file_hash = Column(Unicode, nullable=False) |
Member
There was a problem hiding this comment.
if we're only ever storing sha265 hexdigest values here it would be better to call the column sha256 or similar
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We are able to consistently use the releases from the SI github to generate the CSV ref data we need. We have commands which load it into the tables which we define in an sql script in here. The part we were tripped on was relation between Service IDs and Program IDs, but there is no relation that is being defined or maintained currently so don't have to worry about that.
Just extended the temp datastore table to add an optional org_name to it, which we then use in the triggers. The Service ID one works fine, need to make the error message for it still.
The Program IDs one is gunna be interesting as Program IDs is a multiple select.