Update BNF data importer to handle ISP CSV headers - #3210
Conversation
We previously updated the importer to handle the new CSV column header format used by the ODP (2f6905e). However, the BNF releases from 2024-10-01 to 2025-02-01 are only available as CSVs using the old ISP column header format. The ISP- and ODP-provided CSVs contain the same BNF data, but use different column header formats. For example, the ISP format uses `BNF Chapter Code`, while the ODP format uses `BNF_CHAPTER_CODE`. We can transform the `csv_reader.fieldnames` before importing the BNF data so the dict keys have the same format as `concept_type_column_header` regardless of whether the old ISP and new ODP column header formats are used. This means the existing record-creation logic can work with either ISP- or ODP-format BNF data. Update the tests to: - add mock ISP-format BNF data and verify that it can be imported - keep the existing import and error-handling tests using the ODP-format fixture, as these test general importer behaviour rather than differences between the CSV header formats - use the correct YEAR_MONTH column name and clarify the fixture documentation for the ODP fixture
b0551ea to
cf70eac
Compare
lucyb
left a comment
lucyb
left a comment
There was a problem hiding this comment.
I think this is good and I particularly like the clear and comprehensive commit message. I'd be happy for you to merge it with no further changes, although I ask that you consider the minor comments below.
You specifically asked about the overall test structure and the fixtures. I think the extra duplication is fine and is generally to be expected in tests. However, looking at this with no prior context I'm wondering if it would be better to have actual CSVs with the data in rather than dynamically creating them in test fixtures. That's outside the scope of this change, but might be something we can improve in future if we need to revisit these tests.
This is my personal preference, but it's often a good idea to split the refactoring step (e.g renaming) and the functional changes into two separate commits. This makes it a bit easier for the reviewer, as well as easier for you if you need to revert anything. Again, this is something to consider for the future rather than anything to change right now.
|
|
||
|
|
||
| @pytest.fixture | ||
| def mock_odp_bnf_release_data_csv_path(tmp_path): |
There was a problem hiding this comment.
The other test fixture is called mock_isp_bnf_data_csv_path, but this one is mock_odp_bnf_release_data_csv_path. To keep things more consistent you could drop the word release and call it mock_odp_bnf_data_csv_path instead.
| mock_odp_bnf_import_data = [ | ||
| [ | ||
| "MONTH_YEAR", | ||
| "YEAR_MONTH", |
There was a problem hiding this comment.
Just to clarify, has this changed because it's correcting an earlier mistake or for another reason?
2 participants
We previously (2f6905e, as part of #3167) updated the BNF data importer to handle the new CSV column header format used by the ODP (e.g.
BNF_CHAPTER_CODE) . However, the BNF releases from 2024-10-01 to 2025-02-01 are only available as CSVs using the old ISP column header format (e.g.BNF Chapter Code).This update transforms the CSV column headers before processing the data, so that both formats are represented internally in the same way (e.g.
BNF_CHAPTER_CODE). This allows the importer to support both the old ISP and new ODP CSV header formats while keeping the rest of the import logic unchanged.