diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index b996b7c..2b11bca 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -14,7 +14,9 @@ runtime tests are not applicable and list the static review performed. ## Self-Assessment -Complete the +Before opening this PR, run through the +[Self-Review Checklist](https://github.com/Axionvera/pocketpay-mobile/blob/main/docs/self-review-checklist.md). +Then complete the [Contributor Self-Assessment](https://github.com/Axionvera/pocketpay-mobile/blob/main/docs/contributor-self-assessment.md) before requesting review. Include concise evidence below; use `Not applicable — ` instead of leaving a required area unexplained. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 192f5be..d5fe87e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -214,14 +214,16 @@ We strive to build a wallet that is accessible to everyone. Before submitting a git push -u origin feat/your-feature-name ``` -6. In the PR description: +6. Before opening the PR, run through the [Self-Review Checklist](docs/self-review-checklist.md) — feature completion, tests, CI, screenshots, and device/emulator verification. + +7. In the PR description: - Summarise what changed and why. - Reference any related issues using `Closes #`. - Describe how you tested the change. - Note any accessibility or security considerations. - For GrantFox mobile issues, review the [Evaluation Readiness Checklist](docs/evaluation-readiness-checklist.md) before the payment evaluation period. A merged PR does not guarantee payment approval. -7. A maintainer will review your PR. Please respond to feedback and update your branch as needed. +8. A maintainer will review your PR. Please respond to feedback and update your branch as needed. --- diff --git a/README.md b/README.md index f5e6c08..2e56ad1 100644 --- a/README.md +++ b/README.md @@ -13,6 +13,7 @@ React Native Expo wallet for Stellar Testnet. The app aims to feel polished and - [Architecture Readiness Review](./docs/architecture-readiness-review.md) - Feature boundaries, duplicated state, SDK integration blockers, security-sensitive areas, and test gaps - [Evaluation Readiness Checklist](./docs/evaluation-readiness-checklist.md) - GrantFox contributor checklist for mobile issues, including tests, CI, screenshots, acceptance criteria, and the reminder that merge does not guarantee payment approval +- [Self-Review Checklist](./docs/self-review-checklist.md) - Quick checklist to run before opening a PR, covering feature completion, tests, CI, screenshots, and device/emulator verification - [Contributor Self-Assessment](./docs/contributor-self-assessment.md) - Pre-review form for confirming scope, test evidence, CI, documentation, limitations, and acceptance criteria - [Storage Guide](./docs/storage.md) - SecureStore vs AsyncStorage - [Test-First Contribution Guide](./docs/test-first-contribution-guide.md) - Required test planning, happy-path and negative-path coverage, no-test justification rules, and local verification commands for mobile PRs @@ -50,6 +51,7 @@ PocketPay Mobile is part of a broader PocketPay stack: - [Screen Inventory](docs/screen-inventory.md) - A map of the main screens and routes in the app. - [Mobile Onboarding Checklist](docs/mobile-onboarding-checklist.md) - Quick-reference setup checklist for new contributors +- [Self-Review Checklist](docs/self-review-checklist.md) - Pre-PR checklist covering feature completion, tests, CI, screenshots, and device/emulator verification - [Evaluation Readiness Checklist](docs/evaluation-readiness-checklist.md) - Mobile-issue review checklist for GrantFox contributors before payment evaluation - [UI State Catalogue](docs/ui-states.md) and [Accessibility Checklist](docs/accessibility.md) - Governance for major-screen states, shared component contracts, and accessible review evidence - [QR Receive Payload Format](docs/qr-payment-requests.md) - The address-only and SEP-0007-based payment-request formats the Receive screen encodes into its QR code diff --git a/docs/meaningful-change-guide.md b/docs/meaningful-change-guide.md new file mode 100644 index 0000000..d46f8dd --- /dev/null +++ b/docs/meaningful-change-guide.md @@ -0,0 +1,135 @@ +# What Counts as Meaningful Work + +A PR that technically merges is not the same as a PR that actually solves an +issue. This guide exists because small mobile PRs can change styling, rename +a variable, or touch a few unrelated lines without addressing the behaviour +an issue actually asked for — and merging does not, by itself, mean the work +is complete or payment-ready. + +> ⚠️ **Merging a PR does not guarantee payment approval.** GrantFox +> evaluation looks at whether the requested behaviour, tests, and acceptance +> criteria were actually delivered — not just whether the diff was accepted. +> See the [Evaluation Readiness Checklist](evaluation-readiness-checklist.md) +> for the full bar, and the +> [Self-Review Checklist](self-review-checklist.md) to check your own work +> against it before you open a PR. + +## The Bar: What "Meaningful" Requires + +A change is meaningful when it addresses **all** of the following for the +issue it claims to close — not just the parts that were easy: + +1. **The requested behaviour**, as written in the issue's Expected Behaviour + and Acceptance Criteria — not a partial or adjacent version of it. +2. **Tests** for the new or changed behaviour. Check the screen's row in the + [Screen Test Matrix](screen-test-matrix.md) — if the screen you touched + has a ❌ or ⚠️ there, "I didn't add a test" is now a visible gap, not an + invisible one. +3. **Screenshots or a screen recording** for any visual change. A reviewer + should not have to pull your branch and run the app just to see what you + built. +4. **Accessibility**, where the change touches UI — see the + [Accessibility Checklist](accessibility.md). A new interactive element + without an `accessibilityLabel` is not a finished element. +5. **Every item in the issue's acceptance criteria**, checked individually + against your actual diff. Partially solving 3 of 6 checkboxes and opening + the PR anyway is a different (and acceptable, if disclosed) thing from + silently claiming the issue is done. + +## Examples of Incomplete Changes + +These are realistic shapes an incomplete PR takes in this codebase — the +common thread is that each one *looks* like it addresses the issue title +without actually delivering the behaviour, tests, or evidence the issue +asked for. + +- **Styling-only fix for a functional bug.** The issue asks for the Send + screen to show a validation error when the amount exceeds the balance; the + PR only restyles the existing (unrelated) error banner and adds no + validation logic or test. The title matches the issue; the diff doesn't. +- **Dead UI.** A new "Copy Address" button is added to the Receive screen, + but it isn't wired to the clipboard — pressing it does nothing. It looks + finished in a screenshot and is not finished at all. +- **Happy-path-only fix for a multi-state issue.** An issue about the Vault + lock list asks for correct behaviour across locked, matured, and withdrawn + states; the PR only handles the locked case and the other two are + silently left broken, with no test and no note that they're out of scope. +- **No test for new behaviour.** A new screen or interactive control is + added with zero corresponding entry in `__tests__/` or `tests/`, and the + [Screen Test Matrix](screen-test-matrix.md) is not updated to reflect the + new screen at all — the next contributor has no way to know the gap + exists. +- **No visual evidence for a visual change.** A PR changes a screen's layout + or adds a new state (loading/empty/error) but includes no screenshot or + recording, so the reviewer either has to trust the description or spend + their own time reproducing it. +- **Accessibility left out.** A new icon-only button (e.g. a delete or copy + action) ships with no `accessible`/`accessibilityLabel`, failing the + [Accessibility Checklist](accessibility.md) minimums silently, because + nothing in CI catches missing accessibility props. +- **Unrelated scope bundled in.** A PR "fixes" the requested issue but also + reformats unrelated files or renames unrelated variables, making the diff + harder to review and harder to evaluate against the issue alone. + +## Examples of Acceptable Changes + +- **A full validation fix.** The Send-screen validation issue above, done + completely: the validation logic is added, the error state matches the + [UI State Catalogue](ui-states.md#send) Error row, a test exercises the + new validation branch (both the failing and passing case), and the PR + description includes a screenshot of the new error state. +- **A new screen, fully landed.** A new screen ships with: a component test, + a new row added to the [Screen Test Matrix](screen-test-matrix.md), + accessible labels on every interactive element, and a screenshot or + recording in the PR description showing it rendered on-device or in a + simulator. +- **An honestly partial PR.** The PR covers wallet creation but not wallet + import from the same issue, and the description says so explicitly — + "Import is intentionally out of scope here; tracked as a follow-up because + X" — instead of implying full coverage. Disclosed partial scope is a + legitimate outcome; undisclosed partial scope is not. +- **A bug fix with a regression test.** The fix includes a test that would + have failed before the fix and passes after it, so the specific bug can't + silently come back. + +## Required Testing Evidence + +Every PR should let a reviewer verify the work **without** re-running it +themselves: + +- The exact commands you ran and their result: `npm run typecheck`, + `npm run lint`, `npm test` (see the + [CI Troubleshooting Guide](ci-troubleshooting.md) if any of them fail for + a reason you can't explain — don't open a PR with a known-failing check). +- For any UI change: a screenshot or short recording of the changed + screen/state. For a multi-state change (e.g. adding an error state), + show each state that changed, not just the happy path. +- For a behavioural fix: which acceptance-criteria checkbox(es) from the + issue each part of your change satisfies. Don't make the reviewer guess + which line addresses which requirement. + +## Reviewer Expectations + +- Reviewers check the PR against the issue's acceptance criteria **line by + line**, not against the PR title or description alone. +- If a claimed behaviour has no test and no screenshot, a reviewer may ask + for one before approving — that's expected, not a sign something went + wrong. +- A green CI run is necessary but not sufficient. CI catches typechecking, + lint, and test failures; it does not catch a validation rule that was + never implemented, or an accessibility label that was never added. +- Scope that's intentionally left out should be stated in the PR + description. Scope that's silently missing is treated as incomplete work, + not as an acceptable interpretation of the issue. + +## See Also + +- [Evaluation Readiness Checklist](evaluation-readiness-checklist.md) — the + full pre-evaluation checklist for GrantFox contract issues. +- [Self-Review Checklist](self-review-checklist.md) — check your own PR + against this guide before opening it. +- [Screen Test Matrix](screen-test-matrix.md) — required test types per + screen. +- [Accessibility Checklist](accessibility.md) and + [UI State Catalogue](ui-states.md) — the concrete bars a UI change is + measured against. diff --git a/docs/self-review-checklist.md b/docs/self-review-checklist.md new file mode 100644 index 0000000..5e874aa --- /dev/null +++ b/docs/self-review-checklist.md @@ -0,0 +1,116 @@ +# Self-Review Checklist + +## Purpose + +A short, actionable checklist to run through **immediately before opening a +PR** — not before payment evaluation. It exists so contributors can catch the +same gaps a reviewer would catch, before a reviewer ever sees the diff. + +This is not a replacement for the +[Evaluation Readiness Checklist](evaluation-readiness-checklist.md) (the full +pre-payment-evaluation bar) or the +[Contributor Self-Assessment](contributor-self-assessment.md) (the form you +paste into the PR description). Use this checklist first, as a fast pass over +your own branch; use the other two afterward, when you're ready to write up +evidence. + +## When To Use This Checklist + +- Right before you push your branch and click "Create Pull Request." +- Again after any substantial change to an already-open PR. + +## 1. Feature Completion + +- [ ] Every item in the issue's Acceptance Criteria is implemented, or its + absence is explicitly disclosed in the PR description. +- [ ] The change matches the issue's Expected Behaviour, not a partial or + adjacent version of it. +- [ ] Every new interactive element is actually wired up — no button that + renders but does nothing when pressed. +- [ ] No unrelated files, renames, or formatting changes are bundled in. + +## 2. Tests + +- [ ] New or changed behaviour has a corresponding automated test, or the PR + explains why a test isn't applicable. +- [ ] Both the happy path and at least one negative/edge case are covered. +- [ ] `npm test` passes locally on the latest commit. + +## 3. CI + +- [ ] `npm run typecheck` passes. +- [ ] `npm run lint` passes. +- [ ] `npm run api:check` passes if the change touches + `src/types/pocketpay-sdk.d.ts` or `src/sdk-stub/` (see + [SDK API Compatibility](sdk-api-compatibility.md)). +- [ ] No required check is skipped, muted, or left failing on the latest + commit. If a check is failing for a reason you can't explain, see the + [CI Troubleshooting Guide](ci-troubleshooting.md) before opening the PR. + +## 4. Screenshots Or Recordings + +- [ ] A screenshot or short recording is attached for any change to visible + UI or user flow. +- [ ] Every state that changed is shown (e.g. loading, error, and success — + not just the happy path). +- [ ] The media shows the actual final result, not a mockup or an earlier + draft of the change. +- [ ] Screenshots contain only Testnet or dummy data — no real keys, + balances, or personal data. +- [ ] If the change is non-visual, the PR says so instead of leaving this + section blank. + +## 5. Device Or Emulator Verification + +- [ ] The change was actually run on a physical device or a simulator/ + emulator — not verified by reading the code alone. +- [ ] The app loads without a red error overlay or crash after the change. +- [ ] The exact flow described in the issue was exercised end-to-end + on-device, including any error or edge-case states the issue calls out. +- [ ] If the change is platform-specific (iOS or Android only), it was + verified on that platform rather than assumed to work from the other. + +## 6. Documentation + +- [ ] Any contributor, user, or architecture documentation affected by the + change is updated, or the PR states why no update is needed. +- [ ] README links, code comments, and examples touched by the change still + match the current behaviour. + +## Examples Of Incomplete Submissions + +Each of these has shipped in a repo looking finished while failing one of the +sections above: + +- A "Copy Address" button that renders correctly in a screenshot but isn't + wired to the clipboard — passes review 4 (screenshot) and fails review 1 + (feature completion). +- A validation fix confirmed only by reading the diff, never launched in a + simulator — fails review 5 even if the logic is correct. +- A new screen with no entry in `__tests__/` or `tests/` — fails review 2 + regardless of how polished the UI looks. +- A multi-state fix (e.g. locked / matured / withdrawn) that only handles one + state, with the other two silently left broken — fails review 1 unless the + gap is disclosed. +- A PR description with an empty or missing Screenshots section on a change + that clearly touches UI — fails review 4 by omission. + +See the [Meaningful Change Guide](meaningful-change-guide.md) for the fuller +set of examples and the reasoning behind them. + +## Final Check + +- [ ] I completed sections 1–6 above for this specific PR, not from memory of + a previous one. +- [ ] I would be comfortable if a reviewer checked every box above against my + diff directly. + +If any box is unchecked, treat the PR as not ready to open. + +## Related Guidance + +- [Contributor Self-Assessment](contributor-self-assessment.md) +- [Evaluation Readiness Checklist](evaluation-readiness-checklist.md) +- [Meaningful Change Guide](meaningful-change-guide.md) +- [CI Troubleshooting Guide](ci-troubleshooting.md) +- [Accessibility Checklist](accessibility.md)