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

Refactor geo-coverage property for Initiatives to treat it as with the other resource types #1615

Open
joseAyudarte91 opened this issue Sep 28, 2023 · 0 comments
Assignees

Comments

@joseAyudarte91
Copy link
Contributor

joseAyudarte91 commented Sep 28, 2023

In order to reduce the complexity and optimize performance when dealing with the geo-coverage property of any resource type, we need to refactor the way geo-coverage property is stored and retrieved for Initiatives, so it is treated exactly the same way as the other resource types.
From the backend side, we are storing already most of the mentioned information in the initiative_geo_coverage DB table, but we have the information duplicated in a different format (JSONB), using some other properties for that, which some times are used and other times not. Furthermore, we need to stop using country reference that was replaced by the geo_coverage concept some time ago (which covers that and other use-cases).

On the other hand, geo_coverage_type property is stored as a JSONB and we need to migrate the current data and use the same type we use for the other resource types, which also simplifies the FE that needs to deal with this property separately for Initiatives.

Besides, this refactor will also fix a bug when trying to create initiatives with "national" "geo_coverage_type" value, as we will the generic approach to send and get geo-coverage information.

As a further side note: we are going to merge /api/initiative/{id} endpoint with the current Resource Details GET one (so this last endpoint will return all the raw data from a given initiative) in order to avoid doing extra work for the geo-coverage refactor in the former endpoint and avoid maintaining 2 endpoints.

@joseAyudarte91 joseAyudarte91 self-assigned this Sep 28, 2023
joseAyudarte91 added a commit that referenced this issue Sep 28, 2023
[Re #1615]

Now we are dealing with `geo-coverage` in the same way as with the other
resource types. From one side, `geo_coverage_type` property has been
added to the Initiatives DB model and the related data that was stored
in other format and property has been migrated there. There are some
non-used values for the `geo_coverage_type`, but we can just replace
those the next time those initiatives are updated, to keep things
simple.

On the other hand, we have gotten rid of the old `country` and
`subnational_city` legacy properties that have been replaced already by
the shared `geo-coverage` concept, dropping the related columns about
it, that also included `geo-coverage` values-related duplicated
information. Besides, legacy DB views related to initiatives have been
deleted, as they were relying on those old properties and we could not
drop them otherwise.

Some tests have been also removed as they were as legacy as the
refactored code (which implies the rename and modifications of some
example data files used there) and some others have been updated to
adapt them to the refactored code or just simplify the logic that was
unnecesary complex.

When adapting BRS integration-related code, we have also unified the way
to transform data before being persisted, to avoid doing operations
twice and differently in separated places.

As a side note, there is an endpoint for getting initiative data that
does not use the common `geo-coverage` related tables for providing
this information. As it was using legacy `country` and similar
references, we have get rid of this logic. However, its future is being
discussed, as it should not be needed this endpoint at all.
joseAyudarte91 added a commit that referenced this issue Sep 28, 2023
[Re #1615]

Also reformatted file that was wrongly formatted (unrelated).
joseAyudarte91 added a commit that referenced this issue Oct 3, 2023
[Re #1615]

As we have agreed, we are getting rid of the redundant `/api/initiative/
{id}` GET endpoint to provide all the information in a single endpoint:
the current one already used for resource detail GET operation.

In this case, we have needed to change the shared topic generation SQL
code to get all the properties from initiative entity so then we can
merge it with the other renamed and transformed properties without the
need to fetch the data from the DB twice.

During this process we have got rid of legacy and not-existing column
aliases used in the shared query to get initiatives data for Browse and
Detail API.
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

No branches or pull requests

1 participant