# Contributing to ShopDB Flask

## Coding Standards

### Naming principles

1. Spell it out. If a non-technical user looking at a report would not understand a column at a glance, rename it.
2. No project-specific shorthand. A name either uses a word in full, or uses an acronym from the allowed list below.
3. Consistency beats cleverness. Once a name is committed and used in code, do not rename it for marginal improvement.
4. When unsure, ask: "would a new shop-floor IT person understand this?"

### Database

- **Table names**: lowercase, concatenated, plural. No dashes, no underscores.
  - Good: `machines`, `pctypes`, `networkdevices`, `businessunits`, `auditlogs`
  - Bad: `machine_types`, `network-devices`, `BusinessUnits`
- **Column names**: lowercase, concatenated, singular. No dashes, no underscores.
  - Good: `machineid`, `machinenumber`, `lastzabbixsync`, `isactive`
  - Bad: `machine_id`, `last_zabbix_sync`, `IsActive`
- **Foreign keys**: referenced table name (singular) + `id`.
  - Good: `locationid` references `locations`, `vendorid` references `vendors`
- **Booleans**: prefix with `is` or `has`.
  - Good: `isactive`, `isshopfloor`, `hasprinter`
- **Index names**: `idx_<table>_<column>`. Underscores allowed here only because indexes are infrastructure, not data.
  - Good: `idx_machines_locationid`

### Migrations and deploy ordering

- **A model change ships WITH its migration in the same change.** Adding or
  changing a column/table on a SQLAlchemy model without the matching Alembic
  migration is a broken change, even if it passes tests against a freshly built
  schema.
- **Code and schema must never deploy out of step.** A model that references a
  column its migration has not applied yet makes MySQL raise
  `1054 Unknown column`, which 500s every query on that table (this is not a
  degraded read - the whole endpoint fails).
- **Bringing up a new image MUST run migrations before serving traffic**, in
  order: `flask db upgrade` (core chain) then `flask plugin upgrade-all`
  (per-plugin chains). A rebuilt image with new model code but an
  un-migrated database will 500 until this runs.
- Core schema lives in `migrations/versions/`; each plugin owns its own chain
  under `plugins/<name>/migrations/`. New schema = a new migration with an
  idempotent guard and a real downgrade, clean on strict MySQL 8.

### Allowed acronyms (closed list, expand by PR)

- **Universal**: `id`, `url`, `api`, `http`, `https`, `json`, `jwt`, `sql`, `os`, `ip`, `dns`, `csv`, `pdf`, `cors`, `ttl`, `uuid`, `html`, `css`, `orm`
- **Domain (shop floor / IT infra)**: `cmm`, `cnc`, `pc`, `usb`, `vnc`, `winrm`, `ssh`, `ssl`, `tls`, `tcp`, `udp`, `smtp`, `ldap`, `vlan`, `sso`, `dnc`, `focas`, `clm`, `mtconnect`

Any acronym not on these lists must be spelled out. Proposing a new acronym is a separate PR that updates this list.

**Third-party imports are exempt.** Library and stdlib module paths cannot be renamed. `from sqlalchemy.orm import ...`, `import importlib.util`, `from werkzeug.utils import ...` are all fine even if they contain words that would otherwise be banned.

### Banned shorthand

`cfg`, `ctx`, `mgr`, `req`, `res`, `env`, `util`, `helper`, and any other domain-specific shortening. Spell it out: `config`, `context`, `manager`, `request`, `response`, `environment`, `utilities`. Note: `db` as a standalone variable name is banned, but `db` as a prefix in `database` or as part of project name `shopdb` is fine.

**Suffix exception.** Banned shorthand applies to standalone identifiers. As a suffix where the prefix establishes meaning, it is acceptable:

- `printers_bp`, `auth_bp`, `network_bp` are fine. The prefix names what blueprint, the `_bp` suffix is conventional for Flask blueprints.
- `request_obj` is fine when disambiguating from imported `request`.
- `bp` alone as a variable name is not fine. Always pair with a meaningful prefix.

