diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 00000000..42f706af --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,79 @@ +# Roadkill Mobile Cloud-Agent Guide + +## Repository at a glance + +This is Utah's Roadkill Reporter monorepo. It contains an Expo/React Native mobile app, an Express/Firebase/PostgreSQL API, a Remix public website, shared CommonJS validation/configuration, PostgreSQL schema tooling, and a Python job that publishes database data to ArcGIS Online. + +| Area | Location | Stack / entry points | +| --- | --- | --- | +| Mobile app | `src/front/` | Expo 52, React Native, React 18; `App.js`, screens in `screens/`, UI in `components/`, auth in `auth/`, API/offline/location services in `services/` | +| API | `src/back/` | Node 22, Express 5, Firebase Firestore/Storage, PostgreSQL/Knex; `server.js` starts `app.js`, endpoints are in `api/`, logic in `services/` | +| Shared code | `src/common/` | **CommonJS only**; Yup schemas in `validation/`, shared app/version/bundle config in `config.js`, database constants in `constants.js` | +| Website | `src/website/` | Remix 2, React 18, Tailwind; routes in `app/routes/`, root layout in `app/root.jsx` | +| Database | `data/` | PostgreSQL/PostGIS Docker image, Knex config/migrations, SQL schema/seed files | +| Data publisher | `src/palletjack/` | Python 3.11 `roadkill` package; `src/roadkill/main.py` is the Cloud Run Job entry point | + +The root is not an npm workspace. Every Node project owns a `package-lock.json`; use `npm`, never pnpm. When working in front, back, or website, install `src/common` too because it is a local `file:../common` dependency. Do not convert `src/common` to ESM: Expo 52 fails to build it. + +## Toolchain and setup + +Always use the version pinned by each service's `.node-version`: Node 22. CI uses Node 22. Python work in `src/palletjack` requires Python 3.11 (also its Docker base). Backend Firebase emulator tests require Java 21. Docker/Compose is required for the local PostgreSQL/PostGIS database. CI runs on Ubuntu; native iOS/Android builds additionally need EAS, platform tooling, credentials, and EAS secrets. + +For a clean Node setup, run this from the repository root before the relevant validation: + +```sh +# Select Node 22 by your version manager, then install only affected projects. +cd src/common && npm ci +cd ../front && npm ci # mobile work +cd ../back && npm ci # API work +cd ../website && npm ci # website work +cd ../../data && npm ci # migrations/scripts work +``` + +Do not use `npm install` to validate existing lockfiles. `npm ci` is the CI command. `npm test` in `src/front` is watch mode; do not use it for an agent check. `npm test` in `src/back` invokes Vitest and is also not the CI integration command. + +For local end-to-end development, copy the root, `src/front`, and `src/back` `.env.sample` files to `.env` and populate secrets; run `docker compose up` from root (requires `DEV_POSTGRES_PASSWORD`); start the API with `cd src/back && npm start` (Firebase emulators plus server) and mobile with `cd src/front && npm start` (custom Expo dev client). The emulator UI is on port 4000, API defaults to 3000, and website development uses 3002. Do not commit `.env` files or secret values. + +## Validation: run the narrow path first + +Always run the matching commands below after edits; changes under `src/common` require the consuming service checks too. + +```sh +# Shared validation (required by all Node CI workflows) +cd src/common && npm run lint + +# API CI-equivalent checks. Java 21 and Firebase emulator downloads are required. +# CI=1 prevents Vitest from entering watch mode in an interactive local terminal. +cd src/back && npm run lint && CI=1 npm run test:ci + +# Mobile CI-equivalent check. First remove ignored native build output in a reused checkout: +rm -rf src/front/android/app/build +cd src/front && npm run test:final + +# Website CI lint; build is an additional useful production check. +cd src/website && npm run lint && npm run build + +# Palletjack CI (from a Python 3.11 environment; Linux CI installs libkrb5-dev first). +cd src/palletjack && pip install '.[tests]' && ruff check --output-format=github . && pytest +``` + +The mobile `test:final` runs ESLint, Jest, then translation validation. A prior local Android build leaves ignored `android/app/build` JavaScript bundles that ESLint does not ignore and causes many `__d is not defined` failures. Always remove that generated directory before the front lint/test command; a fresh GitHub Actions checkout does not have it. The API `test:ci` runs Firebase Firestore and Storage emulators around `npm test`; use `CI=1 npm run test:ci` in an interactive local terminal so Vitest exits after one run, but do not substitute plain `npm test` when reproducing CI. `src/common/package.json` and `data/scripts/package.json` have placeholder `test` scripts that intentionally fail, so do not run them. + +Validated locally with Node 22: common lint, API lint, website lint, and website production build pass. The local environment used for this guide lacked Docker, Python 3.11, Python test tools, and Java 21, so database startup, Palletjack tests, and Firebase integration tests must be run in a matching environment rather than inferred from a newer runtime. + +For release-sensitive mobile changes: bump `runtimeVersion` in `src/front/app.config.js` whenever native code, native dependencies, or app configuration requires a new binary; OTA updates only apply to matching runtime versions. Production releases also require a `version`, `buildNumber`, `changelog_context.json`, and `CHANGELOG.md` review as described in `README.md`. Native dependency changes require rebuilding the custom dev client (`src/front/scripts/buildForDevelopment.sh`); do not claim an OTA update is sufficient. + +## CI and deployment map + +GitHub workflows are the authoritative CI definitions in `.github/workflows/`: + +- `push_back.yml`: on API/shared/database migration changes, installs `back` and `common`, lints both, then runs `npm run test:ci`; `dev` deploys staging and `main` production after Cloud SQL migrations. +- `push_front.yml`: on front/shared/release metadata changes, installs front/common, lints common, then `npm run test:final`. +- `push_website.yml`: on website/shared changes, installs website/common and lints both; `dev`/`main` deploy Cloud Run staging/production. +- `push_palletjack.yml`: installs `.[tests]`, runs Ruff and pytest; `dev`/`main` deploy the Cloud Run Job. Its staging scheduler is paused deliberately. + +Database migrations live in `data/migrations/`; `data/knexfile.js` defines `development`, `Staging`, and `Production`. Local resets use `data/rebuild_database.sh` and destructively remove database volumes. Cloud SQL release access uses `data/start_cloud_sql_proxy.sh dev` on 5432 or another environment (for example `prod`) on 5433; it requires authenticated `gcloud` and the Cloud SQL proxy. Do not run deploy, release, EAS, Cloud SQL, or destructive database scripts for ordinary code changes. + +Manual mobile release coverage is in `docs/ManualTests.md`, especially authentication, public/contractor/agency reporting, routes, photos, offline sync, and profile deletion. Include the relevant scenarios when a change affects those workflows. + +Repository root contains the primary overview (`README.md`), release history (`CHANGELOG.md`), release metadata (`changelog_context.json`), `docker-compose.yaml`, and root changelog script (`npm run changelog`). Formatting defaults are `.prettierrc.json` and `.prettierignore`; lint configs are local to each service. Trust this guide for setup and validation. Search only when the requested behavior, affected service, or this guide's facts are incomplete or demonstrably stale. diff --git a/.github/workflows/push_palletjack.yml b/.github/workflows/push_palletjack.yml index 7c07bbf6..6bc6050a 100644 --- a/.github/workflows/push_palletjack.yml +++ b/.github/workflows/push_palletjack.yml @@ -3,12 +3,12 @@ name: Palletjack - Deploy to Cloud Run Job on: push: paths: - - 'src/palletjack/**' - - '.github/workflows/palletjack.yml' - - '.github/actions/palletjack/deploy/action.yml' + - "src/palletjack/**" + - ".github/workflows/palletjack.yml" + - ".github/actions/palletjack/deploy/action.yml" concurrency: - group: '${{ github.head_ref || github.ref }}' + group: "${{ github.head_ref || github.ref }}" cancel-in-progress: true permissions: @@ -33,7 +33,7 @@ jobs: uses: actions/setup-python@v6 with: cache: pip - cache-dependency-path: src/palletjack/setup.py + cache-dependency-path: src/palletjack/pyproject.toml - name: 📥 Download extra dependencies run: | @@ -49,7 +49,6 @@ jobs: - name: 🧪 Run tests run: pytest - deploy-dev: name: Deploy Staging to GCF runs-on: ubuntu-latest @@ -68,7 +67,7 @@ jobs: project_id: ${{ secrets.PROJECT_ID }} identity_provider: ${{ secrets.IDENTITY_PROVIDER }} service_account_email: ${{ secrets.SERVICE_ACCOUNT_EMAIL }} - pause_schedule_job: 'yes' + pause_schedule_job: "yes" github_token: ${{ secrets.GITHUB_TOKEN }} cloudsql_instance: ${{ secrets.CLOUDSQL_INSTANCE }} diff --git a/.vscode/settings.json b/.vscode/settings.json index 9fa10f18..c920e826 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -88,6 +88,8 @@ "prestart", "PSAP", "pylint", + "pyproject", + "pytest", "Reactotron", "Recents", "REMOTEDEV", @@ -109,6 +111,8 @@ "unsubmitted", "Unsubmitted", "upsert", + "Upserted", + "Upserting", "utah", "utahid", "utahwvcr", @@ -137,4 +141,5 @@ "python.testing.pytestEnabled": true, "python.testing.cwd": "src/palletjack", "isort.check": true, + "python-envs.defaultPackageManager": "ms-python.python:conda", } diff --git a/src/back/api/validation.js b/src/back/api/validation.js index 5f989a46..0dc14e08 100644 --- a/src/back/api/validation.js +++ b/src/back/api/validation.js @@ -10,3 +10,17 @@ export default function validate(schema) { } }; } + +// Remove after cached submissions created before the mobile input limit have retried. +export function truncateOverlongComments(request, _, next) { + const { comments } = request.body; + if (typeof comments === 'string' && comments.length > 512) { + console.warn('truncating overlong report comments', { + originalLength: comments.length, + url: request.originalUrl, + }); + request.body.comments = comments.slice(0, 512); + } + + next(); +} diff --git a/src/back/api/validation.test.js b/src/back/api/validation.test.js new file mode 100644 index 00000000..128cc6dd --- /dev/null +++ b/src/back/api/validation.test.js @@ -0,0 +1,38 @@ +import { describe, expect, it, vi } from 'vitest'; +import { truncateOverlongComments } from './validation.js'; + +describe('truncateOverlongComments', () => { + it('truncates overlong comments and logs only metadata', () => { + const comments = 'a'.repeat(513); + const request = { + body: { comments }, + originalUrl: '/reports/report', + }; + const next = vi.fn(); + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + truncateOverlongComments(request, {}, next); + + expect(request.body.comments).toHaveLength(512); + expect(next).toHaveBeenCalledOnce(); + expect(warn).toHaveBeenCalledWith('truncating overlong report comments', { + originalLength: 513, + url: '/reports/report', + }); + + warn.mockRestore(); + }); + + it('leaves valid comments unchanged', () => { + const request = { + body: { comments: 'short comment' }, + originalUrl: '/reports/report', + }; + const next = vi.fn(); + + truncateOverlongComments(request, {}, next); + + expect(request.body.comments).toBe('short comment'); + expect(next).toHaveBeenCalledOnce(); + }); +}); diff --git a/src/back/app.js b/src/back/app.js index 4aab15a3..27660a4a 100644 --- a/src/back/app.js +++ b/src/back/app.js @@ -1,6 +1,9 @@ import { LoggingWinston } from '@google-cloud/logging-winston'; import { getConstants } from 'common/constants.js'; -import { pickup as pickupSchema, report as reportSchema } from 'common/validation/reports.js'; +import { + pickup as pickupSchema, + report as reportSchema, +} from 'common/validation/reports.js'; import { route as routeSchema } from 'common/validation/routes.js'; import compression from 'compression'; import cors from 'cors'; @@ -11,9 +14,18 @@ import Multer from 'multer'; import winston from 'winston'; import { getGetIDImageHandler } from './api/id_images.js'; import { getGetPhotoHandler } from './api/photos.js'; -import { getGetReportHandler, getNewPickupHandler, getNewReportHandler } from './api/reports.js'; +import { + getGetReportHandler, + getNewPickupHandler, + getNewReportHandler, +} from './api/reports.js'; import { getGetRouteHandler, getNewRouteHandler } from './api/routes.js'; -import { authenticate, getToken, logout, verifyAppleTokenAndCode } from './api/security.js'; +import { + authenticate, + getToken, + logout, + verifyAppleTokenAndCode, +} from './api/security.js'; import { getGetAllHandler } from './api/submissions.js'; import { getApprove, @@ -24,12 +36,16 @@ import { getReject, getUpdateProfile, } from './api/user.js'; -import validate from './api/validation.js'; +import validate, { truncateOverlongComments } from './api/validation.js'; import getVersionFromHeader from './api/versioning.js'; import { getIDImage } from './services/id_images.js'; import sendReportNotification from './services/notifications.js'; import { getPhoto, upload } from './services/photos.js'; -import { createPickup, createReport, getReport } from './services/report_management.js'; +import { + createPickup, + createReport, + getReport, +} from './services/report_management.js'; import { createRoute, getRoute } from './services/route_management.js'; import { getMySubmissions } from './services/submissions.js'; import { @@ -39,6 +55,7 @@ import { getUser, isExistingUser, loginSchema, + profileUpdateSchema, registerSchema, registerUser, rejectUser, @@ -59,7 +76,7 @@ app.use(express.urlencoded({ extended: true })); app.use( express.json({ limit: '50mb', - }) + }), ); // accept files @@ -86,15 +103,25 @@ if (process.env.ENVIRONMENT === 'development') { msg: 'HTTP {{req.method}} {{req.url}}', expressFormat: true, colorize: true, - }) + }), ); -} else if (process.env.ENVIRONMENT !== 'development' && process.env.ENVIRONMENT !== 'test') { +} else if ( + process.env.ENVIRONMENT !== 'development' && + process.env.ENVIRONMENT !== 'test' +) { app.use( expressWinston.logger({ transports: [new LoggingWinston()], metaField: null, //this causes the metadata to be stored at the root of the log entry responseField: null, // this prevents the response from being included in the metadata (including body and status code) - requestWhitelist: ['url', 'method', 'httpVersion', 'originalUrl', 'query', 'body'], + requestWhitelist: [ + 'url', + 'method', + 'httpVersion', + 'originalUrl', + 'query', + 'body', + ], responseWhitelist: ['body'], // this populates the `res.body` so we can get the response size (not required) format: winston.format.json(), ignoreRoute: (request) => request?.route?.path === '/user/login', @@ -105,7 +132,10 @@ if (process.env.ENVIRONMENT === 'development') { meta.httpRequest = httpRequest; httpRequest.requestMethod = req.method; httpRequest.requestUrl = `${req.protocol}://${req.get('host')}${req.originalUrl}`; - httpRequest.remoteIp = req.ip.indexOf(':') >= 0 ? req.ip.substring(req.ip.lastIndexOf(':') + 1) : req.ip; // just ipv4 + httpRequest.remoteIp = + req.ip.indexOf(':') >= 0 + ? req.ip.substring(req.ip.lastIndexOf(':') + 1) + : req.ip; // just ipv4 httpRequest.requestSize = req.socket.bytesRead; httpRequest.userAgent = req.get('User-Agent'); httpRequest.referrer = req.get('Referrer'); @@ -129,7 +159,7 @@ if (process.env.ENVIRONMENT === 'development') { return meta; }, meta: true, - }) + }), ); } @@ -149,58 +179,95 @@ app.post( '/user/register', handleAsyncErrors(authenticate), validate(registerSchema), - handleAsyncErrors(getRegister(isExistingUser, registerUser, getUser)) + handleAsyncErrors(getRegister(isExistingUser, registerUser, getUser)), ); app.post( '/user/login', handleAsyncErrors(authenticate), validate(loginSchema), - handleAsyncErrors(getLogin(getUser, updateUser, getConstants)) + handleAsyncErrors(getLogin(getUser, updateUser, getConstants)), ); app.post('/user/logout', handleAsyncErrors(logout)); -app.get('/user/approval/:guid/:role', handleAsyncErrors(getApprove(approveUser))); +app.get( + '/user/approval/:guid/:role', + handleAsyncErrors(getApprove(approveUser)), +); app.get('/user/reject/:guid', handleAsyncErrors(getReject(rejectUser))); -app.get('/user/profile', handleAsyncErrors(authenticate), handleAsyncErrors(getGetProfile(getProfile))); -app.post('/user/profile/update', handleAsyncErrors(authenticate), handleAsyncErrors(getUpdateProfile(updateProfile))); -app.delete('/user/delete', handleAsyncErrors(authenticate), handleAsyncErrors(getDeleteUser(deleteUser))); +app.get( + '/user/profile', + handleAsyncErrors(authenticate), + handleAsyncErrors(getGetProfile(getProfile)), +); +app.post( + '/user/profile/update', + handleAsyncErrors(authenticate), + validate(profileUpdateSchema), + handleAsyncErrors(getUpdateProfile(updateProfile)), +); +app.delete( + '/user/delete', + handleAsyncErrors(authenticate), + handleAsyncErrors(getDeleteUser(deleteUser)), +); // data submission app.post( '/reports/report', handleAsyncErrors(authenticate), multer.single('photo'), + truncateOverlongComments, validate(reportSchema.omit(['photo'])), - handleAsyncErrors(getNewReportHandler(upload, createReport, sendReportNotification)) + handleAsyncErrors( + getNewReportHandler(upload, createReport, sendReportNotification), + ), ); app.post( '/reports/pickup', handleAsyncErrors(authenticate), multer.single('photo'), + truncateOverlongComments, validate(pickupSchema.omit(['photo'])), - handleAsyncErrors(getNewPickupHandler(upload, createPickup)) + handleAsyncErrors(getNewPickupHandler(upload, createPickup)), ); app.post( '/routes/route', handleAsyncErrors(authenticate), validate(routeSchema), - handleAsyncErrors(getNewRouteHandler(createRoute)) + handleAsyncErrors(getNewRouteHandler(createRoute)), ); // data retrieval -app.get('/submissions', handleAsyncErrors(authenticate), handleAsyncErrors(getGetAllHandler(getMySubmissions))); +app.get( + '/submissions', + handleAsyncErrors(authenticate), + handleAsyncErrors(getGetAllHandler(getMySubmissions)), +); app.get( '/reports/report/:reportId', handleAsyncErrors(authenticate), - handleAsyncErrors(getGetReportHandler(getReport)) + handleAsyncErrors(getGetReportHandler(getReport)), +); +app.get( + '/routes/route/:routeId', + handleAsyncErrors(authenticate), + handleAsyncErrors(getGetRouteHandler(getRoute)), ); -app.get('/routes/route/:routeId', handleAsyncErrors(authenticate), handleAsyncErrors(getGetRouteHandler(getRoute))); // these endpoints are unsecured to support report notification emails -app.get('/photos/thumb/:photoId', handleAsyncErrors(getGetPhotoHandler(true, getPhoto))); -app.get('/photos/:photoId', handleAsyncErrors(getGetPhotoHandler(false, getPhoto))); +app.get( + '/photos/thumb/:photoId', + handleAsyncErrors(getGetPhotoHandler(true, getPhoto)), +); +app.get( + '/photos/:photoId', + handleAsyncErrors(getGetPhotoHandler(false, getPhoto)), +); // I don't see any reason to secure this endpoint and am worried that it will just slow it down -app.get('/reports/id_image/:key/:pixelRatio', handleAsyncErrors(getGetIDImageHandler(getIDImage))); +app.get( + '/reports/id_image/:key/:pixelRatio', + handleAsyncErrors(getGetIDImageHandler(getIDImage)), +); export default app; diff --git a/src/back/services/user_management.js b/src/back/services/user_management.js index 4d2572e7..0060a17f 100644 --- a/src/back/services/user_management.js +++ b/src/back/services/user_management.js @@ -20,7 +20,7 @@ export const registerSchema = yup.object().shape({ organization: yup .object() .shape({ - name: yup.string().required(), + name: yup.string().max(128).required(), org_type: yup.string().required(), id: yup.number().required(), }) @@ -32,19 +32,26 @@ export const registerSchema = yup.object().shape({ auth_provider: yup.string().required(), auth_id: yup.string().required(), email: yup.string().email().required(), - first_name: yup.string().required(), - last_name: yup.string().required(), + first_name: yup.string().max(25).required(), + last_name: yup.string().max(25).required(), phone: yup.string().required(), }) .required(), }); +export const profileUpdateSchema = yup.object().shape({ + phone: yup.string().required(), + organization_id: yup.number().nullable(), + organization_name: yup.string().max(128).nullable(), + organization_type: yup.string().required(), +}); + export const loginSchema = yup.object().shape({ auth_id: yup.string().required(), auth_provider: yup.string().required(), email: yup.string().email().nullable(), // apple requests may not have this - first_name: yup.string().nullable(), // apple requests may not have this - last_name: yup.string().nullable(), // apple requests may not have this + first_name: yup.string().max(25).nullable(), // apple requests may not have this + last_name: yup.string().max(25).nullable(), // apple requests may not have this }); export async function isExistingUser({ auth_provider, auth_id }) { diff --git a/src/back/services/user_management.test.js b/src/back/services/user_management.test.js index a30fdeb1..8d364780 100644 --- a/src/back/services/user_management.test.js +++ b/src/back/services/user_management.test.js @@ -1,5 +1,49 @@ +import { report as reportSchema } from 'common/validation/reports.js'; import { describe, expect, it } from 'vitest'; -import { approveUser, checkExpiration, EXPIRED_APPROVAL } from './user_management.js'; +import { + approveUser, + checkExpiration, + EXPIRED_APPROVAL, + profileUpdateSchema, + registerSchema, +} from './user_management.js'; + +describe('text field validation', () => { + it('rejects comments longer than 512 characters', () => { + expect(() => + reportSchema.validateSyncAt('comments', { comments: 'a'.repeat(513) }), + ).toThrow(); + }); + + it('rejects registration organization names longer than 128 characters', () => { + expect(() => + registerSchema.validateSyncAt('organization.name', { + organization: { name: 'a'.repeat(129) }, + }), + ).toThrow(); + }); + + it('rejects registration first and last names longer than 25 characters', () => { + expect(() => + registerSchema.validateSyncAt('user.first_name', { + user: { first_name: 'a'.repeat(26) }, + }), + ).toThrow(); + expect(() => + registerSchema.validateSyncAt('user.last_name', { + user: { last_name: 'a'.repeat(26) }, + }), + ).toThrow(); + }); + + it('rejects profile organization names longer than 128 characters', () => { + expect(() => + profileUpdateSchema.validateSyncAt('organization_name', { + organization_name: 'a'.repeat(129), + }), + ).toThrow(); + }); +}); describe('approveUser', () => { it('throws an error if there is no matching guid in firestore', async () => { diff --git a/src/common/validation/reports.js b/src/common/validation/reports.js index 6880964e..beb14a0e 100644 --- a/src/common/validation/reports.js +++ b/src/common/validation/reports.js @@ -21,7 +21,7 @@ const family = string().required(); const species_confidence_level = string().nullable(); const age_class = string().nullable(); const sex = string().nullable(); -const comments = string().nullable(); +const comments = string().max(512).nullable(); exports.report = object().shape({ animal_location, diff --git a/src/front/components/reports/Form.js b/src/front/components/reports/Form.js index fe5ca4cf..e1b18adc 100644 --- a/src/front/components/reports/Form.js +++ b/src/front/components/reports/Form.js @@ -148,6 +148,7 @@ export default function Form({ ', + ...getRegistrationNameFields(authInfo.oauthUser, name), email: authInfo.oauthUser.email || email, auth_id: authInfo.oauthUser.sub, auth_provider: authInfo.providerName, @@ -221,6 +218,7 @@ export default function NewUserScreen() { : null } textContentType="organizationName" + maxLength={128} value={ values.organization_name !== commonConfig.otherOrg.name diff --git a/src/front/screens/Profile.js b/src/front/screens/Profile.js index a4f6cff4..2ccfb56d 100644 --- a/src/front/screens/Profile.js +++ b/src/front/screens/Profile.js @@ -1,3 +1,4 @@ +import * as Sentry from '@sentry/react-native'; import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query'; import { Button, @@ -18,7 +19,6 @@ import { StyleSheet, View, } from 'react-native'; -import * as Sentry from '@sentry/react-native'; import { number, object, string } from 'yup'; import 'yup-phone-lite'; import useAuth from '../auth/context'; @@ -119,7 +119,7 @@ export default function ProfileScreen() { phone: string().phone('US').required(), }; if (data?.organization_id) { - shape.organization_name = string().required(); + shape.organization_name = string().max(128).required(); shape.organization_id = number().required(); } const schema = object().shape(shape); @@ -237,6 +237,7 @@ export default function ProfileScreen() { : null } textContentType="organizationName" + maxLength={128} value={ values.organization_name !== commonConfig.otherOrg.name diff --git a/src/front/services/utilities.js b/src/front/services/utilities.js index d9bc53ab..5b7fe194 100644 --- a/src/front/services/utilities.js +++ b/src/front/services/utilities.js @@ -119,6 +119,21 @@ export function useAsyncError() { return throwAsyncError; } +export function getRegistrationNameFields(oauthUser, name) { + const nameParts = (name ?? '').trim().split(/\s+/); + const firstName = (oauthUser?.given_name || nameParts[0] || '').trim(); + const lastName = ( + oauthUser?.family_name || + nameParts[1] || + '' + ).trim(); + + return { + first_name: firstName.slice(0, 25), + last_name: lastName.slice(0, 25), + }; +} + export function isTokenExpired(token) { if (typeof token === 'string') { throw new Error('token must be an object, did you forget to decode it?'); diff --git a/src/front/services/utilities.test.js b/src/front/services/utilities.test.js index d4c9c0a9..abc7cb06 100644 --- a/src/front/services/utilities.test.js +++ b/src/front/services/utilities.test.js @@ -4,6 +4,7 @@ import { appendCoordinates, coordinatesToRegion, extentStringToRegion, + getRegistrationNameFields, getSubmitValues, isTokenExpired, lineCoordinatesToString, @@ -14,6 +15,37 @@ import { wrapAsyncWithDelay, } from './utilities'; +describe('getRegistrationNameFields', () => { + it('splits and trims a manually entered name', () => { + expect(getRegistrationNameFields({}, ' Jane Doe ')).toEqual({ + first_name: 'Jane', + last_name: 'Doe', + }); + }); + + it('uses the surname fallback for a single manually entered name', () => { + expect(getRegistrationNameFields({}, 'Jane')).toEqual({ + first_name: 'Jane', + last_name: '', + }); + }); + + it('truncates OAuth name fields to the database limit', () => { + expect( + getRegistrationNameFields( + { + given_name: 'a'.repeat(26), + family_name: 'b'.repeat(26), + }, + '', + ), + ).toEqual({ + first_name: 'a'.repeat(25), + last_name: 'b'.repeat(25), + }); + }); +}); + describe('isTokenExpired', () => { it('correctly checks a future date', () => { const decodedToken = { diff --git a/src/palletjack/pyproject.toml b/src/palletjack/pyproject.toml index e9fd64c2..6cdd6f95 100644 --- a/src/palletjack/pyproject.toml +++ b/src/palletjack/pyproject.toml @@ -1,6 +1,41 @@ +[build-system] +requires = ["hatchling"] +build-backend = "hatchling.build" + +[project] +name = "palletjack" +version = "1.0.0" +description = "Roadkill skid" +readme = "README.md" +license = { text = "MIT" } +authors = [{ name = "UGRC Developers", email = "ugrc-developers@utah.gov" }] +keywords = ["gis"] +classifiers = [ + "Development Status :: 5 - Production/Stable", + "Intended Audience :: Developers", + "Topic :: Utilities", +] +dependencies = ["ugrc-palletjack==6.*", "ugrc-supervisor==3.*"] + +[project.urls] +Homepage = "https://github.com/agrc/roadkill-mobile" + +[project.optional-dependencies] +tests = ["pandas==2.*", "pytest-instafail==0.5.*", "pytest>=8,<10", "ruff==0.*"] + +[project.scripts] +process = "roadkill.main:process" + +[tool.hatch.build.targets.wheel] +packages = ["src/roadkill"] + [tool.ruff] line-length = 120 -lint.ignore = ["E501"] + +[tool.ruff.lint] +extend-select = ["I"] +ignore = ["E501"] + [tool.pytest.ini_options] minversion = "6.0" testpaths = ["tests", "src"] diff --git a/src/palletjack/setup.py b/src/palletjack/setup.py deleted file mode 100644 index 27bd082d..00000000 --- a/src/palletjack/setup.py +++ /dev/null @@ -1,49 +0,0 @@ -#!/usr/bin/env python -# -*- encoding: utf-8 -*- -""" -setup.py -A module that installs the backup process as a module -""" - -from setuptools import find_packages, setup - -setup( - name="palletjack", - version="1.0.0", - license="MIT", - description="Roadkill skid", - author="UGRC Developers", - author_email="ugrc-developers@utah.gov", - url="https://github.com/agrc/roadkill-mobile", - packages=find_packages("src"), - package_dir={"": "src"}, - include_package_data=True, - zip_safe=True, - classifiers=[ - # complete classifier list: http://pypi.python.org/pypi?%3Aaction=list_classifiers - "Development Status :: 5 - Production/Stable", - "Intended Audience :: Developers", - "Topic :: Utilities", - ], - keywords=["gis"], - install_requires=[ - "ugrc-palletjack==5.*", - "ugrc-supervisor==3.*", - ], - extras_require={ - "tests": [ - "pandas==2.*", - "pytest-instafail==0.5.*", - "pytest>=8,<10", - "ruff==0.*", - ] - }, - setup_requires=[ - "pytest-runner", - ], - entry_points={ - "console_scripts": [ - "process = roadkill.main:process", - ] - }, -) diff --git a/src/palletjack/src/roadkill/cleanup_duplicates.py b/src/palletjack/src/roadkill/cleanup_duplicates.py new file mode 100644 index 00000000..84e356d6 --- /dev/null +++ b/src/palletjack/src/roadkill/cleanup_duplicates.py @@ -0,0 +1,151 @@ +"""Remove duplicate logical records from Roadkill AGOL feature layers. + +Run without --execute to preview. Deletions retain the lowest OBJECTID for each +configured business key and refuse layers that support feature attachments. + +This is a one-off script to clean up the Roadkill AGOL feature layers. I'm leaving it here in case we need to run it again, but it should not be used as part of the normal workflow. +""" + +import argparse +from collections import defaultdict +from itertools import islice + +import arcgis + +from . import config +from .main import _get_secrets + +DELETE_BATCH_SIZE = 500 + + +def _chunks(values, size): + iterator = iter(values) + while batch := list(islice(iterator, size)): + yield batch + + +def _get_layer(gis, table): + results = gis.content.search(query=f'owner:{gis.users.me.username} AND title:"{table}" AND type:Feature Service') + matches = [result for result in results if result.title == table] + if len(matches) != 1: + raise RuntimeError(f"Expected exactly one Feature Service titled {table}; found {len(matches)}") + + return matches[0].layers[0] + + +def _get_duplicate_object_ids(layer, id_column): + object_id_field = layer.properties.objectIdField + max_record_count = layer.properties.maxRecordCount + object_ids_by_key = defaultdict(list) + offset = 0 + + while True: + print(f"Querying {layer.properties.name} for {id_column} duplicates, offset {offset}") + result = layer.query( + where="1=1", + out_fields=f"{object_id_field},{id_column}", + return_geometry=False, + result_offset=offset, + result_record_count=max_record_count, + ) + features = result.features + if not features: + break + + for feature in features: + attributes = feature.attributes + key = attributes[id_column] + if key is None: + raise RuntimeError(f"Found a null {id_column} in {layer.properties.name}") + object_ids_by_key[key].append(attributes[object_id_field]) + + offset += len(features) + if len(features) < max_record_count: + break + + duplicate_groups = { + key: sorted(object_ids) for key, object_ids in object_ids_by_key.items() if len(object_ids) > 1 + } + duplicate_object_ids = [object_id for object_ids in duplicate_groups.values() for object_id in object_ids[1:]] + return duplicate_groups, duplicate_object_ids + + +def _delete_duplicate_object_ids(layer, object_ids): + for batch in _chunks(object_ids, DELETE_BATCH_SIZE): + result = layer.delete_features(deletes=",".join(map(str, batch)), rollback_on_failure=True) + failures = [delete for delete in result["deleteResults"] if not delete["success"]] + if failures: + raise RuntimeError(f"Failed to delete features: {failures}") + + +def cleanup_table(gis, table, id_column, execute): + layer = _get_layer(gis, table) + if execute and layer.properties.hasAttachments: + raise RuntimeError( + f"{table} supports attachments; inspect or migrate them before deleting duplicate features." + ) + + duplicate_groups, duplicate_object_ids = _get_duplicate_object_ids(layer, id_column) + print( + f"{table}: {len(duplicate_groups)} duplicate {id_column} groups; " + f"{len(duplicate_object_ids)} features " + f"{'will be deleted' if execute else 'would be deleted'}" + ) + + if not execute or not duplicate_object_ids: + return len(duplicate_object_ids) + + _delete_duplicate_object_ids(layer, duplicate_object_ids) + remaining_groups, remaining_object_ids = _get_duplicate_object_ids(layer, id_column) + if remaining_groups: + raise RuntimeError(f"{table} still has {len(remaining_groups)} duplicate groups after deletion") + if remaining_object_ids: + raise RuntimeError(f"{table} still has {len(remaining_object_ids)} duplicate features after deletion") + + print(f"{table}: deleted {len(duplicate_object_ids)} duplicate features") + return len(duplicate_object_ids) + + +def parse_args(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--table", + choices=[table[0] for table in config.TABLES], + action="append", + help="Target table to clean. Repeat to clean multiple tables. Defaults to every configured table.", + ) + parser.add_argument("--execute", action="store_true", help="Delete duplicates. Omit for a dry run.") + parser.add_argument( + "--confirm", + action="append", + default=[], + help="Required with --execute for every selected table. Repeat once per --table value.", + ) + return parser.parse_args() + + +def main(): + args = parse_args() + selected_tables = set(args.table or [table[0] for table in config.TABLES]) + + if args.execute and selected_tables != set(args.confirm): + raise SystemExit("--execute requires --confirm for every selected --table value") + + secrets = _get_secrets() + gis = arcgis.gis.GIS( + username=secrets["AGOL_USER"], + password=secrets["AGOL_PASSWORD"], + url=secrets["AGOL_ORG"], + expiration=9999, + ) + + total = 0 + for table, id_column, *_ in config.TABLES: + if table in selected_tables: + total += cleanup_table(gis, table, id_column, args.execute) + + print(f"{total} duplicate features {'deleted' if args.execute else 'identified'}") + + +if __name__ == "__main__": + main() diff --git a/src/palletjack/src/roadkill/config.py b/src/palletjack/src/roadkill/config.py index 46dc13d8..4c3c13a3 100644 --- a/src/palletjack/src/roadkill/config.py +++ b/src/palletjack/src/roadkill/config.py @@ -12,7 +12,7 @@ "to_addresses": "ugrc-developers@utah.gov", "prefix": f"{os.getenv('PROJECT_ID')}: ", } -LOG_LEVEL = logging.DEBUG +LOG_LEVEL = logging.INFO LOG_FILE_NAME = "log" TABLES = [ diff --git a/src/palletjack/src/roadkill/main.py b/src/palletjack/src/roadkill/main.py index 82e629b4..26c86021 100644 --- a/src/palletjack/src/roadkill/main.py +++ b/src/palletjack/src/roadkill/main.py @@ -1,5 +1,3 @@ -#!/usr/bin/env python -# * coding: utf8 * """ Run the roadkill skid """ @@ -13,11 +11,10 @@ from types import SimpleNamespace import arcgis +from palletjack import extract, load, transform from supervisor.message_handlers import SendGridHandler from supervisor.models import MessageDetails, Supervisor -from palletjack import extract, load, transform - #: This makes it work when calling with just `python `/installing via pip and in the gcf framework, where #: the relative imports fail because of how it's calling the function. try: @@ -121,7 +118,7 @@ def _remove_log_file_handlers(log_name, loggers): if log_name in handler.stream.name: logger.removeHandler(handler) handler.close() - except Exception: + except AttributeError: pass @@ -145,6 +142,13 @@ def _get_new_and_deleted_records(database_records, agol_records): return (new_records, deleted_ids) +def _ensure_unique_ids(records, table, id_column): + """Raise when a source table contains duplicate business keys.""" + + if records.index.has_duplicates: + raise ValueError(f"{table} contains duplicate {id_column} values") + + def _transform(dataframe, int_fields, date_fields): """A helper function to prepare the dataframe to be loaded into AGOL @@ -170,7 +174,7 @@ def process(): """The main function that does all the work.""" #: Set up secrets, tempdir, supervisor, and logging - start = datetime.datetime.now() + start = datetime.datetime.now(datetime.timezone.utc).astimezone() secrets = SimpleNamespace(**_get_secrets()) @@ -213,6 +217,7 @@ def process(): #: get data from database database_records = loader.read_table_into_dataframe(f"public.{table}", id_column, "4326", geog_column) database_records.rename(columns={geog_column: "SHAPE"}, inplace=True) + _ensure_unique_ids(database_records, table, id_column) module_logger.info("Database records count: %s", len(database_records)) search_results = gis.content.search( @@ -226,7 +231,10 @@ def process(): continue - item = [r for r in search_results if r.title == table][0] + try: + item = next(result for result in search_results if result.title == table) + except StopIteration: + raise RuntimeError(f"Expected a Feature Service titled \"{table}\" but none matched exactly") from None layer = item.layers[0] #: strip off the last part of the url to get the service url service_url = "/".join(layer.url.split("/")[:-1]) @@ -241,13 +249,13 @@ def process(): module_logger.info("Deleted records count: %s", len(deleted_ids)) if len(new_records) > 0: - module_logger.info("Adding new records to AGOL...") + module_logger.info("Upserting new records to AGOL...") prepared_df = _transform(new_records, int_fields, date_fields) if table == "agol_public_reports": prepared_df["repeat_submission"] = prepared_df["repeat_submission"].astype("int") updater = load.ServiceUpdater(gis, item.id, working_dir=tempdir_path) - updates = updater.add(prepared_df) - module_logger.info("Added %s records", updates) + updates = updater.update(prepared_df, matching_field=id_column) + module_logger.info("Upserted %s records", updates) if len(deleted_ids) > 0: module_logger.info("Deleting records...") @@ -259,7 +267,7 @@ def process(): f"{table} database records: {len(database_records)}, agol records: {len(agol_records)}, new records: {len(new_records)}, deleted records: {len(deleted_ids)}" ) - end = datetime.datetime.now() + end = datetime.datetime.now(datetime.timezone.utc).astimezone() summary_message = MessageDetails() summary_message.subject = f"{config.SKID_NAME} Update Summary" @@ -269,7 +277,7 @@ def process(): "", f"Start time: {start.strftime('%H:%M:%S')}", f"End time: {end.strftime('%H:%M:%S')}", - f"Duration: {str(end - start)}", + f"Duration: {end - start!s}", ] + additional_summary_rows summary_message.message = "\n".join(summary_rows) diff --git a/src/palletjack/src/roadkill/test_main.py b/src/palletjack/src/roadkill/test_main.py index 9839f0fd..e2d050dc 100644 --- a/src/palletjack/src/roadkill/test_main.py +++ b/src/palletjack/src/roadkill/test_main.py @@ -1,4 +1,5 @@ import pandas as pd +import pytest from . import main @@ -27,3 +28,10 @@ def test_get_new_and_updated_records(): assert len(deleted_ids) == 1 assert deleted_ids[0] == 20 + + +def test_ensure_unique_ids_raises_for_duplicates(): + records = pd.DataFrame({"report_id": [1, 1], "dummy": ["a", "b"]}).set_index("report_id") + + with pytest.raises(ValueError, match="agol_pickup_reports contains duplicate report_id values"): + main._ensure_unique_ids(records, "agol_pickup_reports", "report_id")