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

Extra column to materialised view and Unit Test #64

Merged
merged 2 commits into from
Mar 16, 2021

Conversation

latin-panda
Copy link
Contributor

@latin-panda latin-panda commented Feb 26, 2021

Description

This PR:

  • Adds contact_type column to contactview_metadata materialized view.
  • Adds a unit test to check that materialized views have the required columns defined.

Ticket: #61

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

@latin-panda latin-panda changed the title Adding extra column to materialised view Extra column to materialised view and Unit Test Feb 26, 2021
@latin-panda
Copy link
Contributor Author

latin-panda commented Feb 27, 2021

Hi @garethbowen! If you have a chance, can you please have a look at this draft and my following questions? Thank you!

  1. Currently we have a view for type=person. Do we need another view for type=contact or perhaps reuse the one for person by expanding the filter: type=person && type=contact ?
  2. In the ticket there is a request to add a ALTER [OBJECT_TYPE] [OBJECT_NAME] OWNER TO full_access; to all objects, is it okay to use this specific role in the scripts? Is this repository only used by Medic (doesn't have private label)?
    I'm running the Postgres with Docker and this role isn't setup in there, if we add this role to the script then maybe we need to set it in the Postgres Docker, so the script doesn't fail when running locally. Another alternative is to add some steps in the Readme file to include the role manually as part of the local setup.
  3. Do we want to use git actions instead of travis for this repo?

Copy link
Contributor

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

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

LGTM!

@garethbowen
Copy link
Contributor

Currently we have a view for type=person. Do we need another view for type=contact or perhaps reuse the one for person by expanding the filter: type=person && type=contact ?

This is complicated because there can be multiple "person" types now (eg: "chw", "supervisor", etc) and they are named arbitrarily so you can't created hardcoded views based on the contact_type. I think we should have have a new materialised view that has a column for type which is coalesced the same way we do in CHT Core, eg: SELECT COALESCE (contact_type, type) AS contact_type. Obviously this will mean the contactview_person_fields view is broken so we should somehow deprecate it, keep it around for backwards compatibility, and recommend people migrate to the new view.

In the ticket there is a request to add a ALTER [OBJECT_TYPE] [OBJECT_NAME] OWNER TO full_access; to all objects, is it okay to use this specific role in the scripts? Is this repository only used by Medic (doesn't have private label)?
I'm running the Postgres with Docker and this role isn't setup in there, if we add this role to the script then maybe we need to set it in the Postgres Docker, so the script doesn't fail when running locally. Another alternative is to add some steps in the Readme file to include the role manually as part of the local setup.

This repo is designed to support all CHT deployments, even self hosted. The reason we have two repos is one is generic enough it could be used for any sort of CouchDB -> PG replication, not just CHT. So I don't think we should hardcode Medic-specific roles, but maybe we can include that as a separate script. Regardless it seems to be outside the scope of this issue, so consider raising a new issue for that.

Do we want to use git actions instead of travis for this repo?

Definitely! It doesn't need to block this work so unless the CI is slowing you down feel free to raise a new issue for that and add it to the backlog.

@latin-panda latin-panda marked this pull request as ready for review March 1, 2021 03:21
@latin-panda
Copy link
Contributor Author

Thanks @garethbowen for the review and information! I'm going to raise new issues for each of the 3 questions and pick up the one about CI, as learning experience and also because travis is failing with node 8. We were able to run this project in node 10, I think is good opportunity for a CI upgrade.

@latin-panda
Copy link
Contributor Author

Ticket for question 1: Create new materialised view
Ticket for question 2: Reassign ownership of object in new script
Ticket for question 3: Upgrade CI to use Github Actions

@garethbowen
Copy link
Contributor

We have to be a bit careful about dropping backwards compatibility as it may require a Node upgrade in production. See if you can get it to work in Node 8, and if not we can explore dropping support.

@latin-panda
Copy link
Contributor Author

Hi @garethbowen Sure, as soon as my current assignment is done, I'll try to fix the project to work with Node 8.
I will do that in a separate PR since it's already a problem in master branch and to have a separate commit targeting the fix :)

@latin-panda latin-panda merged commit 34f1d2a into master Mar 16, 2021
@latin-panda latin-panda deleted the 61-update-contactview-metadata branch March 16, 2021 10:40
latin-panda added a commit that referenced this pull request Mar 19, 2021
Ticket: #61

In this commit:
* Add contact_type column to contactview_metadata materialized view.
* Add a unit test to check that materialized views have the required columns defined.
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.

2 participants