Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EES-5738 Add filtering by geographic level for Data Catalogue page #5468

Merged
merged 7 commits into from
Dec 30, 2024

Conversation

mmyoungman
Copy link
Collaborator

@mmyoungman mmyoungman commented Dec 13, 2024

This PR allows uses of the public Data Catalogue page to filter data sets by geographic level.

🚨 There is an Admin migration endpoint to hit (PUT /bau/migrate-datasetfile-geographiclevels?isDryRun=false) as this is deployed across environments. 🚨

The original approach for this feature was to use EF8 JSON column/query support to make the Files.DataSetFileMeta column queryable. While this approach actually worked, we couldn't test it due to not having any support with the InMemory DB we use in backend testing. For this reason we switched approach to duplicating DataSetFileMeta's geographic levels into a table to make them queryable for the Data Catalogue page.

It's worth additionally mentioning that while the EF JSON column approach did work, it required a lot of finessing to make it work. That, along with no support from the InMemory DB, made me think that persisting with that approach would only lead to more future problems and that it was better to go with a simpler approach.

The new table is named DataSetFileGeographicLevels. This is because we intend to pull data and meta files out of the Files table and into new DataSetFile/DataSetFileVersion tables. This work is going to be done after this work because the priority was to provide this feature to end users first.

We also have work planned (also to happen in the near future) to remove Files.DataSetFileMeta and make all aspects of the meta queryable by bringing the meta data out of JSON and into tables. This PR takes one step in that direction.

@mmyoungman mmyoungman force-pushed the EES-5738 branch 6 times, most recently from 1645f6d to 63e714e Compare December 16, 2024 10:41
Comment on lines 112 to 116
.WithDataSetFileMeta(_fixture.DefaultDataSetFileMeta()
.WithGeographicLevels(
[GeographicLevel.Country, GeographicLevel.LocalAuthority, GeographicLevel.Institution]))
.WithDataSetFileGeographicLevels(
[GeographicLevel.Country, GeographicLevel.LocalAuthority, GeographicLevel.Institution])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is an example of where it can get a bit confusing keeping geographic levels in DataSetFileMeta when it's now also in the collection of DataSetFileGeographicLevel. Presumably only one set of levels needs to be set up here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It uses DataSetFileVersionGeographicLevel to filter in the db query, but then uses the DataSetFileMeta to populate the response.

This will get sorted as part of EES-5666 which I'll be picking up after the filter hierarchies work

@mmyoungman mmyoungman force-pushed the EES-5738 branch 2 times, most recently from 144be3f to e1f68a0 Compare December 19, 2024 10:05
@benoutram benoutram self-requested a review December 19, 2024 10:13
Copy link
Collaborator

@amyb-hiveit amyb-hiveit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The FE code looks fine, but it's missing:

  • the button to remove the filter, it should be above the list of data sets like when you filter by theme etc
  • UI test (probably quick to add to data_catalogue.robot)

Also a couple of queries:

  • should we show levels in the dropdown that there aren't data sets for?
  • there's a discrepancy in labelling for National / Country, can it be made consistent?
    Screenshot 2024-12-19 125950

@mmyoungman
Copy link
Collaborator Author

there's a discrepancy in labelling for National / Country, can it be made consistent?

Discussed with Amy and Nick. We decided to add filterLabel on LocationLevelDetails to handle this instance.

@mmyoungman
Copy link
Collaborator Author

should we show levels in the dropdown that there aren't data sets for?

Discussed this with Rich and we've create another ticket to do this work, EES-5746

@mmyoungman
Copy link
Collaborator Author

the button to remove the filter, it should be above the list of data sets like when you filter by theme etc

👍

@mmyoungman
Copy link
Collaborator Author

UI test (probably quick to add to data_catalogue.robot)

👍

... Local Authority District

user checks page contains button Local Authority District
user checks testid element contains total-results 1 data set
Copy link
Collaborator

@benoutram benoutram Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering how resilient this is going to be when run in parallel with other test suites where other files may be uploaded containing the same geographic level data? I think there's no filtering by theme or publication applied by this stage so it's operating on all published data data set files. Isn't there a possibility of finding more than one result, especially when run against the Dev environment?

Wouldn't it be better to be filtered by the suite's publication by this stage and check the count of files filtered by geographic level against what we expect, out of the four data set files which were uploaded at the beginning?

We could also check the results are those expected by title or at least have the expected level value displayed in 'Geographic level' in the 'Show more details' section.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's no filtering by theme or publication applied by this stage

There is still filtering for the pupils and schools theme and pupil absence publication at this point. The previous test case (Remove release filter) just removes the release filter, but leaves the theme and publication filter. So I think this is fine, as the publication is seed data and we'd not expect anyone to add data sets to it.

I will check the the Geographic level of the data set that is returned.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is still filtering for the pupils and schools theme and pupil absence publication at this point. The previous test case (Remove release filter) just removes the release filter, but leaves the theme and publication filter.

Cool, I missed that 🙂 . Change looks good.

@benoutram benoutram self-requested a review December 20, 2024 16:47
@mmyoungman mmyoungman dismissed amyb-hiveit’s stale review December 30, 2024 10:14

Made relevant changes - their all minor so rather than wait for Amy to appear, we're going to assume they're good - was agreed on a call

@mmyoungman mmyoungman merged commit 8cf86df into dev Dec 30, 2024
8 checks passed
@mmyoungman mmyoungman deleted the EES-5738 branch December 30, 2024 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants