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

Eatyourpeas/issue20 #22

Open
wants to merge 47 commits into
base: live
Choose a base branch
from
Open

Eatyourpeas/issue20 #22

wants to merge 47 commits into from

Conversation

eatyourpeas
Copy link
Member

Pull Request Summary

  1. Dockerises all the services in line with RCPCH Playbook, using E12 as a template: includes refactor of s folder to use s/up.
  2. Includes celery/redis/celerybeat/flower along the same lines. This is because updating of organisations is a scheduled task and pulls from ORD NHS spine - this is called from a new task.py in project root as an async function poll_ORD_spineservices_update_organisations_and_trusts(), according to a new beat schedule in settings.py:
CELERY_BEAT_SCHEDULE = {
    "update-organisations-daily-at-six-am": {
        "task": "rcpch_census_platform.tasks.poll_ORD_spineservices_update_organisations_and_trusts",
        "schedule": crontab(hour="6", minute=0),
        "options": {
            "expires": 15.0,
        },
    }
}

The code for making the API call and updating the models with any newly published changes in the ODS can be found in general_functions/ods_update.py
3. Rename project and application as deprivation scores is now not the only service provided. This is done according to DRF principles - where the application sits within the project folder, not at the same level
4. Introduce seeding of all Organisations/Trusts/Local Health Boards/NHS England Regions and Countries, as well as OPENUK Networks (childhood epilepsy network) and Paediatric Diabetes Units. Includes boundary files from ONS and seeding of the same. This is almost identical to E12.
5. Move seeding of existing Index of Multiple Deprivation data and dependencies into the migrations.
6. Reorganise and refactor all serialisers, views and URLs and cull to include only essential endpoints. These are all refactored with DRF-spectacular, to OpenAPI 3.0 spec.
7. Add OpenAPI examples to all end points. All endpoints closed to GET request only.
7. Add DRF-GIS library to allow GIS data to be included
8. Rewrite documentation, borrowing heavily from E12 for legal/clinical safety aspects. Remove previously written endpoints and replace with OpenAPI spec.

Note this does not include tests which are in a parallel PR to the currently live branch. That branch (#21) arguably should be merged first as those tests need including in this.

Does not include

Tests of the API Spec and endpoints. These should be the next piece of work and should be fairly easy now that this work is done, and tests of the IMD endpoints (which are the most production critical) are already complete in #21.

Implications

The IMD calculation endpoints have changed:

index_of_multiple_deprivation/extended and
index_of_multiple_deprivation/quantile
The code within the functions as has not changed, through the quantile calculation has been moved into a general_function and moved out of the seeding.
This will have implications for the Epilepsy12 so the endpoints will need refactoring there also.

Dockerising the RCPCH Census Platform will mean it will be possible to deprecate the Azure database as a service which currently is used in live.

eatyourpeas and others added 30 commits December 12, 2023 22:11
begin work on viewsets and serializers
add boundaries
add gis to dockerfile
merge organisation bank and deprivation scores
add docs
fix seeding of deprivation scores
fix migration
add ord update function
wire up to celery beat
note not all organisations are in the database
-currently import error
update url paths
refactor data_zone_code to code
schema broken
fix imd and organisation viewsets and serializers
return organisation by ods_code
@eatyourpeas eatyourpeas self-assigned this Dec 27, 2023
@eatyourpeas eatyourpeas added documentation Improvements or additions to documentation feature request New feature or request labels Dec 27, 2023
@eatyourpeas
Copy link
Member Author

image image Slightly old screen shot as the London Boroughs are in there also.

Merging this would close #20
Note that although GP practices are not included here, actually an exhaustive list of these is available on demand from https://directory.spineservices.nhs.uk/ORD/2-0-0 so a list of GP practices probably does not need to be part of RCPCH Census Platform. Functions to look up GP practice by ODS code are found in general_functions/gp_surgeries.py

@eatyourpeas
Copy link
Member Author

eatyourpeas commented Jan 20, 2024

Discussion with @pacharanero and @dc2007git and Keith Blackwood.
Decision taken to remove the organisation element from the rcpch census platform and leave it in its own repo
A secondary suggestion was to consider removing the database from the census platform to make it more modular.

Initial step though is to:

  • remove all reference to organisations and put it in a separate repo
  • leave the remaining app as a dockerised entity, but remove celery and celery beat / redis
  • remove postgis, replace with sqlite
  • change ports for 8005 for django, 8006 for documentation
  • update documentation
  • add tests from tests #19

remove associated views/serializer/routes
dockerize - remove celery/beat/flower
remove postgis and postgis container
add sqlite - commit db to repo
change ports to 8005 for django, 8006 for docs
@eatyourpeas
Copy link
Member Author

Here are some screenshots of the update
Now includes tests, fully dockerized, sqlite3 rather than postgresql and only 3 containers (django+db, caddy and docs)
This will allow us to deprecated the database service
The sqlite3 instance can be refactored to use the spreadsheets and pandas in time if there is appetite but is lightweight already
the endpoints for the deprivation score have changed so once deployed epilepsy12 config will need updating

image image

@eatyourpeas eatyourpeas mentioned this pull request Jan 21, 2024
@dc2007git
Copy link

@eatyourpeas looking forward to the pairing session on this! Just a small question after having a look through the code myself, in general_functions/ods_update.py, in line 148 and 149, I noticed that trust.town and trust.postcode are set to print statements. Since print returns None, would this not set those 2 variables equal to None?

@eatyourpeas
Copy link
Member Author

Thanks @dc2007git sorry yes those are debug statements. That set of functions ultimately will hit the ORD API for any published updates to organisation or trust structure, will search for a matching organisation in our database and update the record and bump the date. The workflow is not yet complete so not for primetime.

@anchit-chandran
Copy link
Collaborator

Just a general FYI - seeing a lot of prints but we've moved to exclusively using logger.debug or higher. Prints will pollute our logs with undifferentiated messages!

@anchit-chandran
Copy link
Collaborator

Also should this be PR'ing into development instead of live?

@eatyourpeas
Copy link
Member Author

That is a good point @anchit-chandran yes, should really be into development. We don't really have a work flow like that for this repo but am open to setting this up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation feature request New feature or request
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants