From a4450ee553e2a590265211f9dd8c788d57e79ded Mon Sep 17 00:00:00 2001 From: Sabrina Yan <9669990+violetbrina@users.noreply.github.com> Date: Fri, 4 Oct 2024 15:30:47 +1000 Subject: [PATCH 1/8] Uplift code quality in metamist repo (#957) * Add tests directory to sonarqube config file * Hide db backup folder results * Fix test exclusion for code coverage settings * Address all current metamist bugs * Fix security issues, including exclusive use of formatMoney with resistent regex * formatMoney bug fix * Fix the ValueFilter so that the useState is called in the same order in every render * Change so that the nonroot user is used in the db dockerfile * Change so that the nonroot user is used in the api dockerfile * Fix default logging level to warning to reduce the leak of secure information * Add merge_group to the test workflow * Fix web pages format using prettier * Fix web pages format using prettier - npm ci ran first * Add referenceBranch to sonar-project.properties * Update test workflow to include metamist dev branch version in analysis * Add Sonarqube scan to deploy check, update db Dockerfile based on comments * Fix run fastapi command in api Dockerfile --- .github/workflows/deploy.yaml | 147 ++++++++++++++++++ .github/workflows/test.yaml | 5 +- .gitignore | 2 + db/deploy/Dockerfile | 18 ++- db/python/connect.py | 2 +- db/python/gcp_connect.py | 2 +- deploy/api/Dockerfile | 21 ++- metamist/audit/audit_upload_bucket.py | 2 +- .../audit/delete_assay_files_from_audit.py | 2 +- metamist/parser/generic_parser.py | 3 +- sonar-project.properties | 4 + web/src/App.tsx | 2 +- web/src/index.css | 2 +- .../pages/billing/BillingCostByAnalysis.tsx | 6 +- .../pages/billing/BillingCostByCategory.tsx | 2 +- web/src/pages/billing/BillingCostByMonth.tsx | 2 +- web/src/pages/billing/BillingCostByTime.tsx | 2 +- web/src/pages/billing/BillingHome.tsx | 2 +- .../pages/billing/BillingInvoiceMonthCost.tsx | 19 +-- .../components/BillingCostByTimeTable.tsx | 13 +- web/src/pages/comments/CommentHistory.tsx | 2 +- .../comments/entities/FamilyCommentsView.tsx | 2 +- .../entities/ParticipantCommentsView.tsx | 2 +- .../comments/entities/ProjectCommentsView.tsx | 2 +- .../comments/entities/SampleCommentsView.tsx | 2 +- .../entities/SequencingGroupCommentsView.tsx | 2 +- web/src/pages/project/ValueFilter.tsx | 25 ++- web/src/shared/components/Header/NavBar.css | 1 - web/src/shared/components/SeqPanel.tsx | 2 +- .../components/pedigree/TangledTree.tsx | 2 +- web/src/shared/utilities/formatMoney.ts | 4 +- 31 files changed, 228 insertions(+), 76 deletions(-) diff --git a/.github/workflows/deploy.yaml b/.github/workflows/deploy.yaml index 0c69fdcbf..b96a05646 100644 --- a/.github/workflows/deploy.yaml +++ b/.github/workflows/deploy.yaml @@ -12,8 +12,155 @@ permissions: contents: read jobs: + unittests: + name: Run unit tests + # Run on merge to main, where the commit name starts with "Bump version:" (for bump2version) + # if: "startsWith(github.event.head_commit.message, 'Bump version:')" + runs-on: ubuntu-latest + env: + DOCKER_BUILDKIT: 1 + BUILDKIT_PROGRESS: plain + CLOUDSDK_CORE_DISABLE_PROMPTS: 1 + # used for generating API + SM_DOCKER: samplemetadata:dev + defaults: + run: + shell: bash -eo pipefail -l {0} + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-python@v5 + with: + python-version: '3.11' + + - uses: actions/setup-java@v4 + with: + distribution: 'temurin' # See 'Supported distributions' for available options + java-version: '17' + + - name: Setup build env + run: | + set -euxo pipefail + + pip install --no-deps -r requirements-dev.txt + + # openapi-generator + wget https://repo1.maven.org/maven2/org/openapitools/openapi-generator-cli/5.3.0/openapi-generator-cli-5.3.0.jar -O openapi-generator-cli.jar + + # liquibase connector + pushd db/ + wget https://repo1.maven.org/maven2/org/mariadb/jdbc/mariadb-java-client/3.0.3/mariadb-java-client-3.0.3.jar + popd + # liquibase + VERSION=4.28.0 + curl -L https://github.com/liquibase/liquibase/releases/download/v${VERSION}/liquibase-${VERSION}.zip --output liquibase-${VERSION}.zip + unzip -o -d liquibase liquibase-${VERSION}.zip + echo "$(pwd)/liquibase" >> $GITHUB_PATH + + - name: 'build image' + run: | + docker build \ + --build-arg SM_ENVIRONMENT=local \ + --tag $SM_DOCKER \ + -f deploy/api/Dockerfile \ + . + + - name: 'build deployable API' + run: | + export OPENAPI_COMMAND="java -jar openapi-generator-cli.jar" + python regenerate_api.py + pip install . + + - name: 'Run unit tests' + id: runtests + run: | + coverage run -m pytest --doctest-modules --doctest-continue-on-failure test/ --junitxml=test-execution.xml + rc=$? + coverage xml + + echo "rc=$rc" >> $GITHUB_OUTPUT + + - name: 'Upload coverage report' + uses: codecov/codecov-action@v4 + with: + files: ./coverage.xml + token: ${{ secrets.CODECOV_TOKEN }} + + - name: 'Save coverage report as an Artifact' + uses: actions/upload-artifact@v4 + with: + name: coverage-report + path: ./coverage.xml + + - name: 'Save execution report as an Artifact' + uses: actions/upload-artifact@v4 + with: + name: execution-report + path: ./test-execution.xml + + - name: 'build web front-end' + run: | + set -eo pipefail + pushd web + # installs package-lock, not what it thinks it should be + npm ci + npm run build + rc=$? + + echo "web_rc=$rc" >> $GITHUB_OUTPUT + # eventually run web front-end tests + popd + + - name: Fail if unit tests are not passing + if: ${{ steps.runtests.outputs.rc != 0}} + uses: actions/github-script@v6 + with: + script: | + core.setFailed('Unittests failed with rc = ${{ steps.runtests.outputs.rc }}') + + - name: Fail if web build fails + if: ${{ steps.runtests.outputs.rc != 0}} + uses: actions/github-script@v6 + with: + script: | + core.setFailed('Web failed to build with rc = ${{ steps.runtests.outputs.web_rc }}') + + sonarqube: + name: SonarQube scan + runs-on: ubuntu-latest + needs: unittests + environment: production + if: github.ref == 'refs/heads/main' || github.ref == 'refs/heads/dev' + steps: + - uses: actions/checkout@v4 + with: + fetch-depth: 0 # Shallow clones should be disabled for a better relevancy of analysis + + # Download the coverage report artifact + - name: 'Download coverage and execution report' + uses: actions/download-artifact@v4 + with: + pattern: '*-report' + + # Perform the SonarQube scan + - uses: sonarsource/sonarqube-scan-action@master + env: + SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} + SONAR_HOST_URL: ${{ secrets.SONAR_HOST_URL }} + + # Optional: Fail the job if Quality Gate is red + # If you wish to fail your job when the Quality Gate is red, uncomment the + # following lines. This would typically be used to fail a deployment. + # - uses: sonarsource/sonarqube-quality-gate-action@master + # timeout-minutes: 5 + # env: + # SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} + # SONAR_HOST_URL: ${{ secrets.SONAR_HOST_URL }} + deploy: + name: Deploy runs-on: ubuntu-latest + needs: sonarqube environment: production env: DOCKER_BUILDKIT: 1 diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index efa6b791d..9651715dc 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -1,11 +1,10 @@ name: Test -on: push +on: + push: jobs: unittests: name: Run unit tests - # Run on merge to main, where the commit name starts with "Bump version:" (for bump2version) - # if: "startsWith(github.event.head_commit.message, 'Bump version:')" runs-on: ubuntu-latest env: DOCKER_BUILDKIT: 1 diff --git a/.gitignore b/.gitignore index 1d5308de5..2382ebfd9 100644 --- a/.gitignore +++ b/.gitignore @@ -68,3 +68,5 @@ profiles # sonarqube .scannerwork/ + +latest_backup/ diff --git a/db/deploy/Dockerfile b/db/deploy/Dockerfile index 24facb0f3..35072a678 100644 --- a/db/deploy/Dockerfile +++ b/db/deploy/Dockerfile @@ -8,17 +8,31 @@ ENV LIQUIBASE_VERSION=4.26.0 ENV MARIADB_JDBC_VERSION=3.0.3 # Install system dependencies -RUN apt-get update && apt-get install -y --no-install-recommends gcc git ssh default-jdk wget unzip +RUN apt-get update && apt-get install -y --no-install-recommends gcc git ssh default-jdk wget unzip \ + && apt-get clean \ + && rm -rf /var/lib/apt/lists/* + +# Create a non-root user and group +RUN groupadd -r appuser && useradd -r -g appuser -d /home/appuser -m -s /bin/bash appuser # Download and install Liquibase RUN wget https://github.com/liquibase/liquibase/releases/download/v${LIQUIBASE_VERSION}/liquibase-${LIQUIBASE_VERSION}.zip \ && unzip liquibase-${LIQUIBASE_VERSION}.zip -d /opt/liquibase \ && chmod +x /opt/liquibase/liquibase \ - # Clean up to reduce layer size && rm liquibase-${LIQUIBASE_VERSION}.zip \ + # Clean up to reduce layer size && apt-get clean \ && rm -rf /var/lib/apt/lists/* +# Change ownership of the liquibase directory to the non-root user +RUN chown -R appuser:appuser /opt/liquibase + +# Switch to the non-root user +USER appuser + +# Set the working directory +WORKDIR /home/appuser + # Download the MariaDB JDBC driver RUN wget https://downloads.mariadb.com/Connectors/java/connector-java-${MARIADB_JDBC_VERSION}/mariadb-java-client-${MARIADB_JDBC_VERSION}.jar RUN mv mariadb-java-client-${MARIADB_JDBC_VERSION}.jar /opt/ diff --git a/db/python/connect.py b/db/python/connect.py index 2d51b5e8f..fd9e97e08 100644 --- a/db/python/connect.py +++ b/db/python/connect.py @@ -23,7 +23,7 @@ ) from models.models.project import Project, ProjectId, ProjectMemberRole -logging.basicConfig(level=logging.DEBUG) +logging.basicConfig(level=logging.WARNING) logger = logging.getLogger(__name__) TABLES_ORDERED_BY_FK_DEPS = [ diff --git a/db/python/gcp_connect.py b/db/python/gcp_connect.py index 5d0271e68..fecfc913b 100644 --- a/db/python/gcp_connect.py +++ b/db/python/gcp_connect.py @@ -10,7 +10,7 @@ from db.python.utils import InternalError -logging.basicConfig(level=logging.DEBUG) +logging.basicConfig(level=logging.WARNING) logger = logging.getLogger(__name__) diff --git a/deploy/api/Dockerfile b/deploy/api/Dockerfile index da0ab23b3..a4e43553f 100644 --- a/deploy/api/Dockerfile +++ b/deploy/api/Dockerfile @@ -7,15 +7,26 @@ ENV SM_ENVIRONMENT ${SM_ENVIRONMENT} # Allow statements and log messages to immediately appear in the Knative logs. ENV PYTHONUNBUFFERED 1 -EXPOSE $PORT +# Create a non-root user and group +RUN groupadd -r appuser && useradd -r -g appuser -d /home/appuser -m -s /bin/bash appuser +# Set the working directory WORKDIR /app/sample_metadata/ -COPY requirements.txt requirements.txt +# Copy requirements file and install dependencies +COPY requirements.txt requirements.txt RUN pip install --no-cache-dir --no-deps -r requirements.txt +# Copy the application code COPY api api -COPY db db/ -COPY models models/ -CMD uvicorn --port ${PORT} --host 0.0.0.0 api.server:app +# Change ownership of the application directory to the non-root user +RUN chown -R appuser:appuser /app/sample_metadata/ + +# Switch to the non-root user +USER appuser + +EXPOSE $PORT + +# Command to run the FastAPI app +CMD ["uvicorn", "api.main:app", "--host", "0.0.0.0", "--port", "$PORT"] diff --git a/metamist/audit/audit_upload_bucket.py b/metamist/audit/audit_upload_bucket.py index 22827a532..c58e006ee 100644 --- a/metamist/audit/audit_upload_bucket.py +++ b/metamist/audit/audit_upload_bucket.py @@ -333,7 +333,7 @@ def main( if __name__ == '__main__': logging.basicConfig( - level=logging.INFO, + level=logging.WARNING, format='%(asctime)s %(levelname)s %(module)s:%(lineno)d - %(message)s', datefmt='%Y-%m-%d %H:%M:%S', stream=sys.stderr, diff --git a/metamist/audit/delete_assay_files_from_audit.py b/metamist/audit/delete_assay_files_from_audit.py index c60e42439..5aedd2e55 100644 --- a/metamist/audit/delete_assay_files_from_audit.py +++ b/metamist/audit/delete_assay_files_from_audit.py @@ -86,7 +86,7 @@ def main(delete_field_name, delete_file_path): if __name__ == '__main__': logging.basicConfig( - level=logging.INFO, + level=logging.WARNING, format='%(asctime)s %(levelname)s %(module)s:%(lineno)d - %(message)s', datefmt='%Y-%M-%d %H:%M:%S', stream=sys.stderr, diff --git a/metamist/parser/generic_parser.py b/metamist/parser/generic_parser.py index 0aece2af4..db3576c9c 100644 --- a/metamist/parser/generic_parser.py +++ b/metamist/parser/generic_parser.py @@ -43,9 +43,8 @@ else: from typing_extensions import Literal -logging.basicConfig() +logging.basicConfig(level=logging.WARNING) logger = logging.getLogger(__file__) -logger.setLevel(logging.INFO) PRIMARY_EXTERNAL_ORG = '' diff --git a/sonar-project.properties b/sonar-project.properties index 576979e29..0719cfb89 100644 --- a/sonar-project.properties +++ b/sonar-project.properties @@ -2,3 +2,7 @@ sonar.projectKey=populationgenomics_metamist sonar.python.version=3.11 sonar.python.coverage.reportPaths=coverage.xml sonar.python.xunit.reportPath=test-execution.xml +sonar.sources=. +sonar.exclusions=test/**,*.xml +sonar.tests=test/ +sonar.cpd.cache.enabled=true diff --git a/web/src/App.tsx b/web/src/App.tsx index 4fa16bb47..65676e7b9 100644 --- a/web/src/App.tsx +++ b/web/src/App.tsx @@ -3,7 +3,7 @@ import * as React from 'react' import { BrowserRouter as Router } from 'react-router-dom' import Routes from './Routes' import NavBar from './shared/components/Header/NavBar' -import { ViewerContext, useViewer } from './viewer' +import { useViewer, ViewerContext } from './viewer' const App: React.FunctionComponent = () => { const { viewer, error } = useViewer() diff --git a/web/src/index.css b/web/src/index.css index 3e01b622c..016e5d9b9 100644 --- a/web/src/index.css +++ b/web/src/index.css @@ -151,7 +151,6 @@ body { background-color: var(--color-bg); color: var(--color-text-primary); - background: var(--color-bg); } code { @@ -203,6 +202,7 @@ blockquote { .ui .modal > .content { background: var(--color-bg-card); } + .ui .modal > .header { background: var(--color-bg-card); color: var(--color-text-primary); diff --git a/web/src/pages/billing/BillingCostByAnalysis.tsx b/web/src/pages/billing/BillingCostByAnalysis.tsx index a225b4bde..b3e4f9538 100644 --- a/web/src/pages/billing/BillingCostByAnalysis.tsx +++ b/web/src/pages/billing/BillingCostByAnalysis.tsx @@ -5,7 +5,6 @@ import { Button, Card, Dropdown, Grid, Input, Message } from 'semantic-ui-react' import { PaddedPage } from '../../shared/components/Layout/PaddedPage' import LoadingDucks from '../../shared/components/LoadingDucks/LoadingDucks' import generateUrl from '../../shared/utilities/generateUrl' -import { getMonthStartDate } from '../../shared/utilities/monthStartEndDate' import { AnalysisCostRecord, BillingApi } from '../../sm-api' import BatchGrid from './components/BatchGrid' @@ -22,9 +21,6 @@ const BillingCostByAnalysis: React.FunctionComponent = () => { const [isLoading, setIsLoading] = React.useState(true) const [error, setError] = React.useState() const [data, setData] = React.useState() - const [start, setStart] = React.useState( - searchParams.get('start') ?? getMonthStartDate() - ) const setBillingRecord = (records: AnalysisCostRecord[]) => { setIsLoading(false) @@ -138,7 +134,7 @@ const BillingCostByAnalysis: React.FunctionComponent = () => { setError(undefined)}> {error}
- diff --git a/web/src/pages/billing/BillingCostByCategory.tsx b/web/src/pages/billing/BillingCostByCategory.tsx index fce19ca54..629db7422 100644 --- a/web/src/pages/billing/BillingCostByCategory.tsx +++ b/web/src/pages/billing/BillingCostByCategory.tsx @@ -191,7 +191,7 @@ const BillingCostByCategory: React.FunctionComponent = () => { setError(undefined)}> {error}
- diff --git a/web/src/pages/billing/BillingCostByMonth.tsx b/web/src/pages/billing/BillingCostByMonth.tsx index 40dc076b7..3c69e616d 100644 --- a/web/src/pages/billing/BillingCostByMonth.tsx +++ b/web/src/pages/billing/BillingCostByMonth.tsx @@ -150,7 +150,7 @@ const BillingCostByTime: React.FunctionComponent = () => { setError(undefined)}> {error}
- diff --git a/web/src/pages/billing/BillingCostByTime.tsx b/web/src/pages/billing/BillingCostByTime.tsx index 70f476022..ee59c8289 100644 --- a/web/src/pages/billing/BillingCostByTime.tsx +++ b/web/src/pages/billing/BillingCostByTime.tsx @@ -185,7 +185,7 @@ const BillingCostByTime: React.FunctionComponent = () => { setError(undefined)}> {error}
- diff --git a/web/src/pages/billing/BillingHome.tsx b/web/src/pages/billing/BillingHome.tsx index 4cb827ea9..706b45e2a 100644 --- a/web/src/pages/billing/BillingHome.tsx +++ b/web/src/pages/billing/BillingHome.tsx @@ -3,7 +3,7 @@ import ReactGoogleSlides from 'react-google-slides' import { Button, Menu, MenuItem, Segment, SemanticWIDTHS } from 'semantic-ui-react' import { PaddedPage } from '../../shared/components/Layout/PaddedPage' import { ThemeContext } from '../../shared/components/ThemeProvider' -import { IBillingPage, billingPages } from './BillingPages' +import { billingPages, IBillingPage } from './BillingPages' // Google Slides const Slides = React.memo(function ReactGoogleSlidesWrapper({ link }: { link: string }) { diff --git a/web/src/pages/billing/BillingInvoiceMonthCost.tsx b/web/src/pages/billing/BillingInvoiceMonthCost.tsx index c9366c14b..c8710c489 100644 --- a/web/src/pages/billing/BillingInvoiceMonthCost.tsx +++ b/web/src/pages/billing/BillingInvoiceMonthCost.tsx @@ -13,6 +13,7 @@ import { HorizontalStackedBarChart } from '../../shared/components/Graphs/Horizo import { PaddedPage } from '../../shared/components/Layout/PaddedPage' import Table from '../../shared/components/Table' import { convertFieldName } from '../../shared/utilities/fieldName' +import formatMoney from '../../shared/utilities/formatMoney' import generateUrl from '../../shared/utilities/generateUrl' import { BillingApi, BillingColumn, BillingCostBudgetRecord } from '../../sm-api' import FieldSelector from './components/FieldSelector' @@ -129,14 +130,6 @@ const BillingCurrentCost = () => { } } - function currencyFormat(num: number | undefined | null): string { - if (num === undefined || num === null) { - return '' - } - - return `$${num.toFixed(2).replace(/(\d)(?=(\d{3})+(?!\d))/g, '$1,')}` - } - function percFormat(num: number | undefined | null): string { if (num === undefined || num === null) { return '' @@ -414,7 +407,7 @@ const BillingCurrentCost = () => { { // @ts-ignore - currencyFormat(p[k.category]) + formatMoney(p[k.category]) } ) @@ -456,14 +449,14 @@ const BillingCurrentCost = () => { {invoiceMonth === thisMonth ? ( - {currencyFormat(dk.daily_cost)} + {formatMoney(dk.daily_cost)} ) : null} - {currencyFormat(dk.monthly_cost)} + {formatMoney(dk.monthly_cost)} @@ -473,14 +466,14 @@ const BillingCurrentCost = () => { {invoiceMonth === thisMonth ? ( - {currencyFormat(dk.daily_cost)} + {formatMoney(dk.daily_cost)} ) : null} - {currencyFormat(dk.monthly_cost)} + {formatMoney(dk.monthly_cost)} diff --git a/web/src/pages/billing/components/BillingCostByTimeTable.tsx b/web/src/pages/billing/components/BillingCostByTimeTable.tsx index e367b8c7c..8bf166277 100644 --- a/web/src/pages/billing/components/BillingCostByTimeTable.tsx +++ b/web/src/pages/billing/components/BillingCostByTimeTable.tsx @@ -4,6 +4,7 @@ import { IStackedAreaByDateChartData } from '../../../shared/components/Graphs/S import LoadingDucks from '../../../shared/components/LoadingDucks/LoadingDucks' import Table from '../../../shared/components/Table' import { convertFieldName } from '../../../shared/utilities/fieldName' +import formatMoney from '../../../shared/utilities/formatMoney' interface IBillingCostByTimeTableProps { heading: string @@ -105,14 +106,6 @@ const BillingCostByTimeTable: React.FC = ({ return undefined } - const currencyFormat = (num: number): string => { - if (num === undefined || num === null) { - return '' - } - - return `$${num.toFixed(2).replace(/(\d)(?=(\d{3})+(?!\d))/g, '$1,')}` - } - if (isLoading) { return (
@@ -160,7 +153,7 @@ const BillingCostByTimeTable: React.FC = ({ {headerFields().map((k) => ( - {currencyFormat(p.values[k.category])} + {formatMoney(p.values[k.category])} ))} @@ -224,7 +217,7 @@ const BillingCostByTimeTable: React.FC = ({ {headerFields().map((k) => ( - {currencyFormat( + {formatMoney( internalData.reduce( (acc, cur) => acc + cur.values[k.category], 0 diff --git a/web/src/pages/comments/CommentHistory.tsx b/web/src/pages/comments/CommentHistory.tsx index 6e4854a16..f59ec0b08 100644 --- a/web/src/pages/comments/CommentHistory.tsx +++ b/web/src/pages/comments/CommentHistory.tsx @@ -1,8 +1,8 @@ import { Box, List, ListItem, ListItemButton, ListItemText, Typography } from '@mui/material' import { DateTime } from 'luxon' import { useState } from 'react' -import { CommentContent } from './CommentContent' import { CommentData } from './commentConfig' +import { CommentContent } from './CommentContent' import { parseAuthor } from './commentUtils' export function CommentHistory(props: { comment: CommentData; theme: 'light' | 'dark' }) { diff --git a/web/src/pages/comments/entities/FamilyCommentsView.tsx b/web/src/pages/comments/entities/FamilyCommentsView.tsx index 6ec2870ef..03e8969ea 100644 --- a/web/src/pages/comments/entities/FamilyCommentsView.tsx +++ b/web/src/pages/comments/entities/FamilyCommentsView.tsx @@ -1,7 +1,7 @@ import { useQuery } from '@apollo/client' import { gql } from '../../../__generated__' -import { DiscussionView } from '../DiscussionView' import { useNewComment } from '../data/commentMutations' +import { DiscussionView } from '../DiscussionView' export const FAMILY_COMMENTS = gql(` query FamilyComments($familyId: Int!) { diff --git a/web/src/pages/comments/entities/ParticipantCommentsView.tsx b/web/src/pages/comments/entities/ParticipantCommentsView.tsx index 4468c9631..9c3453feb 100644 --- a/web/src/pages/comments/entities/ParticipantCommentsView.tsx +++ b/web/src/pages/comments/entities/ParticipantCommentsView.tsx @@ -1,7 +1,7 @@ import { useQuery } from '@apollo/client' import { gql } from '../../../__generated__' -import { DiscussionView } from '../DiscussionView' import { useNewComment } from '../data/commentMutations' +import { DiscussionView } from '../DiscussionView' export const PARTICIPANT_COMMENTS = gql(` query ParticipantComments($participantId: Int!) { diff --git a/web/src/pages/comments/entities/ProjectCommentsView.tsx b/web/src/pages/comments/entities/ProjectCommentsView.tsx index 64d28980f..ce4446591 100644 --- a/web/src/pages/comments/entities/ProjectCommentsView.tsx +++ b/web/src/pages/comments/entities/ProjectCommentsView.tsx @@ -1,7 +1,7 @@ import { useQuery } from '@apollo/client' import { gql } from '../../../__generated__' -import { DiscussionView } from '../DiscussionView' import { useNewComment } from '../data/commentMutations' +import { DiscussionView } from '../DiscussionView' export const PROJECT_COMMENTS = gql(` query ProjectComments($projectName: String!) { diff --git a/web/src/pages/comments/entities/SampleCommentsView.tsx b/web/src/pages/comments/entities/SampleCommentsView.tsx index 43534fb97..7659bd73c 100644 --- a/web/src/pages/comments/entities/SampleCommentsView.tsx +++ b/web/src/pages/comments/entities/SampleCommentsView.tsx @@ -1,7 +1,7 @@ import { useQuery } from '@apollo/client' import { gql } from '../../../__generated__' -import { DiscussionView } from '../DiscussionView' import { useNewComment } from '../data/commentMutations' +import { DiscussionView } from '../DiscussionView' export const SAMPLE_COMMENTS = gql(` query SampleComments($sampleId: String!) { diff --git a/web/src/pages/comments/entities/SequencingGroupCommentsView.tsx b/web/src/pages/comments/entities/SequencingGroupCommentsView.tsx index 000cb98b9..1e0d35e9a 100644 --- a/web/src/pages/comments/entities/SequencingGroupCommentsView.tsx +++ b/web/src/pages/comments/entities/SequencingGroupCommentsView.tsx @@ -1,7 +1,7 @@ import { useQuery } from '@apollo/client' import { gql } from '../../../__generated__' -import { DiscussionView } from '../DiscussionView' import { useNewComment } from '../data/commentMutations' +import { DiscussionView } from '../DiscussionView' export const SEQUENCING_GROUP_COMMENTS = gql(` query SequencingGroupComments($sequencingGroupId: String!) { diff --git a/web/src/pages/project/ValueFilter.tsx b/web/src/pages/project/ValueFilter.tsx index c213164aa..3f8302c8f 100644 --- a/web/src/pages/project/ValueFilter.tsx +++ b/web/src/pages/project/ValueFilter.tsx @@ -85,19 +85,22 @@ export const ValueFilter: React.FC = ({ // So check if the filterKey starts with 'meta.' to determine if it is a meta key, and // then check the [category].meta object for the value - if (!field.filter_key) return <> - - /* eslint-disable react-hooks/rules-of-hooks*/ const [_defaultFilterType, setDefaultFilterType] = React.useState< ProjectParticipantGridFilterType | undefined >() - /* eslint-enable react-hooks/rules-of-hooks*/ + + let optionsToCheck = props?.filterValues?.[category] || {} + const name = (field.filter_key ?? '').replace(/^meta\./, '') + + // @ts-ignore + const _value = optionsToCheck?.[name]?.[operator] + const [_tempValue, setTempValue] = React.useState(_value ?? '') + const tempValue = _tempValue ?? _value + + if (!field.filter_key) return <> const isMeta = field.filter_key?.startsWith('meta.') // set name to the filterKey without the .meta prefix - const name = field.filter_key.replace(/^meta\./, '') - - let optionsToCheck = props?.filterValues?.[category] || {} if (isMeta) { // get the meta bit from the filterValues @@ -143,14 +146,6 @@ export const ValueFilter: React.FC = ({ operator = getOperatorFromFilterType(queryType) } - // @ts-ignore - const _value = optionsToCheck?.[name]?.[operator] - - /* eslint-disable react-hooks/rules-of-hooks*/ - const [_tempValue, setTempValue] = React.useState(_value ?? '') - /* eslint-enable react-hooks/rules-of-hooks*/ - const tempValue = _tempValue ?? _value - const updateQueryType = (newFilterType: ProjectParticipantGridFilterType) => { setDefaultFilterType(newFilterType) const newOperator = getOperatorFromFilterType(newFilterType) diff --git a/web/src/shared/components/Header/NavBar.css b/web/src/shared/components/Header/NavBar.css index 3dd6f94e4..b26be7c5d 100644 --- a/web/src/shared/components/Header/NavBar.css +++ b/web/src/shared/components/Header/NavBar.css @@ -80,7 +80,6 @@ border-radius: 4px !important; padding: 10px !important; margin: 1px !important; - border-radius: 4px !important; font-weight: 400 !important; font-size: 1rem !important; font-family: Lato, 'Helvetica Neue', Arial, Helvetica, sans-serif !important; diff --git a/web/src/shared/components/SeqPanel.tsx b/web/src/shared/components/SeqPanel.tsx index 5de78dca9..983b425e1 100644 --- a/web/src/shared/components/SeqPanel.tsx +++ b/web/src/shared/components/SeqPanel.tsx @@ -5,8 +5,8 @@ import { Accordion } from 'semantic-ui-react' import { GraphQlSequencingGroup } from '../../__generated__/graphql' import iconStyle from '../iconStyle' import { DeepPartial } from '../utilities/deepPartial' -import SequencingGroupInfo from './SequencingGroupInfo' import SequencingGroupLink from './links/SequencingGroupLink' +import SequencingGroupInfo from './SequencingGroupInfo' const SeqPanel: React.FunctionComponent<{ sequencingGroups: DeepPartial[] diff --git a/web/src/shared/components/pedigree/TangledTree.tsx b/web/src/shared/components/pedigree/TangledTree.tsx index 3c61219b5..da1074055 100644 --- a/web/src/shared/components/pedigree/TangledTree.tsx +++ b/web/src/shared/components/pedigree/TangledTree.tsx @@ -507,7 +507,7 @@ export const PersonNode: React.FC = ({ > - val ? `$${val.toFixed(dp).replace(/\d(?=(\d{3})+\.)/g, '$&,')}` : '' +const formatMoney = (val: number | undefined | null, dp: number = 2): string => + val ? `$${val.toFixed(dp).replace(/\B(?=(\d{3})+(?!\d))/g, ',')}` : '' export default formatMoney From f2de605d7454a62d1580c6b21b008294fc2de3f2 Mon Sep 17 00:00:00 2001 From: John Marshall Date: Wed, 9 Oct 2024 17:07:02 +1300 Subject: [PATCH 2/8] Implement multiple external IDs for families (#896) * Create family_external_id database table * Replace family.external_id with external_ids etc * In api/graphql/schema.py, add external_ids alongside external_id * In ParticipantGridRow.tsx, reuse prepareExternalIds() to handle family multiple extids * Complete rewrite of insert_or_update_multiple_families() The previous INSERT ... ON DUPLICATE KEY UPDATE code inserts a new family or, if the same project+external_id entry already exists, updates description and coded_phenotype -- as external_id is part of the key used to locate the record, it can't be updated. With external_id moving to a separate table, we need to write this logic out explicitly. We search by any external id, but only insert the primary external id for new records. (At present, this functionality is used only by FamilyLayer.import_families(), which parses only the primary external id.) * Use transactions in create_family() and update_family() * FamilyTable.get_id_map_by_internal_ids() returns only primary extids Most users of this function want a single external id (per internal id) that they can use to populate a pedigree or for use with seqr. (The call in ParticipantLayer.generic_individual_metadata_importer() has complex wants but is mostly similar.) Hence, at least for now, it is simplest to keep the 1:1 map return type and return only the primary external ids. --- api/graphql/schema.py | 4 +- api/routes/family.py | 4 +- db/project.xml | 76 ++++++ db/python/connect.py | 1 + db/python/layers/family.py | 13 +- db/python/layers/participant.py | 2 +- db/python/layers/seqr.py | 5 +- db/python/layers/web.py | 2 +- db/python/tables/family.py | 271 +++++++++++++------ db/python/tables/project.py | 1 + models/models/family.py | 27 +- test/test_comment.py | 8 +- test/test_external_id.py | 150 +++++++++- test/test_search.py | 4 +- test/test_update_participant_family.py | 6 +- test/test_web.py | 10 +- web/src/pages/project/ParticipantGridRow.tsx | 2 +- 17 files changed, 470 insertions(+), 116 deletions(-) diff --git a/api/graphql/schema.py b/api/graphql/schema.py index b424d8575..f849e3932 100644 --- a/api/graphql/schema.py +++ b/api/graphql/schema.py @@ -728,6 +728,7 @@ class GraphQLFamily: id: int external_id: str + external_ids: strawberry.scalars.JSON description: str | None coded_phenotype: str | None @@ -739,7 +740,8 @@ class GraphQLFamily: def from_internal(internal: FamilyInternal) -> 'GraphQLFamily': return GraphQLFamily( id=internal.id, - external_id=internal.external_id, + external_id=internal.external_ids[PRIMARY_EXTERNAL_ORG], + external_ids=internal.external_ids or {}, description=internal.description, coded_phenotype=internal.coded_phenotype, project_id=internal.project, diff --git a/api/routes/family.py b/api/routes/family.py index 6a5b966f1..c20cce3ce 100644 --- a/api/routes/family.py +++ b/api/routes/family.py @@ -30,7 +30,7 @@ class FamilyUpdateModel(BaseModel): """Model for updating a family""" id: int - external_id: str | None = None + external_ids: dict[str, str] | None = None description: str | None = None coded_phenotype: str | None = None @@ -171,7 +171,7 @@ async def update_family( return { 'success': await family_layer.update_family( id_=family.id, - external_id=family.external_id, + external_ids=family.external_ids, description=family.description, coded_phenotype=family.coded_phenotype, ) diff --git a/db/project.xml b/db/project.xml index a28dbb947..4b4e9a875 100644 --- a/db/project.xml +++ b/db/project.xml @@ -1789,4 +1789,80 @@ ALTER TABLE `analysis_outputs` ADD SYSTEM VERSIONING; + + + + + + + + + + + + + + + + + + + + + + + + + ALTER TABLE family_external_id ADD SYSTEM VERSIONING; + + + INSERT INTO audit_log (author, on_behalf_of, ar_guid, comment, auth_project, meta) + VALUES ('liquibase', NULL, NULL, 'family external_id migration', NULL, NULL) + RETURNING @audit_log_id := id; + + INSERT INTO family_external_id (project, family_id, name, external_id, audit_log_id) + SELECT project, id, '', external_id, @audit_log_id + FROM family; + + + + + SET @@system_versioning_alter_history = 1; + + + UPDATE family SET external_id = NULL + + diff --git a/db/python/connect.py b/db/python/connect.py index fd9e97e08..05a1dd5b7 100644 --- a/db/python/connect.py +++ b/db/python/connect.py @@ -42,6 +42,7 @@ 'sample_external_id', 'sequencing_group_external_id', 'family', + 'family_external_id', 'family_participant', 'participant_phenotypes', 'group_member', diff --git a/db/python/layers/family.py b/db/python/layers/family.py index 130915243..1d296ff4f 100644 --- a/db/python/layers/family.py +++ b/db/python/layers/family.py @@ -29,11 +29,14 @@ def __init__(self, connection: Connection): self.fptable = FamilyParticipantTable(self.connection) async def create_family( - self, external_id: str, description: str = None, coded_phenotype: str = None + self, + external_ids: dict[str, str], + description: str | None = None, + coded_phenotype: str | None = None, ): """Create a family""" return await self.ftable.create_family( - external_id=external_id, + external_ids=external_ids, description=description, coded_phenotype=coded_phenotype, ) @@ -127,7 +130,7 @@ async def get_families_by_participants( async def update_family( self, id_: int, - external_id: str = None, + external_ids: dict[str, str] | None = None, description: str = None, coded_phenotype: str = None, ) -> bool: @@ -140,7 +143,7 @@ async def update_family( return await self.ftable.update_family( id_=id_, - external_id=external_id, + external_ids=external_ids, description=description, coded_phenotype=coded_phenotype, ) @@ -303,7 +306,7 @@ async def import_pedigree( for external_family_id in missing_external_family_ids: internal_family_id = await self.ftable.create_family( - external_id=external_family_id, + external_ids={PRIMARY_EXTERNAL_ORG: external_family_id}, description=None, coded_phenotype=None, ) diff --git a/db/python/layers/participant.py b/db/python/layers/participant.py index 6456cd9a7..f29171ab9 100644 --- a/db/python/layers/participant.py +++ b/db/python/layers/participant.py @@ -532,7 +532,7 @@ async def generic_individual_metadata_importer( # they might not be missing for external_family_id in missing_family_ids: new_pid = await ftable.create_family( - external_id=external_family_id, + external_ids={PRIMARY_EXTERNAL_ORG: external_family_id}, description=None, coded_phenotype=None, ) diff --git a/db/python/layers/seqr.py b/db/python/layers/seqr.py index ae2c0cd7b..242fd5f51 100644 --- a/db/python/layers/seqr.py +++ b/db/python/layers/seqr.py @@ -35,6 +35,7 @@ from db.python.tables.project import Project from models.enums import AnalysisStatus from models.enums.web import SeqrDatasetType +from models.models import PRIMARY_EXTERNAL_ORG # literally the most temporary thing ever, but for complete # automation need to have sample inclusion / exclusion @@ -282,8 +283,8 @@ async def sync_families( return ['No families to synchronise'] family_data = [ { - 'familyId': fam.external_id, - 'displayName': fam.external_id, + 'familyId': fam.external_ids[PRIMARY_EXTERNAL_ORG], + 'displayName': fam.external_ids[PRIMARY_EXTERNAL_ORG], 'description': fam.description, 'codedPhenotype': fam.coded_phenotype, } diff --git a/db/python/layers/web.py b/db/python/layers/web.py index e217b37a0..beba36330 100644 --- a/db/python/layers/web.py +++ b/db/python/layers/web.py @@ -343,7 +343,7 @@ def assemble_nested_participants_from( families = [] for family in families_by_pid.get(participant.id, []): families.append( - FamilySimpleInternal(id=family.id, external_id=family.external_id) + FamilySimpleInternal(id=family.id, external_ids=family.external_ids) ) nested_participant = NestedParticipantInternal( id=participant.id, diff --git a/db/python/tables/family.py b/db/python/tables/family.py index 6e77a249f..a8dab8fee 100644 --- a/db/python/tables/family.py +++ b/db/python/tables/family.py @@ -5,8 +5,7 @@ from db.python.filters import GenericFilter, GenericFilterModel from db.python.tables.base import DbBase from db.python.utils import NotFoundError, escape_like_term -from models.models.family import FamilyInternal -from models.models.project import ProjectId +from models.models import PRIMARY_EXTERNAL_ORG, FamilyInternal, ProjectId @dataclasses.dataclass @@ -54,15 +53,21 @@ async def query( ) -> tuple[set[ProjectId], list[FamilyInternal]]: """Get all families for some project""" _query = """ - SELECT f.id, f.external_id, f.description, f.coded_phenotype, f.project + SELECT f.id, JSON_OBJECTAGG(feid.name, feid.external_id) AS external_ids, + f.description, f.coded_phenotype, f.project FROM family f + INNER JOIN family_external_id feid ON f.id = feid.family_id """ if not filter_.project and not filter_.id: raise ValueError('Project or ID filter is required for family queries') has_participant_join = False - field_overrides = {'id': 'f.id', 'external_id': 'f.external_id'} + field_overrides = { + 'id': 'f.id', + 'external_id': 'feid.external_id', + 'project': 'f.project', + } has_participant_join = False if filter_.participant_id: @@ -87,6 +92,10 @@ async def query( if wheres: _query += f'WHERE {wheres}' + _query += """ + GROUP BY f.id, f.description, f.coded_phenotype, f.project + """ + rows = await self.connection.fetch_all(_query, values) seen = set() families = [] @@ -107,10 +116,13 @@ async def get_families_by_participants( return set(), {} _query = """ - SELECT id, external_id, description, coded_phenotype, project, fp.participant_id - FROM family - INNER JOIN family_participant fp ON family.id = fp.family_id + SELECT f.id, JSON_OBJECTAGG(feid.name, feid.external_id) AS external_ids, + f.description, f.coded_phenotype, f.project, fp.participant_id + FROM family f + INNER JOIN family_external_id feid ON f.id = feid.family_id + INNER JOIN family_participant fp ON f.id = fp.family_id WHERE fp.participant_id in :pids + GROUP BY f.id, f.description, f.coded_phenotype, f.project, fp.participant_id """ ret_map = defaultdict(list) projects: set[ProjectId] = set() @@ -129,8 +141,8 @@ async def search( Search by some term, return [ProjectId, FamilyId, ExternalId] """ _query = """ - SELECT project, id, external_id - FROM family + SELECT project, family_id, external_id + FROM family_external_id WHERE project in :project_ids AND external_id LIKE :search_pattern LIMIT :limit """ @@ -142,7 +154,7 @@ async def search( 'limit': limit, }, ) - return [(r['project'], r['id'], r['external_id']) for r in rows] + return [(r['project'], r['family_id'], r['external_id']) for r in rows] async def get_family_external_ids_by_participant_ids( self, participant_ids @@ -152,9 +164,9 @@ async def get_family_external_ids_by_participant_ids( return {} _query = """ - SELECT f.external_id, fp.participant_id + SELECT feid.external_id, fp.participant_id FROM family_participant fp - INNER JOIN family f ON fp.family_id = f.id + INNER JOIN family_external_id feid ON fp.family_id = feid.family_id WHERE fp.participant_id in :pids """ rows = await self.connection.fetch_all(_query, {'pids': participant_ids}) @@ -167,31 +179,78 @@ async def get_family_external_ids_by_participant_ids( async def update_family( self, id_: int, - external_id: str | None = None, + external_ids: dict[str, str | None] | None = None, description: str | None = None, coded_phenotype: str | None = None, ) -> bool: """Update values for a family""" - values: Dict[str, Any] = {'audit_log_id': await self.audit_log_id()} - if external_id: - values['external_id'] = external_id + audit_log_id = await self.audit_log_id() + + values: Dict[str, Any] = {'audit_log_id': audit_log_id} if description: values['description'] = description if coded_phenotype: values['coded_phenotype'] = coded_phenotype - setters = ', '.join(f'{field} = :{field}' for field in values) - _query = f""" -UPDATE family -SET {setters} -WHERE id = :id - """ - await self.connection.execute(_query, {**values, 'id': id_}) + async with self.connection.transaction(): + if external_ids is None: + external_ids = {} + + to_delete = [k.lower() for k, v in external_ids.items() if v is None] + to_update = {k.lower(): v for k, v in external_ids.items() if v is not None} + + if to_delete: + await self.connection.execute( + """ + -- Set audit_log_id to this transaction before deleting the rows + UPDATE family_external_id + SET audit_log_id = :audit_log_id + WHERE family_id = :id AND name IN :names; + + DELETE FROM family_external_id + WHERE family_id = :id AND name in :names + """, + {'id': id_, 'names': to_delete, 'audit_log_id': audit_log_id}, + ) + + if to_update: + project = await self.connection.fetch_val( + 'SELECT project FROM family WHERE id = :id', + {'id': id_}, + ) + + _update_query = """ + INSERT INTO family_external_id (project, family_id, name, external_id, audit_log_id) + VALUES (:project, :id, :name, :external_id, :audit_log_id) + ON DUPLICATE KEY UPDATE external_id = :external_id, audit_log_id = :audit_log_id + """ + _update_values = [ + { + 'project': project, + 'id': id_, + 'name': name, + 'external_id': eid, + 'audit_log_id': audit_log_id, + } + for name, eid in to_update.items() + ] + await self.connection.execute_many(_update_query, _update_values) + + setters = ', '.join(f'{field} = :{field}' for field in values) + await self.connection.execute( + f""" + UPDATE family + SET {setters} + WHERE id = :id + """, + {**values, 'id': id_}, + ) + return True async def create_family( self, - external_id: str, + external_ids: dict[str, str], description: Optional[str], coded_phenotype: Optional[str], project: ProjectId | None = None, @@ -199,25 +258,41 @@ async def create_family( """ Create a new sample, and add it to database """ - updater = { - 'external_id': external_id, - 'description': description, - 'coded_phenotype': coded_phenotype, - 'audit_log_id': await self.audit_log_id(), - 'project': project or self.project_id, - } - keys = list(updater.keys()) - str_keys = ', '.join(keys) - placeholder_keys = ', '.join(f':{k}' for k in keys) - _query = f""" -INSERT INTO family - ({str_keys}) -VALUES - ({placeholder_keys}) -RETURNING id - """ + audit_log_id = await self.audit_log_id() + + async with self.connection.transaction(): + new_id = await self.connection.fetch_val( + """ + INSERT INTO family (project, description, coded_phenotype, audit_log_id) + VALUES (:project, :description, :coded_phenotype, :audit_log_id) + RETURNING id + """, + { + 'project': project or self.project_id, + 'description': description, + 'coded_phenotype': coded_phenotype, + 'audit_log_id': audit_log_id, + }, + ) + + await self.connection.execute_many( + """ + INSERT INTO family_external_id (project, family_id, name, external_id, audit_log_id) + VALUES (:project, :family_id, :name, :external_id, :audit_log_id) + """, + [ + { + 'project': project or self.project_id, + 'family_id': new_id, + 'name': name, + 'external_id': eid, + 'audit_log_id': audit_log_id, + } + for name, eid in external_ids.items() + ], + ) - return await self.connection.fetch_val(_query, updater) + return new_id async def insert_or_update_multiple_families( self, @@ -226,35 +301,65 @@ async def insert_or_update_multiple_families( coded_phenotypes: List[Optional[str]], project: ProjectId | None = None, ): - """Upsert""" - updater = [ - { - 'external_id': eid, - 'description': descr, - 'coded_phenotype': cph, - 'audit_log_id': await self.audit_log_id(), - 'project': project or self.project_id, - } - for eid, descr, cph in zip(external_ids, descriptions, coded_phenotypes) - ] - - keys = list(updater[0].keys()) - str_keys = ', '.join(keys) - placeholder_keys = ', '.join(f':{k}' for k in keys) - - update_only_keys = [k for k in keys if k not in ('external_id', 'project')] - str_uo_placeholder_keys = ', '.join(f'{k} = :{k}' for k in update_only_keys) - - _query = f"""\ -INSERT INTO family - ({str_keys}) -VALUES - ({placeholder_keys}) -ON DUPLICATE KEY UPDATE - {str_uo_placeholder_keys} """ + Upsert several families. + At present, this function only supports upserting the primary external id. + """ + audit_log_id = await self.audit_log_id() + + for eid, descr, cph in zip(external_ids, descriptions, coded_phenotypes): + existing_id = await self.connection.fetch_val( + """ + SELECT family_id FROM family_external_id + WHERE project = :project AND external_id = :external_id + """, + {'project': project or self.project_id, 'external_id': eid}, + ) + + if existing_id is None: + new_id = await self.connection.fetch_val( + """ + INSERT INTO family (project, description, coded_phenotype, audit_log_id) + VALUES (:project, :description, :coded_phenotype, :audit_log_id) + RETURNING id + """, + { + 'project': project or self.project_id, + 'description': descr, + 'coded_phenotype': cph, + 'audit_log_id': audit_log_id, + }, + ) + await self.connection.execute( + """ + INSERT INTO family_external_id (project, family_id, name, external_id, audit_log_id) + VALUES (:project, :family_id, :name, :external_id, :audit_log_id) + """, + { + 'project': project or self.project_id, + 'family_id': new_id, + 'name': PRIMARY_EXTERNAL_ORG, + 'external_id': eid, + 'audit_log_id': audit_log_id, + }, + ) + + else: + await self.connection.execute( + """ + UPDATE family + SET description = :description, coded_phenotype = :coded_phenotype, + audit_log_id = :audit_log_id + WHERE id = :id + """, + { + 'id': existing_id, + 'description': descr, + 'coded_phenotype': cph, + 'audit_log_id': audit_log_id, + }, + ) - await self.connection.execute_many(_query, updater) return True async def get_id_map_by_external_ids( @@ -265,9 +370,12 @@ async def get_id_map_by_external_ids( if not family_ids: return {} - _query = 'SELECT external_id, id FROM family WHERE external_id in :external_ids AND project = :project' results = await self.connection.fetch_all( - _query, {'external_ids': family_ids, 'project': project or self.project_id} + """ + SELECT external_id, family_id AS id FROM family_external_id + WHERE external_id in :external_ids AND project = :project + """, + {'external_ids': family_ids, 'project': project or self.project_id}, ) id_map = {r['external_id']: r['id'] for r in results} @@ -288,19 +396,26 @@ async def get_id_map_by_external_ids( async def get_id_map_by_internal_ids( self, family_ids: List[int], allow_missing=False ): - """Get map of {external_id: internal_id} for a family""" + """Get map of {internal_id: primary_external_id} for a family""" if len(family_ids) == 0: return {} - _query = 'SELECT id, external_id FROM family WHERE id in :ids' - results = await self.connection.fetch_all(_query, {'ids': family_ids}) - id_map = {r['id']: r['external_id'] for r in results} + + results = await self.connection.fetch_all( + """ + SELECT family_id, external_id + FROM family_external_id + WHERE family_id in :ids AND name = :PRIMARY_EXTERNAL_ORG + """, + {'ids': family_ids, 'PRIMARY_EXTERNAL_ORG': PRIMARY_EXTERNAL_ORG}, + ) + id_map = {r['family_id']: r['external_id'] for r in results} if not allow_missing and len(id_map) != len(family_ids): - provided_external_ids = set(family_ids) + provided_internal_ids = set(family_ids) # do the check again, but use the set this time # (in case we're provided a list with duplicates) - if len(id_map) != len(provided_external_ids): + if len(id_map) != len(provided_internal_ids): # we have families missing from the map, so we'll 404 the whole thing - missing_family_ids = provided_external_ids - set(id_map.keys()) + missing_family_ids = provided_internal_ids - set(id_map.keys()) raise NotFoundError( f"Couldn't find families with internal IDS: {', '.join(str(m) for m in missing_family_ids)}" diff --git a/db/python/tables/project.py b/db/python/tables/project.py index 6efd859d9..cd064833d 100644 --- a/db/python/tables/project.py +++ b/db/python/tables/project.py @@ -242,6 +242,7 @@ async def delete_project_data(self, project_id: int, delete_project: bool) -> bo DELETE FROM family_participant WHERE family_id IN ( SELECT id FROM family where project = :project ); +DELETE FROM family_external_id WHERE project = :project; DELETE FROM family WHERE project = :project; DELETE FROM sequencing_group_external_id WHERE project = :project; DELETE FROM sample_external_id WHERE project = :project; diff --git a/models/models/family.py b/models/models/family.py index 6113f27d8..b55d812d9 100644 --- a/models/models/family.py +++ b/models/models/family.py @@ -1,27 +1,27 @@ import logging -from pydantic import BaseModel +from models.base import SMBase, parse_sql_dict -class FamilySimpleInternal(BaseModel): +class FamilySimpleInternal(SMBase): """Simple family model for internal use""" id: int - external_id: str + external_ids: dict[str, str] def to_external(self): """Convert to external model""" return FamilySimple( id=self.id, - external_id=self.external_id, + external_ids=self.external_ids, ) -class FamilyInternal(BaseModel): +class FamilyInternal(SMBase): """Family model""" id: int - external_id: str + external_ids: dict[str, str] project: int description: str | None = None coded_phenotype: str | None = None @@ -29,31 +29,32 @@ class FamilyInternal(BaseModel): @staticmethod def from_db(d): """From DB fields""" - return FamilyInternal(**d) + external_ids = parse_sql_dict(d.pop('external_ids', {})) + return FamilyInternal(**d, external_ids=external_ids) def to_external(self): """Convert to external model""" return Family( id=self.id, - external_id=self.external_id, + external_ids=self.external_ids, project=self.project, description=self.description, coded_phenotype=self.coded_phenotype, ) -class FamilySimple(BaseModel): +class FamilySimple(SMBase): """Simple family model, mostly for web access""" id: int - external_id: str + external_ids: dict[str, str] -class Family(BaseModel): +class Family(SMBase): """Family model""" id: int | None - external_id: str + external_ids: dict[str, str] project: int description: str | None = None coded_phenotype: str | None = None @@ -62,7 +63,7 @@ def to_internal(self): """Convert to internal model""" return FamilyInternal( id=self.id, - external_id=self.external_id, + external_ids=self.external_ids, project=self.project, description=self.description, coded_phenotype=self.coded_phenotype, diff --git a/test/test_comment.py b/test/test_comment.py index ce0530940..4c350f9b2 100644 --- a/test/test_comment.py +++ b/test/test_comment.py @@ -626,7 +626,9 @@ async def test_add_comment_to_participant(self): async def test_add_comment_to_family(self): """Test adding a comment to an family""" - family = await self.flayer.create_family(external_id='f_external_id') + family = await self.flayer.create_family( + external_ids={PRIMARY_EXTERNAL_ORG: 'f_external_id'}, + ) comment_text = 'Family Test Comment 1234' created_comment = await self.add_comment_to_family(family, comment_text) @@ -823,7 +825,9 @@ async def test_deleting_and_restoring_comment(self): async def test_sample_discussion_related_comments(self): """Test getting related comments""" - family_id = await self.flayer.create_family(external_id='f_external_id') + family_id = await self.flayer.create_family( + external_ids={PRIMARY_EXTERNAL_ORG: 'f_external_id'}, + ) # This will create participant, sample, assay, sequencing group participant = await self.player.upsert_participant(get_participant_to_insert()) diff --git a/test/test_external_id.py b/test/test_external_id.py index c62468ba2..b248c393f 100644 --- a/test/test_external_id.py +++ b/test/test_external_id.py @@ -2,7 +2,9 @@ from pymysql.err import IntegrityError +from db.python.filters import GenericFilter from db.python.layers import FamilyLayer, ParticipantLayer, SampleLayer +from db.python.tables.family import FamilyFilter, FamilyTable from db.python.utils import NotFoundError from models.models import ( PRIMARY_EXTERNAL_ORG, @@ -170,7 +172,7 @@ async def test_fill_in_missing(self): async def test_get_by_families(self): """Exercise get_participants_by_families() method""" flayer = FamilyLayer(self.connection) - fid = await flayer.create_family(external_id='Jones') + fid = await flayer.create_family(external_ids={'org': 'Jones'}) child = await self.player.upsert_participant( ParticipantUpsertInternal( @@ -193,6 +195,35 @@ async def test_get_by_families(self): result[fid][0].external_ids, {PRIMARY_EXTERNAL_ORG: 'P20', 'd': 'D20'} ) + @run_as_sync + async def test_get_families_by_participants(self): + """Exercise FamilyLayer's get_families_by_participants() method""" + flayer = FamilyLayer(self.connection) + fid = await flayer.create_family( + external_ids={PRIMARY_EXTERNAL_ORG: 'Smith'}, + description='Blacksmiths', + coded_phenotype='burnt', + ) + + child = await self.player.upsert_participant( + ParticipantUpsertInternal( + external_ids={PRIMARY_EXTERNAL_ORG: 'P20', 'd': 'D20'} + ), + ) + + await self.player.add_participant_to_family( + family_id=fid, + participant_id=child.id, + paternal_id=self.p1.id, + maternal_id=self.p2.id, + affected=2, + ) + + result = await flayer.get_families_by_participants([child.id, self.p1.id]) + self.assertEqual(len(result), 1) + self.assertEqual(len(result[child.id]), 1) + self.assertEqual(result[child.id][0].description, 'Blacksmiths') + @run_as_sync async def test_update_many(self): """Exercise update_many_participant_external_ids() method""" @@ -423,3 +454,120 @@ async def test_get_history(self): self.assertDictEqual(result[1].meta, {'foo': 'bar'}) self.assertDictEqual(result[2].meta, {'foo': 'bar', 'fruit': 'banana'}) self.assertDictEqual(result[2].meta, sample.meta) + + +class TestFamily(DbIsolatedTest): + """Test family external ids""" + + @run_as_sync + async def setUp(self): + super().setUp() + self.flayer = FamilyLayer(self.connection) + + @run_as_sync + async def test_create_update(self): + """Exercise create_family() and update_family() methods""" + family_id = await self.flayer.create_family( + external_ids={PRIMARY_EXTERNAL_ORG: 'Smith'}, + description='Blacksmiths', + coded_phenotype='burnt', + ) + + family = await self.flayer.get_family_by_internal_id(family_id) + self.assertDictEqual(family.external_ids, {PRIMARY_EXTERNAL_ORG: 'Smith'}) + self.assertEqual(family.description, 'Blacksmiths') + self.assertEqual(family.coded_phenotype, 'burnt') + + await self.flayer.update_family(family_id, external_ids={'foo': 'bar'}) + family = await self.flayer.get_family_by_internal_id(family_id) + self.assertEqual(family.external_ids['foo'], 'bar') + + await self.flayer.update_family(family_id, external_ids={'foo': 'baz'}) + family = await self.flayer.get_family_by_internal_id(family_id) + self.assertEqual(family.external_ids['foo'], 'baz') + + await self.flayer.update_family(family_id, external_ids={'foo': None}) + family = await self.flayer.get_family_by_internal_id(family_id) + self.assertDictEqual(family.external_ids, {PRIMARY_EXTERNAL_ORG: 'Smith'}) + + await self.flayer.update_family(family_id, description='Goldsmiths') + family = await self.flayer.get_family_by_internal_id(family_id) + self.assertEqual(family.description, 'Goldsmiths') + self.assertEqual(family.coded_phenotype, 'burnt') + + await self.flayer.update_family(family_id, coded_phenotype='gilt') + family = await self.flayer.get_family_by_internal_id(family_id) + self.assertEqual(family.description, 'Goldsmiths') + self.assertEqual(family.coded_phenotype, 'gilt') + + @run_as_sync + async def test_bad_query(self): + """Exercise invalid query() usage""" + with self.assertRaises(ValueError): + await self.flayer.query(FamilyFilter()) + + @run_as_sync + async def test_none_by_participants(self): + """Exercise get_families_by_participants() method""" + result = await self.flayer.get_families_by_participants([]) + self.assertDictEqual(result, {}) + + @run_as_sync + async def test_import_families(self): + """Exercise import_families() method""" + await self.flayer.import_families( + ['familyid', 'description', 'phenotype'], + [ + ['Smith', 'Blacksmiths', 'burnt'], + ['Jones', 'From Wales', 'sings well'], + ['Taylor', 'Post Norman', 'sews'], + ], + ) + + result = await self.flayer.query( + FamilyFilter(project=GenericFilter(eq=self.project_id)) + ) + self.assertEqual(len(result), 3) + family = {f.external_ids[PRIMARY_EXTERNAL_ORG]: f for f in result} + self.assertEqual(family['Smith'].description, 'Blacksmiths') + self.assertEqual(family['Smith'].coded_phenotype, 'burnt') + self.assertEqual(family['Jones'].description, 'From Wales') + self.assertEqual(family['Jones'].coded_phenotype, 'sings well') + self.assertEqual(family['Taylor'].description, 'Post Norman') + self.assertEqual(family['Taylor'].coded_phenotype, 'sews') + + await self.flayer.import_families( + ['familyid', 'description', 'phenotype'], + [ + ['Smith', 'Goldsmiths actually', 'gilt'], + ['Brown', 'From Jamaica', 'brunette'], + ], + ) + + result = await self.flayer.query( + FamilyFilter(project=GenericFilter(eq=self.project_id)) + ) + self.assertEqual(len(result), 4) + family = {f.external_ids[PRIMARY_EXTERNAL_ORG]: f for f in result} + self.assertEqual(family['Smith'].description, 'Goldsmiths actually') + self.assertEqual(family['Smith'].coded_phenotype, 'gilt') + self.assertEqual(family['Brown'].description, 'From Jamaica') + self.assertEqual(family['Brown'].coded_phenotype, 'brunette') + self.assertEqual(family['Jones'].description, 'From Wales') + self.assertEqual(family['Jones'].coded_phenotype, 'sings well') + self.assertEqual(family['Taylor'].description, 'Post Norman') + self.assertEqual(family['Taylor'].coded_phenotype, 'sews') + + @run_as_sync + async def test_direct_get_id_map(self): + """Exercise the table's get_id_map_by_internal_ids() method""" + ftable = FamilyTable(self.connection) + + result = await ftable.get_id_map_by_internal_ids([]) + self.assertDictEqual(result, {}) + + result = await ftable.get_id_map_by_internal_ids([42], allow_missing=True) + self.assertDictEqual(result, {}) + + with self.assertRaises(NotFoundError): + _ = await ftable.get_id_map_by_internal_ids([42]) diff --git a/test/test_search.py b/test/test_search.py index 0f14a776a..d26fa06f6 100644 --- a/test/test_search.py +++ b/test/test_search.py @@ -188,7 +188,7 @@ async def test_search_family(self): Search family by External ID should only return one result """ - f_id = await self.flayer.create_family(external_id='FAMXX01') + f_id = await self.flayer.create_family(external_ids={'forg': 'FAMXX01'}) results = await self.schlay.search( query='FAMXX01', project_ids=[self.project_id] ) @@ -208,7 +208,7 @@ async def test_search_mixed(self): p = await self.player.upsert_participant( ParticipantUpsertInternal(external_ids={PRIMARY_EXTERNAL_ORG: 'X:PART01'}) ) - f_id = await self.flayer.create_family(external_id='X:FAM01') + f_id = await self.flayer.create_family(external_ids={'famxorg': 'X:FAM01'}) await fptable.create_rows( [ PedRowInternal( diff --git a/test/test_update_participant_family.py b/test/test_update_participant_family.py index 1774fb3c7..95c41398f 100644 --- a/test/test_update_participant_family.py +++ b/test/test_update_participant_family.py @@ -16,8 +16,10 @@ async def setUp(self) -> None: fl = FamilyLayer(self.connection) - self.fid_1 = await fl.create_family(external_id='FAM01') - self.fid_2 = await fl.create_family(external_id='FAM02') + self.fid_1 = await fl.create_family(external_ids={'forg': 'FAM01'}) + self.fid_2 = await fl.create_family(external_ids={'forg': 'FAM02'}) + # Also exercise update_family() + await fl.update_family(self.fid_2, external_ids={'otherorg': 'OFAM02'}) pl = ParticipantLayer(self.connection) self.pid = ( diff --git a/test/test_web.py b/test/test_web.py index bcca16e3a..85146d63d 100644 --- a/test/test_web.py +++ b/test/test_web.py @@ -362,7 +362,7 @@ def get_test_participant_2(): fields={ MetaSearchEntityPrefix.FAMILY: [ ProjectParticipantGridField( - key='external_id', label='Family ID', is_visible=True + key='external_ids', label='Family ID', is_visible=True ) ], MetaSearchEntityPrefix.PARTICIPANT: [ @@ -756,7 +756,7 @@ def test_nested_participant_to_rows(self): id=1, external_ids={PRIMARY_EXTERNAL_ORG: 'pex1'}, meta={'pkey': 'value'}, - families=[FamilySimple(id=-2, external_id='fex1')], + families=[FamilySimple(id=-2, external_ids={'family_org': 'fex1'})], samples=[ NestedSample( id='xpgA', @@ -800,7 +800,7 @@ def test_nested_participant_to_rows(self): fields={ MetaSearchEntityPrefix.FAMILY: [ ProjectParticipantGridField( - key='external_id', label='', is_visible=True + key='external_ids', label='', is_visible=True ) ], MetaSearchEntityPrefix.PARTICIPANT: [ @@ -840,7 +840,7 @@ def test_nested_participant_to_rows(self): self.assertTupleEqual( headers, ( - 'family.external_id', + 'family.external_ids', 'participant.external_ids', 'participant.meta.pkey', 'sample.meta.skey', @@ -852,7 +852,7 @@ def test_nested_participant_to_rows(self): ), ) non_sg_keys = ( - 'fex1', + 'family_org: fex1', 'pex1', 'value', 'svalue', diff --git a/web/src/pages/project/ParticipantGridRow.tsx b/web/src/pages/project/ParticipantGridRow.tsx index 38b144ac3..70e730f0a 100644 --- a/web/src/pages/project/ParticipantGridRow.tsx +++ b/web/src/pages/project/ParticipantGridRow.tsx @@ -83,7 +83,7 @@ const FamilyCells: React.FC<{ key={`family-${participant.id}-${f.id}`} id={`${f.id ?? ''}`} > - {f.external_id} + {prepareExternalIds(f.external_ids || {})} )) : participant.families From d433e6213632e4d970d8818306288174d1724202 Mon Sep 17 00:00:00 2001 From: Dan Coates Date: Thu, 10 Oct 2024 10:54:02 +1100 Subject: [PATCH 3/8] Fix permissions error (#974) * fix error in translating old permissions to new this would have allowed read access permissions to upsert samples * fix other permission inconsistencies --- api/routes/cohort.py | 9 +++++++-- db/python/layers/family.py | 4 ++-- db/python/layers/sample.py | 4 ++-- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/api/routes/cohort.py b/api/routes/cohort.py index 4a5568926..9a5074f10 100644 --- a/api/routes/cohort.py +++ b/api/routes/cohort.py @@ -3,7 +3,12 @@ from api.utils.db import Connection, get_project_db_connection from db.python.layers.cohort import CohortLayer from models.models.cohort import CohortBody, CohortCriteria, CohortTemplate, NewCohort -from models.models.project import ProjectId, ProjectMemberRole, ReadAccessRoles +from models.models.project import ( + FullWriteAccessRoles, + ProjectId, + ProjectMemberRole, + ReadAccessRoles, +) from models.utils.cohort_template_id_format import ( cohort_template_id_format, cohort_template_id_transform_to_raw, @@ -85,7 +90,7 @@ async def create_cohort_template( if template.criteria.projects: projects_for_criteria = connection.get_and_check_access_to_projects_for_names( project_names=template.criteria.projects, - allowed_roles=ReadAccessRoles, + allowed_roles=FullWriteAccessRoles, ) criteria_project_ids = [p.id for p in projects_for_criteria if p.id] diff --git a/db/python/layers/family.py b/db/python/layers/family.py index 1d296ff4f..4438d11f8 100644 --- a/db/python/layers/family.py +++ b/db/python/layers/family.py @@ -16,7 +16,7 @@ from models.models import PRIMARY_EXTERNAL_ORG from models.models.family import FamilyInternal, PedRow, PedRowInternal from models.models.participant import ParticipantUpsertInternal -from models.models.project import ProjectId, ReadAccessRoles +from models.models.project import FullWriteAccessRoles, ProjectId, ReadAccessRoles class FamilyLayer(BaseLayer): @@ -138,7 +138,7 @@ async def update_family( project_ids = await self.ftable.get_projects_by_family_ids([id_]) self.connection.check_access_to_projects_for_ids( - project_ids, allowed_roles=ReadAccessRoles + project_ids, allowed_roles=FullWriteAccessRoles ) return await self.ftable.update_family( diff --git a/db/python/layers/sample.py b/db/python/layers/sample.py index f01cab7c0..87392e1af 100644 --- a/db/python/layers/sample.py +++ b/db/python/layers/sample.py @@ -287,7 +287,7 @@ async def upsert_samples( if sids: pjcts = await self.st.get_project_ids_for_sample_ids(sids) self.connection.check_access_to_projects_for_ids( - pjcts, allowed_roles=ReadAccessRoles + pjcts, allowed_roles=FullWriteAccessRoles ) async with with_function(): @@ -438,7 +438,7 @@ async def get_history_of_sample(self, id_: int) -> list[SampleInternal]: projects = set(r.project for r in rows) self.connection.check_access_to_projects_for_ids( - projects, allowed_roles=FullWriteAccessRoles + projects, allowed_roles=ReadAccessRoles ) return rows From 71ede81c1aa03a5cb366725628a31fb8841436bb Mon Sep 17 00:00:00 2001 From: Dan Coates Date: Mon, 28 Oct 2024 09:50:18 +1100 Subject: [PATCH 4/8] Deploy fixes (#975) * remove duplicated unittest workflow definition from deploy * switch dockerfile CMD back to shell form as exec form wasn't working --- .github/workflows/deploy.yaml | 144 +--------------------------------- .github/workflows/test.yaml | 1 + deploy/api/Dockerfile | 7 +- 3 files changed, 8 insertions(+), 144 deletions(-) diff --git a/.github/workflows/deploy.yaml b/.github/workflows/deploy.yaml index b96a05646..78229bd63 100644 --- a/.github/workflows/deploy.yaml +++ b/.github/workflows/deploy.yaml @@ -13,149 +13,7 @@ permissions: jobs: unittests: - name: Run unit tests - # Run on merge to main, where the commit name starts with "Bump version:" (for bump2version) - # if: "startsWith(github.event.head_commit.message, 'Bump version:')" - runs-on: ubuntu-latest - env: - DOCKER_BUILDKIT: 1 - BUILDKIT_PROGRESS: plain - CLOUDSDK_CORE_DISABLE_PROMPTS: 1 - # used for generating API - SM_DOCKER: samplemetadata:dev - defaults: - run: - shell: bash -eo pipefail -l {0} - steps: - - uses: actions/checkout@v4 - - - uses: actions/setup-python@v5 - with: - python-version: '3.11' - - - uses: actions/setup-java@v4 - with: - distribution: 'temurin' # See 'Supported distributions' for available options - java-version: '17' - - - name: Setup build env - run: | - set -euxo pipefail - - pip install --no-deps -r requirements-dev.txt - - # openapi-generator - wget https://repo1.maven.org/maven2/org/openapitools/openapi-generator-cli/5.3.0/openapi-generator-cli-5.3.0.jar -O openapi-generator-cli.jar - - # liquibase connector - pushd db/ - wget https://repo1.maven.org/maven2/org/mariadb/jdbc/mariadb-java-client/3.0.3/mariadb-java-client-3.0.3.jar - popd - # liquibase - VERSION=4.28.0 - curl -L https://github.com/liquibase/liquibase/releases/download/v${VERSION}/liquibase-${VERSION}.zip --output liquibase-${VERSION}.zip - unzip -o -d liquibase liquibase-${VERSION}.zip - echo "$(pwd)/liquibase" >> $GITHUB_PATH - - - name: 'build image' - run: | - docker build \ - --build-arg SM_ENVIRONMENT=local \ - --tag $SM_DOCKER \ - -f deploy/api/Dockerfile \ - . - - - name: 'build deployable API' - run: | - export OPENAPI_COMMAND="java -jar openapi-generator-cli.jar" - python regenerate_api.py - pip install . - - - name: 'Run unit tests' - id: runtests - run: | - coverage run -m pytest --doctest-modules --doctest-continue-on-failure test/ --junitxml=test-execution.xml - rc=$? - coverage xml - - echo "rc=$rc" >> $GITHUB_OUTPUT - - - name: 'Upload coverage report' - uses: codecov/codecov-action@v4 - with: - files: ./coverage.xml - token: ${{ secrets.CODECOV_TOKEN }} - - - name: 'Save coverage report as an Artifact' - uses: actions/upload-artifact@v4 - with: - name: coverage-report - path: ./coverage.xml - - - name: 'Save execution report as an Artifact' - uses: actions/upload-artifact@v4 - with: - name: execution-report - path: ./test-execution.xml - - - name: 'build web front-end' - run: | - set -eo pipefail - pushd web - # installs package-lock, not what it thinks it should be - npm ci - npm run build - rc=$? - - echo "web_rc=$rc" >> $GITHUB_OUTPUT - # eventually run web front-end tests - popd - - - name: Fail if unit tests are not passing - if: ${{ steps.runtests.outputs.rc != 0}} - uses: actions/github-script@v6 - with: - script: | - core.setFailed('Unittests failed with rc = ${{ steps.runtests.outputs.rc }}') - - - name: Fail if web build fails - if: ${{ steps.runtests.outputs.rc != 0}} - uses: actions/github-script@v6 - with: - script: | - core.setFailed('Web failed to build with rc = ${{ steps.runtests.outputs.web_rc }}') - - sonarqube: - name: SonarQube scan - runs-on: ubuntu-latest - needs: unittests - environment: production - if: github.ref == 'refs/heads/main' || github.ref == 'refs/heads/dev' - steps: - - uses: actions/checkout@v4 - with: - fetch-depth: 0 # Shallow clones should be disabled for a better relevancy of analysis - - # Download the coverage report artifact - - name: 'Download coverage and execution report' - uses: actions/download-artifact@v4 - with: - pattern: '*-report' - - # Perform the SonarQube scan - - uses: sonarsource/sonarqube-scan-action@master - env: - SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} - SONAR_HOST_URL: ${{ secrets.SONAR_HOST_URL }} - - # Optional: Fail the job if Quality Gate is red - # If you wish to fail your job when the Quality Gate is red, uncomment the - # following lines. This would typically be used to fail a deployment. - # - uses: sonarsource/sonarqube-quality-gate-action@master - # timeout-minutes: 5 - # env: - # SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} - # SONAR_HOST_URL: ${{ secrets.SONAR_HOST_URL }} + uses: './.github/workflows/test.yaml' deploy: name: Deploy diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 9651715dc..07f2295fe 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -1,6 +1,7 @@ name: Test on: push: + workflow_call: jobs: unittests: diff --git a/deploy/api/Dockerfile b/deploy/api/Dockerfile index a4e43553f..faff71763 100644 --- a/deploy/api/Dockerfile +++ b/deploy/api/Dockerfile @@ -29,4 +29,9 @@ USER appuser EXPOSE $PORT # Command to run the FastAPI app -CMD ["uvicorn", "api.main:app", "--host", "0.0.0.0", "--port", "$PORT"] +# execute in shell form rather than exec form to allow for variable substitution +# @see https://docs.docker.com/reference/dockerfile/#shell-and-exec-form +# some linting tools recomment exec form with a JSON array but the docker docs suggest the +# only way to get variable substitution with exec form is by prefixing with `sh -c` which +# is the exact same as using shell form, so ergonimcally this is much nicer +CMD uvicorn --port ${PORT} --host 0.0.0.0 api.server:app From 584c1508f290d5f0c87223c9ae85f3cb6d7ddbb5 Mon Sep 17 00:00:00 2001 From: Dan Coates Date: Mon, 28 Oct 2024 11:01:30 +1100 Subject: [PATCH 5/8] update needs reference in deploy workflow to reference top level (#977) --- .github/workflows/deploy.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/deploy.yaml b/.github/workflows/deploy.yaml index 78229bd63..99ff8534d 100644 --- a/.github/workflows/deploy.yaml +++ b/.github/workflows/deploy.yaml @@ -18,7 +18,7 @@ jobs: deploy: name: Deploy runs-on: ubuntu-latest - needs: sonarqube + needs: unittests environment: production env: DOCKER_BUILDKIT: 1 From 4305682d02f6d9ccd550d2a0e04c01ef2f0d5443 Mon Sep 17 00:00:00 2001 From: Dan Coates Date: Mon, 28 Oct 2024 15:39:59 +1100 Subject: [PATCH 6/8] Pass secrets from deploy workflow to test workflow (#978) --- .github/workflows/deploy.yaml | 4 ++++ .github/workflows/test.yaml | 7 +++++++ 2 files changed, 11 insertions(+) diff --git a/.github/workflows/deploy.yaml b/.github/workflows/deploy.yaml index 99ff8534d..ac7fa7801 100644 --- a/.github/workflows/deploy.yaml +++ b/.github/workflows/deploy.yaml @@ -14,6 +14,10 @@ permissions: jobs: unittests: uses: './.github/workflows/test.yaml' + secrets: + CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} + SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} + SONAR_HOST_URL: ${{ secrets.SONAR_HOST_URL }} deploy: name: Deploy diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 07f2295fe..e608e5a3b 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -2,6 +2,13 @@ name: Test on: push: workflow_call: + secrets: + CODECOV_TOKEN: + required: true + SONAR_TOKEN: + required: true + SONAR_HOST_URL: + required: true jobs: unittests: From 48ccd22522c194f011872138d45c4aa0de2ef369 Mon Sep 17 00:00:00 2001 From: Dan Coates Date: Tue, 29 Oct 2024 09:32:01 +1100 Subject: [PATCH 7/8] add copying of application code back into dockerfile (#979) this was accidentally removed in PR #957 --- deploy/api/Dockerfile | 3 +++ 1 file changed, 3 insertions(+) diff --git a/deploy/api/Dockerfile b/deploy/api/Dockerfile index faff71763..e92c3abe7 100644 --- a/deploy/api/Dockerfile +++ b/deploy/api/Dockerfile @@ -19,6 +19,9 @@ RUN pip install --no-cache-dir --no-deps -r requirements.txt # Copy the application code COPY api api +COPY db db/ +COPY models models/ + # Change ownership of the application directory to the non-root user RUN chown -R appuser:appuser /app/sample_metadata/ From 0d9e0bd522dcfc48f5c4184121ee853f382cf8f9 Mon Sep 17 00:00:00 2001 From: Dan Coates Date: Wed, 30 Oct 2024 08:20:26 +1100 Subject: [PATCH 8/8] =?UTF-8?q?Bump=20version:=207.4.3=20=E2=86=92=207.5.0?= =?UTF-8?q?=20(#981)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .bumpversion.cfg | 2 +- api/server.py | 2 +- deploy/python/version.txt | 2 +- setup.py | 2 +- web/package.json | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.bumpversion.cfg b/.bumpversion.cfg index 18378773b..c9fef60ea 100644 --- a/.bumpversion.cfg +++ b/.bumpversion.cfg @@ -1,5 +1,5 @@ [bumpversion] -current_version = 7.4.3 +current_version = 7.5.0 commit = True tag = False parse = (?P\d+)\.(?P\d+)\.(?P[A-z0-9-]+) diff --git a/api/server.py b/api/server.py index 595960e09..1443b5b24 100644 --- a/api/server.py +++ b/api/server.py @@ -25,7 +25,7 @@ from db.python.utils import get_logger # This tag is automatically updated by bump2version -_VERSION = '7.4.3' +_VERSION = '7.5.0' logger = get_logger() diff --git a/deploy/python/version.txt b/deploy/python/version.txt index 0f4a1d6e3..18bb4182d 100644 --- a/deploy/python/version.txt +++ b/deploy/python/version.txt @@ -1 +1 @@ -7.4.3 +7.5.0 diff --git a/setup.py b/setup.py index ad16572bb..481a0ddce 100644 --- a/setup.py +++ b/setup.py @@ -19,7 +19,7 @@ setup( name=PKG, # This tag is automatically updated by bump2version - version='7.4.3', + version='7.5.0', description='Python API for interacting with the Sample API system', long_description=readme, long_description_content_type='text/markdown', diff --git a/web/package.json b/web/package.json index 257c303a6..eed2edd2d 100644 --- a/web/package.json +++ b/web/package.json @@ -1,6 +1,6 @@ { "name": "metamist", - "version": "7.4.3", + "version": "7.5.0", "private": true, "dependencies": { "@apollo/client": "^3.11.5",