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

Fixes for 76 search_filters and create_spatial_views sql files #1356

Open
wants to merge 6 commits into
base: 76_upgrade
Choose a base branch
from

Conversation

SDScandrettKint
Copy link
Member

@SDScandrettKint SDScandrettKint commented Jan 3, 2025

Closes #1355 and #1358

Removes sortorder and enabled, and adds an empty config object to preliminary_sql/search_filters.sql

Adds languageid and 'en' to all sp_view rows of post_sql/create_spatial_views.sql

Should the config for BNG filter be populated?

@SDScandrettKint SDScandrettKint self-assigned this Jan 3, 2025
@SDScandrettKint SDScandrettKint requested a review from chiatt January 3, 2025 12:49
@SDScandrettKint
Copy link
Member Author

I am still getting issues with BNG filter:

loading preliminary sql
  /home/samuel/arches_dev/arches_her/AfHER_application/arches_her/arches_her/pkg/preliminary_sql/search_filters.sql
duplicate key value violates unique constraint "search_component_componentpath_key"
DETAIL:  Key (componentpath)=(views/components/search/bng-filter) already exists.

Failed to load sql files

Is it populating from 0001_initial.py? Is search_filters.py required anymore?
The SearchComponent.objects.update_or_create in 0001_initial still includes sortorder and enabled, so do we need a new migration to update this (and potentially others)?


Lastly, post_sql only runs successfully if the spatial_views_intl_hotfix.sql file is deleted, @aj-he are we able to delete this hotfix file for 7.6?

@SDScandrettKint SDScandrettKint requested a review from aj-he January 3, 2025 13:30
@aj-he
Copy link
Collaborator

aj-he commented Jan 3, 2025

@SDScandrettKint - given the new pattern of using migrations in apps, I suspect that the inclusion of sql files in the package should be removed in favour of having them being in a migration file, or as files used by a migration process as per @bferguso proposal here: archesproject/arches#11696.

With reference to your specific question on the spatial view hotfix, this can be removed from 7.6 onwards. However, can you check that an existing 7.5 instance with hotfix in place can be migrated to this new version. If there is an issue, you may need to add a reversible migration to handle any issues.

We need to make sure we include release instructions if users need to make any local changes. @chiatt could you advise what needs doing for this?

We also need to bump the arches_her version number. There is a question as to how we bump it? Given Arches moving up a minor version, we should do the same? Again, @chiatt, any thoughts on this?

@SDScandrettKint
Copy link
Member Author

@aj-he latest commit 559c981 adds the start of a 0002 migration (may need a better name). Currently this migration just adds the search overlay map layers. Previously these map layers were populated with a random UUID, so I've done a name search incase the layers already exist (as we don't want duplicates), but if not then assigned them a set UUID. I'm unsure if we want it to continue using a random uuid1 or if a set UUID is better?
I've also removed the spatial view hotfix, but still need to test upgrading from 7.5 -> 7.6.

All preliminary_sql files can be included in a migration, but the post_sql files are a little more tricky, as they rely on the graphs/nodes already populated (spatial views require geometry nodes, and updating the graphs table with the report templates requires a populated graphs table). Should these remain in post_sql, or is there a better way we can manage this within a migration?

@chiatt
Copy link
Member

chiatt commented Jan 16, 2025

@SDScandrettKint - given the new pattern of using migrations in apps, I suspect that the inclusion of sql files in the package should be removed in favour of having them being in a migration file, or as files used by a migration process as per @bferguso proposal here: archesproject/arches#11696.

With reference to your specific question on the spatial view hotfix, this can be removed from 7.6 onwards. However, can you check that an existing 7.5 instance with hotfix in place can be migrated to this new version. If there is an issue, you may need to add a reversible migration to handle any issues.

We need to make sure we include release instructions if users need to make any local changes. @chiatt could you advise what needs doing for this?

We also need to bump the arches_her version number. There is a question as to how we bump it? Given Arches moving up a minor version, we should do the same? Again, @chiatt, any thoughts on this?

Yeah - I would bump this up a minor version given the significance of the changes. Regarding documenting the release - for AfS we added a releases directory, following the same pattern as core Arches, so I think that would be a good pattern for AfHERs as well.

Copy link
Member

@chiatt chiatt left a comment

Choose a reason for hiding this comment

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

Rather than querying map layers based on name, it would be more dependable to query based on the source property found in the layer definition json (search-results-points, search-results-hashes, search-results-hex).

@SDScandrettKint
Copy link
Member Author

Rather than querying map layers based on name, it would be more dependable to query based on the source property found in the layer definition json (search-results-points, search-results-hashes, search-results-hex).

In 67c315b I replaced searching for the name with searching the layerdefinition source attribute.
This has been tested with setting up the db via the package, migrating with the map_layers already in, and migrating with the map layers having changed their name.

I also added a new migration file (which could possibly be squashed into 0002 within this commit) for adding the BNG filter into the new search component structure. This migration needs to come after the core migration 10804_core_search_filters, however, django currently isn't playing nice and allowing this to work as it should when setting up the db from scratch with the package. If AfHER has already been installed up to 0001_initial, then these migrations work as intended.

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