### Code

#### Python

- **Variables and functions holding a database value**: match the column name exactly. No conversion to snake_case.
  - DB column `machineid` -> Python variable `machineid`, attribute `Machine.machineid`, dict key `{"machineid": 1}`
  - DB column `lastzabbixsync` -> Python variable `lastzabbixsync`
- **Pure code variables and functions** (loop counters, helpers, anything that does not mirror a DB field): snake_case per PEP 8.
  - Good: `loop_count`, `current_user`, `validate_input()`
- **Classes**: PascalCase. Spell out fully.
  - Good: `PrinterData`, `AssetType`, `NetworkDevice`
- **Modules / file names**: lowercase. Underscores allowed where it improves readability (PEP 8 permits this for modules).
  - Good: `employee_service.py`, `asset_routes.py`

#### JavaScript / Vue

- **Variables**: camelCase per JS convention.
  - Good: `currentUser`, `isLoading`
- **Variables holding API field values**: match the API field name exactly. Do NOT convert to camelCase.
  - API returns `{"machineid": 1}` -> JS `response.machineid`, NOT `response.machineId`
  - API returns `{"lastzabbixsync": "..."}` -> JS `device.lastzabbixsync`
- **Components**: PascalCase. Spell out fully.
  - Good: `AssetDetail.vue`, `MachineForm.vue`, `PrinterList.vue`
- **CSS classes**: lowercase with dashes.
  - Good: `asset-detail`, `machine-form`

#### API

- **Endpoints**: lowercase, plural nouns. No underscores or dashes.
  - Good: `/api/machines`, `/api/networkdevices`, `/api/businessunits`
- **Query parameters**: lowercase, concatenated. Match column names where applicable.
  - Good: `?locationid=5`, `?isactive=true`
- **Response keys**: match database column names exactly.
  - Good: `{"machineid": 1, "lastzabbixsync": "2026-01-12T10:00:00Z"}`

### Examples

```python
# Good - DB-mirrored fields keep column names; pure code uses snake_case
class PrinterData(db.Model):
    __tablename__ = 'printerdata'

    machineid = db.Column(db.Integer, db.ForeignKey('machines.machineid'))
    lastzabbixsync = db.Column(db.DateTime)
    isnetworkprinter = db.Column(db.Boolean)


def list_printers_by_location(locationid: int) -> list[dict]:
    printers = PrinterData.query.filter_by(locationid=locationid).all()
    result_count = len(printers)
    return [{"machineid": p.machineid, "isnetworkprinter": p.isnetworkprinter} for p in printers]
```

```python
# Bad - PEP 8-style underscores on DB fields, banned shorthand, project acronym not on allowed list
class PrinterData(db.Model):
    __tablename__ = 'printer_data'

    machine_id = db.Column(db.Integer)
    last_zabbix_sync = db.Column(db.DateTime)
    is_net_prn = db.Column(db.Boolean)


def list_prns_by_loc(loc_id):
    bp_ctx = get_context()
    cfg = load_cfg()
    return PrinterData.query.filter_by(loc_id=loc_id).all()
```

### Style policy (non-naming)

- No emojis in code, comments, documentation, string literals, or UI.
- No em-dashes (U+2014), en-dashes (U+2013), Unicode arrows, or smart quotes anywhere. Plain ASCII only.
- Comments default to none. Add one only when the WHY is non-obvious. Keep comments terse (lite caveman style is fine for inline `#` and `//` comments). Docstrings stay normal English.
- Dark theme is default. Keep UI functional and professional.

### File structure

- Core models: `shopdb/core/models/`
- Core API routes: `shopdb/core/api/`
- Core services: `shopdb/core/services/`
- Plugin code: `plugins/<plugin>/{models,api,services,schemas}/`

### Plugin development

When creating a new plugin:

1. Create directory `plugins/<name>/`. Use the scaffold (`flask plugin new <name>`) when available.
2. Include `manifest.json` with metadata. This is the single source of truth for plugin name, version, dependencies, api_prefix.
3. Extend `BasePlugin` class.
4. Follow naming conventions above.
5. Run `flask plugin install <name>` to install.

See the PLUGIN-QUICKSTART wiki page (when published) for the 30-minute walkthrough.

#### A plugin must not hold a DB session across external I/O

Rule: finish with the database before you call anything outside the process -
an HTTP API (Dell warranty, ServiceNow, Zabbix, Azure/Intune), a subprocess, a
file on a network share - and open a fresh session afterwards to write the
result.

Why it is a rule and not a preference. The app runs gevent workers, so a
request that blocks on a network call does not block its worker, and the call
can take minutes without anything complaining. What it DOES hold for those
minutes is a pooled database connection. Each worker owns a pool of
`DB_POOL_SIZE + DB_MAX_OVERFLOW` connections (see `shopdb/config.py` and the
arithmetic beside `--workers` in `docker-compose.yml`), and hundreds of
greenlets can enter the same slow path at once. Drain the pool and every other
request in that worker blocks on checkout and then fails - the whole site,
every plugin, while the CPU sits idle and MySQL reports itself healthy. That is
the worst diagnostic signature there is.

The shape to write:

```python
# Read what you need, then let the session go.
servicetags = [row.servicetag for row in query.all()]
db.session.remove()

results = provider.bulk_lookup(servicetags)   # slow, no connection held

for servicetag, result in results.items():    # new session, short write
    ...
db.session.commit()
```

Rules of thumb: no `requests`, `urllib`, `subprocess` or network-share access
between a query and its `commit()`; cap every external call with a timeout; and
if a sync is inherently long, chunk it so each chunk is its own
read / call / write cycle.

## Testing

Run tests with:

```bash
pytest tests/
```

Tests are required for:
- New routes (smoke + happy path + 1-2 edge cases)
- New plugin hooks (contract test)
- Schema migrations (before/after data integrity)

### Running the gates on Windows

Three commands in this repository resolve to the wrong program on a Windows dev
box. Each one fails in a way that looks like a code fault, which is why all
three are written out here rather than left to be rediscovered.

```bash
& "C:\Program Files\Git\bin\bash.exe" scripts/check-naming-and-style.sh
cd frontend; npm.cmd test; npm.cmd run build; cd ..
python -m pytest tests/ -q
```

- **`bash` resolves to WSL.** `C:\Windows\System32\bash.exe` is on PATH by
  default. With no distribution installed it fails with
  `execvpe(/bin/bash) failed: No such file or directory`, and with one installed
  it cannot read the Windows path you handed it. Always give the Git Bash path
  in full. The same trap bit the test suite: `tests/test_naming_gate_migrations.py`
  used bare `bash`, and its 13 failures were miscategorised as "environmental"
  for weeks. It now resolves Git Bash explicitly and probes it before use.
- **`npm` resolves to `npm.ps1`.** PowerShell prefers the `.ps1` over the
  `.cmd`, and the default execution policy is `Restricted`, so bare `npm` dies
  with "npm.ps1 cannot be loaded because running scripts is disabled on this
  system" before any test runs. Use `npm.cmd`. Nothing in the repo causes this
  and nothing in the repo can fix it; it is machine policy. If you would rather
  fix it once, `Set-ExecutionPolicy -Scope CurrentUser RemoteSigned` also works,
  but that is a machine-level change and `npm.cmd` is free.
- **`npm test`, not `npx vitest run`.** The `pretest` hook runs
  `stage-frontend.mjs`; bare vitest never stages plugin frontends and silently
  skips their specs.

### Keeping the environment matched to the lockfiles

```bash
python scripts/check-dependency-drift.py
```

Editing a lockfile changes what a FUTURE install produces. It does not touch an
already-populated `site-packages`. Commit `a8f8580` raised eight pins carrying
advisories; no install was re-run afterwards, so that workstation kept the old
libraries for three weeks while `requirements.txt` said otherwise. Every local
test run in that window exercised different libraries than a container build
would have.

Run this after any pull that touches `requirements.txt` or
`requirements-dev.txt`, and fix drift with:

```bash
python -m pip install --require-hashes -r requirements.txt
python -m pip install --require-hashes -r requirements-dev.txt
```

The check reports MISSING separately from MISMATCHED, because absent packages
are the class that gets missed: `pytest-xdist` was simply not installed, which
surfaced only as `pytest: error: unrecognized arguments: -n` - a message that
reads as a broken `pytest.ini` and invites deleting `-n auto` instead of
installing the plugin.

`scripts/check-naming-and-style.sh` runs this check too, advisory by default.
CI sets `DEPENDENCY_DRIFT_ENFORCE=1` to make it fail the build.

If you use the repo-local `env/` virtualenv, check it specifically - it drifts
independently of the global interpreter, and both were stale in the incident
above:

```bash
env/Scripts/python.exe scripts/check-dependency-drift.py
```

## Branches: what belongs on main, and what does not

`main` is the product every site runs and the publication mirror exports.
`<site>` branches hold work that is not ready for other sites to see. There are
two: `wj` here, and `wilm` on the corporate GitHub, where the other site
develops the .NET endpoint agent.

A site branch is not a fork. Both feed `main`, and the sooner they do it the
cheaper it is. A branch the dev box cannot see is still a branch: `wilm` is not
visible from here because this machine has no GitHub account, which is why
`fetch-wilm-branch.cmd` on the share exists to bring it back for review.

**Most work is product.** ADR-015 gives site differences three homes on `main`:
a setting with a neutral default, seed data, or a site-namespaced directory
(`scripts/site_imports/<site>/`, which the style gate exempts). That is the only
site-specific home in the tree, so a site branch is about TIMING, not locality:
things that are not ready yet, rather than things that are permanently ours.

Say which at commit time, with a trailer, because the decision does not survive
being deferred - months later it is a list of subject lines and a guess:

```bash
git commit -m "Add the thing" -m "Scope: product"   # main should have it
git commit -m "Add the thing" -m "Scope: site"      # this site alone
```

Then:

```bash
tools/wj-status.sh                          # what is unpromoted, and where it belongs
tools/promote-to-main.sh --dry-run <sha>    # what files would reach main
tools/promote-to-main.sh <sha> [<sha> ...]  # oldest first; runs main's gates
tools/git-push.sh main                      # you push, deliberately
```

Windows equivalents are `tools\git-push.cmd` and `tools\promote-to-main.cmd`.

Promotion cherry-picks NAMED commits rather than merging the branch. Merging
takes the site work along with the product work, which is how a site branch
quietly becomes the product.

**The failure to watch for is not what you think.** Promoting something
site-specific by accident is caught by the ADR-015 gate on the way into `main`.
The real risk is product work sitting on a site branch until bringing it back is
a port rather than a merge - one such round trip cost a MySQL dialect fix, a
contract violation and a plugin install.

So rebase the site branch on `main` often, and promote as you go rather than in
one batch at the end.

## Code review checklist

- [ ] No underscores in table or column names
- [ ] No banned shorthand or unapproved acronyms in any new identifier
- [ ] DB-mirrored Python and JS variables match column names exactly
- [ ] API response keys match column names exactly
- [ ] No em-dashes, en-dashes, smart quotes, Unicode arrows, or emojis (run `grep -rP '[^\x00-\x7F]' --include='*.py' --include='*.vue' --include='*.js' .`)
- [ ] Plugin follows `BasePlugin` interface and contract version
- [ ] Tests included for new functionality