diff --git a/.claude/skills/watch-gradle-tests/SKILL.md b/.claude/skills/watch-gradle-tests/SKILL.md new file mode 100644 index 00000000..6ee0fbc3 --- /dev/null +++ b/.claude/skills/watch-gradle-tests/SKILL.md @@ -0,0 +1,73 @@ +--- +name: watch-gradle-tests +description: Watch a Gradle instrumented-test run to a verdict without hand-writing greps. Use whenever running :app:testEmulatorDebugAndroidTest or :app:connectedDebugAndroidTest in the background. +--- + +# Watching a test run to a verdict + +The emulator suite takes upwards of ten minutes, so it gets backgrounded, so something has to +report on it. **Do not write that something fresh each time.** Use the script. + +```bash +# 1. Start the run, redirected to a file. +./gradlew :app:testEmulatorDebugAndroidTest --console=plain > tmp/run.log 2>&1 + +# 2. Prove the watcher sees the log before trusting it. One command, always. +python .claude/skills/watch-gradle-tests/watch_tests.py tmp/run.log --once + +# 3. Arm a Monitor on the same script with no --once. +python .claude/skills/watch-gradle-tests/watch_tests.py tmp/run.log +``` + +Each line it prints is one event: `FAILED .` as each failure appears, +`STALLED …` if the log stops growing, and `FINISHED` + `VERDICT` at the end. + +## Why this exists + +Three hand-written monitors in one afternoon each matched **nothing**, and each looked like a +green run: + +| What was written | Why it matched nothing | +| --- | --- | +| `grep "(0 skipped)"` | the run had 5 skipped | +| `grep "\S+ \[testEmulator\]"` | there is no space before `[testEmulator]` | +| `... \| head -25` | truncated before the verdict, and `head`'s exit code hid it | + +All three exited 0. **Silence from a monitor is indistinguishable from silence from a healthy +run**, so a red suite was reported as green until the log was read by hand. That is the whole +argument for a script: the patterns get fixed once, and `--once` proves they still match before +anything depends on them. + +## What it knows that a grep does not + +- **The XML has the last word.** The console can truncate, interleave, or count a retry + (`605/600` is a real line from this repo). `verdict_from_xml` reads + `app/build/outputs/androidTest-results/`, and **scopes to the newest run's own directory** — + AGP keeps `connected/` and `managedDevice/` side by side and clears neither, so summing + everything reported a 22-test run as 74. It always prints the results' age, because a verdict + that is quietly ten minutes old is the same bug wearing a coat. +- **No results at all is not zero failures.** A run that dies before any test reports writes no + XML, and reading that as success is exactly how a red build gets called green. +- **A stall is a result.** No output for eight minutes gets reported, with a diagnosis, rather + than looking like a slow test for another twenty. + +## The two silent hangs, which need opposite fixes + +The script separates them by whether *any* test has reported. Do not guess between them — the +distinction is in the output. + +| Symptom | Cause | Fix | +| --- | --- | --- | +| No test ever reports, no managed AVD in `adb devices` | leaked managed-device slots — `MDLockCount` above zero with nothing running | `./gradlew --stop && rm ~/.android/avd/gradle-managed/active_gradle_devices` | +| Tests ran, then stopped mid-suite | the device died under them | `adb logcat -b crash` — the emulator's Bluetooth stack aborting has done this here | +| `connectedDebugAndroidTest` fails to install or hangs at once | stale ADB bridge inside the Gradle daemon | restart the emulator **and** `./gradlew --stop` — either alone leaves it broken | + +Killing a run is what leaks a slot, and every `Ctrl-C` adds one. Prefer letting a run finish; if +you must kill one, clear the count in the same breath rather than meeting it next time. + +## Related + +- `AGENTS.md`, "Building and testing" — the managed device, and why it beats a hand-started + emulator. +- `.claude/skills/watch-pr/` — the same discipline for CI: prove the poll body emits before + arming a Monitor on it. diff --git a/.claude/skills/watch-gradle-tests/watch_tests.py b/.claude/skills/watch-gradle-tests/watch_tests.py new file mode 100644 index 00000000..c45c7392 --- /dev/null +++ b/.claude/skills/watch-gradle-tests/watch_tests.py @@ -0,0 +1,243 @@ +#!/usr/bin/env python +""" +Watch a Gradle instrumented-test run and say what happened. + +Written because hand-rolled greps kept reporting nothing and being believed. Three separate +monitors in one afternoon matched no lines each - one required ``(0 skipped)`` against a run with +five skipped, one required a space before ``[testEmulator]`` that is not there, one truncated +before the verdict with ``head``. Every one of them exited quietly, and quiet reads as green. + +So the patterns live here, once, with ``--once`` to prove they match before anything relies on +them. Nothing about a run should be matched by a regex typed fresh at the call site. + +Usage:: + + python watch_tests.py --once # print what the log says now, and exit + python watch_tests.py # poll, one line per new event, exit when done + +Every line printed is an event worth a notification. Silence means nothing new, and only after +``--once`` has shown the patterns matching something. +""" + +from __future__ import annotations + +import argparse +import glob +import os +import re +import sys +import time +import xml.etree.ElementTree as ET + +# **Anchored on the word FAILED, not on the decorations around it.** The name is followed +# immediately by `[deviceName]` with no space, and the line carries ANSI colour codes; both have +# already broken a hand-written pattern. +FAILED_LINE = re.compile(r"^(dev\.wander\S*) > (\S+?)\[") + +# `(N skipped)` is not always `(0 skipped)`. Capture the numbers rather than matching a shape. +PROGRESS = re.compile(r"Tests (\d+)/(\d+) completed\. \((\d+) skipped\) \((\d+) failed\)") + +# **MULTILINE, or `^` only matches the start of the whole file** and this reports "not yet" +# forever on a finished run. The same omission has already shipped once in redact.py. +TERMINAL = re.compile(r"^BUILD (SUCCESSFUL|FAILED)|^FAILURE: ", re.MULTILINE) + +# Where AGP leaves the authoritative answer. The console can be truncated, retried or interleaved; +# these cannot. +RESULT_XML = "app/build/outputs/androidTest-results/**/*.xml" + +LOCK_FILE = os.path.expanduser("~/.android/avd/gradle-managed/active_gradle_devices") + + +def failures_in(log: str) -> list[str]: + """Every test the console has reported as failed, by name, without duplicates.""" + seen = [] + for line in log.splitlines(): + if "FAILED" not in line: + continue + match = FAILED_LINE.match(line.strip()) + if match: + name = f"{match.group(1).split('.')[-1]}.{match.group(2)}" + if name not in seen: + seen.append(name) + return seen + + +def progress_of(log: str): + """The most recent (done, total, skipped, failed), or None before any test has run.""" + found = PROGRESS.findall(log) + return tuple(int(n) for n in found[-1]) if found else None + + +def leaked_device_locks() -> int: + """ + How many managed-device slots AGP believes are in use. + + Above zero with nothing running means the next run waits forever for a slot and boots no + emulator at all - see AGENTS.md. It is the single most misdiagnosable hang here, because it + produces no output whatsoever. + """ + try: + with open(LOCK_FILE, encoding="utf-8") as handle: + match = re.search(r"MDLockCount\s+(\d+)", handle.read()) + return int(match.group(1)) if match else 0 + except OSError: + return 0 + + +def verdict_from_xml() -> str | None: + """ + The result as the XML reports it, which is the one to trust. + + **Scoped to the newest run's own directory.** AGP keeps ``connected/`` and + ``managedDevice/`` side by side and neither is cleared, so summing everything on disk mixes + this run with whatever ran before it - a 22-test run reported as 74 the first time this was + written, and a stale green would have masked a fresh red exactly as readily. + + The age is always stated. A verdict that is quietly minutes old is the same failure in a + different coat. + + Returns None when no results exist - itself an answer, and a different one from "zero + failures": a run that dies before any test reports writes nothing at all, and reading that as + success is how a red build gets called green. + """ + files = glob.glob(RESULT_XML, recursive=True) + if not files: + return None + + newest = max(files, key=os.path.getmtime) + run_dir = os.path.dirname(newest) + files = [f for f in files if os.path.dirname(f) == run_dir] + + tests = failures = skipped = 0 + named = [] + for path in files: + root = ET.parse(path).getroot() + tests += int(root.get("tests", 0)) + failures += int(root.get("failures", 0)) + int(root.get("errors", 0)) + skipped += int(root.get("skipped", 0)) + for case in root.iter("testcase"): + if list(case.iter("failure")) or list(case.iter("error")): + named.append(f"{case.get('classname', '').split('.')[-1]}.{case.get('name')}") + + age = (time.time() - os.path.getmtime(newest)) / 60 + where = os.path.basename(run_dir) or run_dir + summary = f"{tests} tests, {failures} failed, {skipped} skipped ({where}, {age:.0f} min old)" + return summary if not named else summary + " -> " + ", ".join(named) + + +def describe_now(log: str) -> list[str]: + """Everything the log currently says, for --once.""" + lines = [] + done = progress_of(log) + lines.append(f"progress: {done[0]}/{done[1]}, {done[3]} failed, {done[2]} skipped" + if done else "progress: no test has reported yet") + + found = failures_in(log) + lines.extend(f"FAILED {name}" for name in found) + if not found: + lines.append("failures: none reported on the console") + + lines.append("terminal: " + ("yes" if TERMINAL.search(log) else "not yet")) + lines.append("xml verdict: " + (verdict_from_xml() or "no results written yet")) + + locks = leaked_device_locks() + if locks: + lines.append(f"NOTE MDLockCount is {locks} - if nothing is running, that is the hang") + return lines + + +def diagnose_stall(log: str, age: float) -> list[str]: + """ + Say which of the two silent hangs this is, because the fixes are unrelated. + + Whether any test has reported is what separates them. Nothing at all means the run never got + a device - overwhelmingly a leaked managed-device slot. Tests that ran and then stopped means + the device died under them, which no amount of lock-clearing helps. + """ + done = progress_of(log) + where = f"at {done[0]}/{done[1]}" if done else "before any test ran" + lines = [f"STALLED no output for {age / 60:.0f} min, {where}"] + + locks = leaked_device_locks() + if not done and locks: + lines.append(f"STALLED MDLockCount is {locks} with no test output - almost certainly " + f"leaked managed-device slots. ./gradlew --stop && rm {LOCK_FILE}") + elif done: + lines.append("STALLED tests had been running, so suspect the device rather than the " + "lock: adb logcat -b crash") + return lines + + +def read(path: str) -> str: + try: + with open(path, encoding="utf-8", errors="replace") as handle: + return handle.read() + except OSError: + return "" + + +def final_report(log: str) -> list[str]: + """What to say once the run is over. The XML has the last word, not the console.""" + lines = [] + done = progress_of(log) + if done: + lines.append(f"FINISHED {done[0]}/{done[1]}, {done[3]} failed, {done[2]} skipped") + lines.append("VERDICT " + (verdict_from_xml() + or "NO RESULTS WRITTEN - the run died before any test reported")) + return lines + + +def watch(args) -> int: + """Poll until the run reaches a terminal state, printing each new event once.""" + reported: set[str] = set() + stalled_at = None + started = time.time() + + while True: + log = read(args.logfile) + + for name in failures_in(log): + if name not in reported: + reported.add(name) + print(f"FAILED {name}", flush=True) + + if TERMINAL.search(log): + for line in final_report(log): + print(line, flush=True) + return 0 + + # **A run that stops producing output is a result too.** Left unsaid it is + # indistinguishable from a slow test, which is how ten minutes goes by twice. + age = time.time() - os.path.getmtime(args.logfile) if os.path.exists(args.logfile) else 0 + if age > args.stall_minutes * 60 and stalled_at != int(age // 60): + stalled_at = int(age // 60) + for line in diagnose_stall(log, age): + print(line, flush=True) + + if time.time() - started > 3 * 60 * 60: + print("GIVING UP watched for three hours", flush=True) + return 1 + + time.sleep(args.interval) + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("logfile", help="the file the gradle run is redirected to") + parser.add_argument("--once", action="store_true", + help="print the current state and exit, to prove the patterns match") + parser.add_argument("--interval", type=int, default=40, help="seconds between polls") + parser.add_argument("--stall-minutes", type=float, default=8.0, + help="say so if the log stops growing for this long") + args = parser.parse_args() + + if args.once: + for line in describe_now(read(args.logfile)): + print(line) + return 0 + + return watch(args) + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/ISSUE_TEMPLATE/app-bug.yml b/.github/ISSUE_TEMPLATE/app-bug.yml index 1f42ec60..e7060db5 100644 --- a/.github/ISSUE_TEMPLATE/app-bug.yml +++ b/.github/ISSUE_TEMPLATE/app-bug.yml @@ -8,8 +8,9 @@ body: value: | For the **Android app**. - The most useful thing you can attach is the log. The app can write one for you — see the - box near the bottom, which explains where the button is and how to make it appear. + The most useful thing you can attach is the log. The app writes one for you and takes + the personal details out of it first — see the box near the bottom for where the button + is. - type: input id: version @@ -66,9 +67,9 @@ body: attributes: label: Which exporter made the zip? description: >- - Only if this is about importing or about missing locations. If the ⋮ menu → - **Information** lists what your tags were imported from, copy it from there; otherwise - whichever version you remember downloading, or leave it blank. + Only if this is about importing or about missing locations. The ⋮ menu → + **Information** prints it under the version, and the app's error page prints it too — + copy it from either. **Do not open the zip to find out.** It holds the keys to your tags, newer ones are password-protected on purpose, and unpacking it to read a version line is not worth the risk of what you might then attach. @@ -81,8 +82,9 @@ body: description: | There are two ways to get one, and the first is easier if it is offered: - **If the app showed you an error page with an Export logs button**, use that button. It is - the same log, without any of the setting-up below. + **If the app showed you a page saying "This one is a bug"**, use its **Get the log** + button. It offers to copy the log for pasting into the box below, or to save it as a file + to attach — and it needs none of the setting-up below. **Otherwise** the button is hidden, because most people never need it: @@ -97,10 +99,11 @@ body: keys to your tags: anybody who has it can locate them, and it cannot be un-posted. Nothing that gets asked here needs it. - **This is a raw Android log and nothing is removed from it.** Read it before posting and - take out anything you would not put on a public page: your Apple ID, device names, serial - numbers, anything a notification happened to say. Unlike the desktop exporter's Save logs - button, this one does not redact. + **Both routes clean the log first**, and tell you what they took out — Apple IDs, device + names, serial numbers, coordinates, place names and bundle passwords, replaced by + placeholders that stay consistent so the log still reads. It is best effort, not a + guarantee: it removes what has a recognisable shape, and something unusual on your + account may not. Worth a glance before you post, not a line-by-line audit. Never paste your Apple ID password, a verification code, or a device passcode. No answer requires them. @@ -111,7 +114,7 @@ body: attributes: label: Before you post options: - - label: If I attached a log, I have read it and taken out anything personal I did not want public. + - label: If I attached a log, I have had a look at it and taken out anything personal the app missed. required: true - type: markdown diff --git a/AGENTS.md b/AGENTS.md index fc60f824..a24c5a4a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -156,6 +156,7 @@ Skills carry the longer version of a workflow, so this file can stay short: | `.claude/skills/add-strings/` | any user-facing string — adding, rewording, removing, checking | | `.claude/skills/device-screenshots/` | rendering the UI on the managed device and reading it cheaply | | `.claude/skills/watch-pr/` | watching a pushed PR's checks through to a verdict, and acting on it | +| `.claude/skills/watch-gradle-tests/` | watching a backgrounded emulator suite to a verdict, and telling the silent hangs apart | The test for whether it belongs here rather than in a comment: would somebody hit it *before* reading the code that explains it? Anisette's machine-identity binding is the example — it @@ -314,6 +315,45 @@ has nothing to do with the code. Recovering from that needs *both* an emulator r bridge. A managed device is created fresh per run, so nothing survives to go stale. It is also the only form of this that CI can run unattended. +#### The run sits in `testEmulatorDebugAndroidTest` and never starts a test + +**Check the lock count first.** It is a one-line fix and it looks like four other things: + +```bash +cat ~/.android/avd/gradle-managed/active_gradle_devices # e.g. "MDLockCount 4" +adb devices # no managed AVD listed +``` + +If the count is above zero with no run in flight, that is the fault. Stop the daemons, delete +the file, run again — AGP recreates it and there is nothing in it worth keeping: + +```bash +./gradlew --stop +rm ~/.android/avd/gradle-managed/active_gradle_devices +``` + +**Why it happens:** AGP counts the managed devices it has in flight in that file, and a run that +is interrupted rather than finished never gives its slot back. Each `Ctrl-C`, each killed +background job, each IDE stop button adds one. Once the count reaches the concurrency limit the +next run waits for a slot that nothing will ever release. + +**Why it is worth a heading:** it boots *no emulator at all* and prints nothing while waiting, so +for the first ten minutes it is indistinguishable from a device starting slowly, and after that +from every other reason a run can hang. Three runs were abandoned here diagnosing it as port +contention with a hand-started emulator — which it was not, and `adb devices` said so the whole +time by listing no managed AVD. + +**So prefer letting a run finish over killing it**, and when you do kill one, clear the count in +the same breath rather than meeting it on the next run. + +Two other hangs with the same symptom, so rule them in or out by what they leave behind: + +| What you see | What it is | +| --- | --- | +| No managed AVD in `adb devices`, no test output | the leaked lock above | +| A managed AVD is up, tests ran and then stopped mid-suite | the device died — check `adb logcat -b crash`; the emulator's Bluetooth stack aborting has done this here | +| `connectedDebugAndroidTest` fails to install, or hangs immediately | stale ADB bridge in the daemon — needs *both* an emulator restart and `./gradlew --stop` | + Three consequences worth knowing: - The `aosp-atd` image carries no Play Services, so **a test that touches Maps will not run diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 14d12d86..f1c8f401 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -279,6 +279,23 @@ The device is defined in `testOptions { managedDevices { ... } }` in `app/build. and uses an `aosp-atd` image, which has **no Play Services** — a test that needs Maps would need a `google` image. +**If a run sits in `:app:testEmulatorDebugAndroidTest` and never starts a test**, check the lock +count before anything else: + +```bash +cat ~/.android/avd/gradle-managed/active_gradle_devices # "MDLockCount 4" with nothing running +``` + +AGP counts its in-flight managed devices there, and a run that is killed rather than finished +never gives its slot back — so the count creeps up and eventually every run waits for a slot +nothing will release. It boots no emulator and says nothing, which is why it reads as a slow +start. Stop the daemons, delete the file, run again: + +```bash +./gradlew --stop +rm ~/.android/avd/gradle-managed/active_gradle_devices +``` + > [!WARNING] > `connectedDebugAndroidTest` **uninstalls the app afterwards**, taking its session, settings > and every imported beacon with it. `allowBackup` is false, so on a real device that is gone diff --git a/app/build.gradle.kts b/app/build.gradle.kts index ffb7bf4a..3d7568a3 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -68,6 +68,26 @@ if (requestedAbis != null) { } } +/** + * The short commit this is being built from, or null when that cannot be known. + * + * **For log headers only.** It must never reach `versionName`: the app stamps + * `via: OpenTagViewer.android:` into every bundle it exports, and rule 9 is that + * nothing patches a version at build time, because two artifacts built from one commit would + * then disagree about what produced them. A build description is a different question from a + * product version, and only the first one wants a commit in it. + * + * Null rather than a guess when git is absent or this is not a checkout - a source zip off a + * release tag has no commit to name, and inventing one is worse than saying nothing. + */ +val gitCommit: String? by lazy { + runCatching { + providers.exec { + commandLine("git", "rev-parse", "--short", "HEAD") + }.standardOutput.asText.get().trim().ifEmpty { null } + }.getOrNull() +} + android { namespace = "dev.wander.android.opentagviewer" compileSdk = 35 @@ -79,6 +99,11 @@ android { versionCode = 3 versionName = "1.0.5" + // Null unless a build type sets it - see the debug block. A release is built from a tag + // and its versionName is exactly right, so there is nothing a commit would add; the + // field exists in both variants so code reading it compiles in both. + buildConfigField("String", "BUILD_COMMIT", "null") + testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner" // **Do not add `timeout_msec` here.** It works - a hanging test fails at the cap with @@ -188,6 +213,17 @@ android { applicationIdSuffix = ".debug" versionNameSuffix = "-debug" + // **`-debug` says this is not a release; it does not say which build.** + // `versionName` is a committed literal, so every commit after 1.0.5 reports 1.0.5 + // perfectly confidently - and `build-debug.yml` publishes a debug APK artifact, so + // somebody can be running a build whose version string is months stale. The commit + // is the only thing that identifies such a build, which is exactly the distinction + // the exporter's `describe_build()` draws between a frozen download and a checkout. + buildConfigField( + "String", + "BUILD_COMMIT", + gitCommit?.let { "\"$it\"" } ?: "null") + // Distinct launcher name, otherwise a debug install sits next to a real one // with an identical icon and label and there is no way to tell them apart. manifestPlaceholders["appLabel"] = "OpenTagViewer (debug)" @@ -422,6 +458,15 @@ chaquopy { // desktop rather than reimplemented because what is displayed is what gets // agreed to, and two renderers would eventually show two different documents. "exporter/terms.py", + // Strips personal identifiers out of a log before anybody sends it. Pure stdlib - + // re, Counter, dataclass - and nothing else in exporter/. + // + // Shared for the same reason as terms.py, and more sharply: the wizard's Save + // logs button already runs these rules, and a second set in Java would mean two + // answers to "is my Apple ID in this file". The rules are patterns, so they need + // adding to as new identifiers turn up, and the one that gets forgotten is the + // copy nobody is looking at. + "exporter/redact.py", ) // The package's own test suite is not part of the app. It imports pytest, which is // not in the APK, so it is dead weight that would fail if anything ever touched it. diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/Shot.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/Shot.java new file mode 100644 index 00000000..7ec1b44d --- /dev/null +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/Shot.java @@ -0,0 +1,58 @@ +package dev.wander.android.opentagviewer; + +import android.graphics.Bitmap; +import android.util.Log; + +import androidx.test.platform.app.InstrumentationRegistry; + +import java.io.File; +import java.io.FileOutputStream; + +/** + * A picture of whatever is on the screen, dialogs included. + * + *

Whole-screen, unlike the {@code view.draw(canvas)} pattern used elsewhere. That one + * needs the view to be in the activity's own hierarchy, and a dialog is not - it lives in its + * own window, so drawing the activity produces the page behind it with a hole where the dialog + * should be. {@code UiAutomation} photographs the compositor's output instead, which is what a + * person actually sees. + * + *

Writes into the directory AGP passes as {@code additionalTestOutputDir} and does nothing + * when there isn't one, so it is free in an ordinary run. Names are + * {@code -.png} so {@code .claude/skills/device-screenshots/sheet.py} groups + * them. + * + *

As ever: a screenshot is not an assertion. It explains why a failure looks wrong; it cannot + * fail. Assert the thing that matters as well. + */ +public final class Shot { + + private Shot() {} + + private static final String TAG = "Shot"; + + public static void ofTheScreen(final String name) { + final String dir = InstrumentationRegistry.getArguments() + .getString("additionalTestOutputDir"); + if (dir == null) { + return; + } + + try { + final Bitmap bitmap = + InstrumentationRegistry.getInstrumentation().getUiAutomation().takeScreenshot(); + if (bitmap == null) { + Log.w(TAG, "the screen could not be photographed for " + name); + return; + } + try (FileOutputStream out = + new FileOutputStream(new File(new File(dir), name + ".png"))) { + bitmap.compress(Bitmap.CompressFormat.PNG, 100, out); + } + bitmap.recycle(); + } catch (final Exception e) { + // A screenshot explains a failure; it is never the reason for one. + Log.w(TAG, "could not write " + name, e); + } + } +} diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/db/room/dao/WhichExportersMadeTheseTagsTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/db/room/dao/WhichExportersMadeTheseTagsTest.java new file mode 100644 index 00000000..a9e23dca --- /dev/null +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/db/room/dao/WhichExportersMadeTheseTagsTest.java @@ -0,0 +1,130 @@ +package dev.wander.android.opentagviewer.db.room.dao; + +import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import androidx.room.Room; +import androidx.test.ext.junit.runners.AndroidJUnit4; +import androidx.test.filters.SmallTest; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +import java.util.List; + +import dev.wander.android.opentagviewer.db.room.OpenTagViewerDatabase; +import dev.wander.android.opentagviewer.db.room.entity.Import; + +/** + * Which exporters produced the bundles on this install. + * + *

The question the Information screen used to answer with {@code getMostRecent}. That + * names one producer and reads as though it accounts for every tag on the phone - and importing + * twice is ordinary: a second Mac, a re-export after buying a tag, an old bundle alongside a + * current one. A report saying "exported with 1.3.0" when half the tags came out of 1.1.0 sends + * whoever reads it looking in the wrong place, which is the whole failure this screen exists to + * prevent. + * + *

An in-memory database rather than the device's own, so this says nothing about whatever the + * emulator happens to have imported and cannot disturb it. + */ +@SmallTest +@RunWith(AndroidJUnit4.class) +public class WhichExportersMadeTheseTagsTest { + + private OpenTagViewerDatabase db; + + @Before + public void openAnEmptyOne() { + this.db = Room.inMemoryDatabaseBuilder( + getInstrumentation().getTargetContext(), OpenTagViewerDatabase.class) + .allowMainThreadQueries() + .build(); + } + + @After + public void closeIt() { + if (this.db != null) { + this.db.close(); + } + } + + /** Every producer, not the last one. */ + @Test + public void twoBundlesFromTwoExportersAreBothNamed() { + this.imported("OpenTagViewer.wizard:1.1.0", 1_000L); + this.imported("OpenTagViewer.cli:1.3.0", 2_000L); + + assertEquals( + List.of("OpenTagViewer.cli:1.3.0", "OpenTagViewer.wizard:1.1.0"), + this.dao().getDistinctProducers()); + } + + /** + * Most recently used first, which is not the same as most recently inserted. + * + *

Somebody who imports from 1.3.0, then re-imports an older bundle, then imports from + * 1.3.0 again has three rows and two producers. The ordering is by each producer's newest + * import - so 1.3.0 leads, despite its first row being the oldest of the three. + */ + @Test + public void theOrderIsByEachProducersNewestImport() { + this.imported("OpenTagViewer.wizard:1.3.0", 1_000L); + this.imported("OpenTagViewer.wizard:1.1.0", 2_000L); + this.imported("OpenTagViewer.wizard:1.3.0", 3_000L); + + assertEquals( + List.of("OpenTagViewer.wizard:1.3.0", "OpenTagViewer.wizard:1.1.0"), + this.dao().getDistinctProducers()); + } + + /** The same producer twice is one answer, not two. */ + @Test + public void thesameExporterTwiceIsNamedOnce() { + this.imported("OpenTagViewer.wizard:1.3.0", 1_000L); + this.imported("OpenTagViewer.wizard:1.3.0", 2_000L); + + assertEquals(1, this.dao().getDistinctProducers().size()); + } + + /** + * An export from before {@code via:} existed contributes nothing rather than a blank. + * + *

Null and empty are both real in this column - format 0.0.1 predates the field entirely. + * Letting either through puts "Tags imported from , OpenTagViewer.wizard:1.3.0" on the + * screen, which reads as a rendering bug rather than as an old bundle. + */ + @Test + public void anexportThatNeverRecordedItselfIsNotAnEmptyEntry() { + this.imported(null, 1_000L); + this.imported("", 2_000L); + this.imported("OpenTagViewer.wizard:1.3.0", 3_000L); + + assertEquals(List.of("OpenTagViewer.wizard:1.3.0"), this.dao().getDistinctProducers()); + } + + /** Nothing imported is an empty list, which the screen turns into words of its own. */ + @Test + public void nothingImportedIsEmptyRatherThanNull() { + final List producers = this.dao().getDistinctProducers(); + + assertTrue("expected no producers, got " + producers, producers.isEmpty()); + } + + private ImportDao dao() { + return this.db.importDao(); + } + + private void imported(final String via, final long at) { + this.dao().insert(Import.builder() + .version("0.0.2") + .importedAt(at) + .exportedAt(at) + .sourceUser("someone@example.com") + .exportedVia(via) + .build()); + } +} diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/python/PythonPackagingTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/python/PythonPackagingTest.java index 0f0a500f..b87e1d12 100644 --- a/app/src/androidTest/java/dev/wander/android/opentagviewer/python/PythonPackagingTest.java +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/python/PythonPackagingTest.java @@ -2,6 +2,7 @@ import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; @@ -165,6 +166,34 @@ public void theicloudPipelineIsPackagedAndImports() { } } + /** + * The log redactor imports, and still strips something. + * + *

The one whitelisted module whose absence would be silent and harmful. Everything + * else here fails loudly when it is missing - no sign-in, no import. This one is reached at + * the moment somebody is about to send their log to a public issue, and a caller that + * shrugged off the import error would hand over an unredacted one. So the app refuses to + * produce a log it could not clean, and this is what says the refusal will not be the normal + * case. + * + *

Asserted by running it rather than by importing it: {@code redact} is patterns and a + * {@code Counter}, so an import that resolved against something empty would still be an + * import. The rules themselves are the exporter's to test - {@code python/test/test_redact.py}, + * a different CI job - and duplicating them here would be the second copy this arrangement + * exists to avoid. + */ + @Test + public void theredactorIsPackagedAndRedacts() { + final PyObject redact = Python.getInstance().getModule("exporter.redact"); + assertNotNull("exporter.redact must be importable in the APK", redact); + + final PyObject result = redact.callAttr("redact", "signed in as someone@example.com"); + final String cleaned = result.asList().get(0).toString(); + + assertFalse("the address survived redaction: " + cleaned, + cleaned.contains("someone@example.com")); + } + /** * And it still knows who the exporter is, so the desktop side is unchanged. * diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/WhatTheInformationScreenAnswersTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/WhatTheInformationScreenAnswersTest.java new file mode 100644 index 00000000..c6e9f701 --- /dev/null +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/WhatTheInformationScreenAnswersTest.java @@ -0,0 +1,128 @@ +package dev.wander.android.opentagviewer.ui; + +import static androidx.test.espresso.Espresso.onView; +import static androidx.test.espresso.assertion.ViewAssertions.matches; +import static androidx.test.espresso.matcher.ViewMatchers.isDisplayed; +import static androidx.test.espresso.matcher.ViewMatchers.withId; +import static androidx.test.espresso.matcher.ViewMatchers.withText; +import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.startsWith; + +import android.content.Context; + +import androidx.test.core.app.ActivityScenario; +import androidx.test.ext.junit.runners.AndroidJUnit4; +import androidx.test.filters.LargeTest; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +import java.util.List; + +import dev.wander.android.opentagviewer.Eventually; +import dev.wander.android.opentagviewer.InformationActivity; +import dev.wander.android.opentagviewer.R; +import dev.wander.android.opentagviewer.Shot; +import dev.wander.android.opentagviewer.db.room.OpenTagViewerDatabase; +import dev.wander.android.opentagviewer.db.room.dao.ImportDao; +import dev.wander.android.opentagviewer.db.room.entity.Import; + +/** + * The screen a bug report sends people to, and whether it answers what the report asks. + * + *

Two questions, and until now it answered one. The app version was here; which + * exporter produced the bundle was not - and the only place that lives is {@code + * OPENTAGVIEWER.yml} inside the export zip, a file holding the private keys to somebody's tags + * and one the issue template tells them in bold not to open. Asking a question whose answer is + * inside a file you have told people not to open is asking them to ignore you. + * + *

Both states matter. An install connected straight to an Apple account has no bundle behind + * it at all, and "nothing" is a real answer rather than a gap - it tells a maintainer the + * exporter is not involved, which rules out a whole class of cause. + */ +@LargeTest +@RunWith(AndroidJUnit4.class) +public class WhatTheInformationScreenAnswersTest { + + private static final String AN_EXPORTER = "OpenTagViewer.wizard:1.3.0"; + + private ActivityScenario scenario; + + /** Put back whatever the device had, so this cannot bleed into another test. */ + private List before; + + @Before + public void rememberWhatWasThere() { + this.before = imports().getAll(); + for (final Import existing : this.before) { + imports().delete(existing); + } + } + + @After + public void putItBack() { + if (this.scenario != null) { + this.scenario.close(); + } + for (final Import existing : imports().getAll()) { + imports().delete(existing); + } + for (final Import original : this.before) { + imports().insert(original); + } + } + + /** + * The exporter that made the bundle, on screen, so nobody opens the zip to find it. + */ + @Test + public void awhichExporterMadeTheBundle() { + imports().insert(Import.builder() + .version("0.0.2") + .importedAt(System.currentTimeMillis()) + .exportedAt(System.currentTimeMillis()) + .sourceUser("someone@example.com") + .exportedVia(AN_EXPORTER) + .build()); + + this.scenario = ActivityScenario.launch(InformationActivity.class); + + // The read is a database call on a background thread, so the line arrives after the + // screen does. + Eventually.check(() -> onView(withId(R.id.appImportedFrom)) + .check(matches(withText(containsString(AN_EXPORTER))))); + + // And the version is still the thing above it, which is the other half the report wants. + onView(withId(R.id.appVersion)).check(matches(withText(startsWith("Version")))); + + Shot.ofTheScreen("the_information_screen-imported_from_an_exporter"); + } + + /** + * And "nothing" said out loud, rather than an empty line. + * + *

A blank where an answer should be reads as a bug in this screen. It is not - it is the + * answer for anybody who connected an Apple account instead of importing a zip, and saying + * so rules the exporter out of whatever they are reporting. + */ + @Test + public void bnothingImportedIsAlsoAnAnswer() { + this.scenario = ActivityScenario.launch(InformationActivity.class); + + final Context context = getInstrumentation().getTargetContext(); + Eventually.check(() -> onView(withId(R.id.appImportedFrom)) + .check(matches(withText(context.getString(R.string.imported_from_nothing))))); + onView(withId(R.id.appImportedFrom)).check(matches(isDisplayed())); + + Shot.ofTheScreen("the_information_screen-nothing_imported"); + } + + private static ImportDao imports() { + return OpenTagViewerDatabase + .getInstance(getInstrumentation().getTargetContext().getApplicationContext()) + .importDao(); + } +} diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/error/TheErrorPageIsReportableTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/error/TheErrorPageIsReportableTest.java new file mode 100644 index 00000000..ab49ae4e --- /dev/null +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/error/TheErrorPageIsReportableTest.java @@ -0,0 +1,317 @@ +package dev.wander.android.opentagviewer.ui.error; + +import static androidx.test.espresso.Espresso.onView; +import static androidx.test.espresso.action.ViewActions.click; +import static androidx.test.espresso.action.ViewActions.scrollTo; +import static androidx.test.espresso.assertion.ViewAssertions.matches; +import static androidx.test.espresso.intent.Intents.intended; +import static androidx.test.espresso.intent.Intents.intending; +import static androidx.test.espresso.intent.matcher.IntentMatchers.hasAction; +import static androidx.test.espresso.intent.matcher.IntentMatchers.hasData; +import static androidx.test.espresso.intent.matcher.IntentMatchers.hasExtra; +import static androidx.test.espresso.intent.matcher.IntentMatchers.hasType; +import static androidx.test.espresso.matcher.RootMatchers.isDialog; +import static androidx.test.espresso.matcher.ViewMatchers.isDisplayed; +import static androidx.test.espresso.matcher.ViewMatchers.withId; +import static androidx.test.espresso.matcher.ViewMatchers.withText; +import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation; +import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.anyOf; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.startsWith; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.hamcrest.MatcherAssert.assertThat; + +import android.app.Activity; +import android.content.ClipData; +import android.content.ClipboardManager; +import android.app.Instrumentation.ActivityResult; +import android.content.Intent; + +import androidx.lifecycle.Lifecycle.State; +import androidx.test.core.app.ActivityScenario; +import androidx.test.espresso.intent.Intents; +import androidx.test.ext.junit.runners.AndroidJUnit4; +import androidx.test.filters.LargeTest; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +import dev.wander.android.opentagviewer.Eventually; +import dev.wander.android.opentagviewer.R; +import dev.wander.android.opentagviewer.python.AppDependencies; +import dev.wander.android.opentagviewer.python.LogRedactor; + +/** + * The page somebody lands on when the app cannot say what went wrong. + * + *

Its whole job is to make a report possible, so what it hands over is the thing to test. + * Two questions decide whether it works: does the link carry the template that puts the questions + * in front of the reporter, and can an unredacted log ever leave the app. The second is the + * one with a permanent cost - an Apple ID posted to a public issue cannot be un-posted - and it + * only happens on the path where redaction fails, which is exactly the path a real device will not + * take on demand. + * + *

Hence a fake redactor. {@code AppDependencies.replaceLogRedactor} produces both the working + * case and the broken one; the broken one is not reachable otherwise, because it means Chaquopy + * failing to start. + */ +@LargeTest +@RunWith(AndroidJUnit4.class) +public class TheErrorPageIsReportableTest { + + private static final String A_CAUSE = "KeychainSessionError: No keychain keys are held"; + + /** Something a real logcat would carry and a report must not. */ + private static final String SOMETHING_PERSONAL = "someone@example.com"; + + /** Only the redactor's output carries this, so finding it proves which text went. + * Asserting the absence of the personal string alone would pass on an empty payload. */ + private static final String REDACTED_MARKER = " [cleaned by the redactor]"; + + private ActivityScenario scenario; + + @Before + public void catchTheIntents() { + Intents.init(); + + // **Only what leaves the app, named explicitly.** + // + // "anything that is not ACTION_MAIN" reads as the same thing and is not: the intent that + // launches the activity under test matches it too, so ActivityScenario.launch was + // answered by the stub, the activity never started, and the scenario waited for a RESUMED + // state that could not arrive. A hang rather than a failure, at 0 of 8 tests, with + // nothing in the output naming the cause. + // **Every intent the page can fire, or the real thing launches.** `intending` stubs + // only what it is named, and this class went on stubbing ACTION_CHOOSER after the share + // sheet became a document picker - so a real picker opened over the app and stayed there + // for whatever ran next. + intending(anyOf( + hasAction(Intent.ACTION_VIEW), + hasAction(Intent.ACTION_CREATE_DOCUMENT))) + .respondWith(new ActivityResult(Activity.RESULT_CANCELED, null)); + } + + @After + public void putTheRealOnesBack() { + if (this.scenario != null) { + this.scenario.close(); + } + Intents.release(); + AppDependencies.reset(); + } + + private void open() { + this.scenario = ActivityScenario.launch(ErrorReportActivity.intentFor( + getInstrumentation().getTargetContext(), A_CAUSE)); + } + + /** + * The report button opens the form, with the template that asks the questions. + * + *

Not {@code /issues/new}. GitHub applies a template's labels and questions from its front + * matter; a bare form gives the reporter a blank box and the maintainer an unlabelled issue. + * And GitHub does not error on a wrong template name - it silently serves the blank one - so + * nothing but an assertion notices. + */ + @Test + public void thereportButtonOpensTheTemplatedForm() { + AppDependencies.replaceLogRedactor(log -> new LogRedactor.Redacted(log, "nothing")); + this.open(); + + Eventually.check(() -> onView(withId(R.id.error_report_button)) + .check(matches(isDisplayed()))); + onView(withId(R.id.error_report_button)).perform(click()); + + intended(allOf( + hasAction(Intent.ACTION_VIEW), + hasData(hasToString(IssueReport.NEW_APP_BUG)))); + } + + /** + * And the cause is on screen verbatim, in the words the failure arrived in. + * + *

It is evidence, not prose: a maintainer searches for it, and a reporter pastes it. A + * translated or prettified cause is one nobody can match against a stack trace. + */ + @Test + public void thefailureIsShownAsItArrived() { + AppDependencies.replaceLogRedactor(log -> new LogRedactor.Redacted(log, "nothing")); + this.open(); + + Eventually.check(() -> onView(withId(R.id.error_report_cause)) + .check(matches(withText(A_CAUSE)))); + } + + /** + * Copying puts the redacted text on the clipboard, and not the raw log. + * + *

The fake stands in for {@code exporter.redact}, whose own rules are the exporter's to + * test. What is asserted here is the wiring - and it is asserted on the payload, + * which is the whole point. This test previously checked that a share sheet had opened and + * called itself "the shared log is the redacted one"; it would have stayed green while + * handing over an unredacted log, which is the one outcome that cannot be taken back. + */ + @Test + public void thecopiedLogIsTheRedactedOne() { + AppDependencies.replaceLogRedactor(log -> + new LogRedactor.Redacted( + log.replace(SOMETHING_PERSONAL, "") + REDACTED_MARKER, + "1 email address")); + this.open(); + + this.chooseFromTheLogMenu(R.string.error_report_log_copy); + + final CharSequence copied = theClipboard(); + assertNotNull("nothing was copied", copied); + assertThat(copied.toString(), containsString(REDACTED_MARKER)); + assertThat("the raw log reached the clipboard", + copied.toString(), not(containsString(SOMETHING_PERSONAL))); + } + + /** + * Saving asks the system for somewhere to put it, rather than choosing for the user. + * + *

A file the user picked the location of is one the browser's file picker can find again + * when they go to attach it, which is the entire reason this option exists. A cache file + * handed to a share sheet is not, and Drive and Files do not even appear as targets for one. + */ + @Test + public void thesaveOptionOpensTheDocumentPicker() { + AppDependencies.replaceLogRedactor(log -> new LogRedactor.Redacted(log, "nothing")); + this.open(); + + this.chooseFromTheLogMenu(R.string.error_report_log_save); + + intended(allOf( + hasAction(Intent.ACTION_CREATE_DOCUMENT), + hasType("text/plain"), + hasExtra(Intent.EXTRA_TITLE, "opentagviewer-log.txt"))); + } + + /** + * And there is a way off this page that is on the page. + * + *

The action bar is hidden here, so before Close existed the system back gesture was the + * only exit - no arrow, no X. Fine for anybody who knows that and a dead end for anybody who + * does not, on a screen somebody reaches at the moment they are already stuck. + */ + @Test + public void thereisAWayOutThatIsNotTheBackGesture() { + AppDependencies.replaceLogRedactor(log -> new LogRedactor.Redacted(log, "nothing")); + this.open(); + + Eventually.check(() -> onView(withId(R.id.error_report_close)) + .check(matches(isDisplayed()))); + onView(withId(R.id.error_report_close)).perform(scrollTo(), click()); + + Eventually.check(() -> assertEquals( + State.DESTROYED, this.scenario.getState())); + } + + /** + * Opens the two-way choice and picks one of them. + * + *

The wait between the two is not padding. A dialog animates in, and + * `animationsDisabled` does not stop it - AGP zeroes the window and transition scales and + * leaves `animator_duration_scale` alone, so clicking the instant the builder returns hits a + * row that is still scaling up and less than 90% visible. Espresso calls that a + * PerformException, which reads like the view being wrong rather than early. It passed on a + * fast device and failed on the managed one, which is the usual way round. + */ + private void chooseFromTheLogMenu(final int option) { + Eventually.check(() -> onView(withId(R.id.error_report_share_log)) + .check(matches(isDisplayed()))); + onView(withId(R.id.error_report_share_log)).perform(scrollTo(), click()); + + final String label = getInstrumentation().getTargetContext().getString(option); + Eventually.check(() -> onView(withText(label)).inRoot(isDialog()) + .check(matches(isDisplayed()))); + onView(withText(label)).inRoot(isDialog()).perform(click()); + } + + /** What is on the clipboard, read on the main thread as the framework requires. */ + private static CharSequence theClipboard() { + final CharSequence[] held = new CharSequence[1]; + getInstrumentation().runOnMainSync(() -> { + final ClipboardManager clipboard = getInstrumentation().getTargetContext() + .getSystemService(ClipboardManager.class); + final ClipData clip = clipboard == null ? null : clipboard.getPrimaryClip(); + held[0] = clip == null || clip.getItemCount() == 0 + ? null + : clip.getItemAt(0).getText(); + }); + return held[0]; + } + + /** + * And it says what came out, rather than asking to be trusted. + */ + @Test + public void itsaysWhatTheRedactorRemoved() { + AppDependencies.replaceLogRedactor(log -> + new LogRedactor.Redacted(log, "1 email address, 2 serial numbers")); + this.open(); + + Eventually.check(() -> onView(withId(R.id.error_report_log_note)) + .check(matches(withText(containsString("1 email address, 2 serial numbers"))))); + } + + /** + * A redactor that cannot run means no log at all - never the raw one. + * + *

The case this class exists for. This page is reached because something broke, and + * "Chaquopy did not start" is a candidate - so the redactor failing is not hypothetical, it is + * correlated with being here. Falling back to the unredacted log would put somebody's Apple ID + * on a public issue at the exact moment they are least inclined to read it first, and it + * cannot be taken back. + * + *

So the button is absent, and the page says why rather than leaving a dead control. + */ + @Test + public void arefusedRedactionOffersNoLogAtAll() { + AppDependencies.replaceLogRedactor(log -> null); + this.open(); + + Eventually.check(() -> onView(withId(R.id.error_report_log_note)) + .check(matches(isDisplayed()))); + + onView(withId(R.id.error_report_share_log)).check(matches(not(isDisplayed()))); + onView(withId(R.id.error_report_log_note)).check(matches( + withText(getInstrumentation().getTargetContext() + .getString(R.string.error_report_log_unavailable)))); + } + + /** + * And reporting still works without one, because a report with no log still helps. + */ + @Test + public void reportingIsStillOfferedWithoutALog() { + AppDependencies.replaceLogRedactor(log -> null); + this.open(); + + Eventually.check(() -> onView(withId(R.id.error_report_button)) + .check(matches(isDisplayed()))); + onView(withId(R.id.error_report_button)).perform(click()); + + intended(hasAction(Intent.ACTION_VIEW)); + } + + private static org.hamcrest.Matcher hasToString(final String expected) { + return new org.hamcrest.TypeSafeMatcher<>() { + @Override + protected boolean matchesSafely(final android.net.Uri uri) { + return expected.equals(uri.toString()); + } + + @Override + public void describeTo(final org.hamcrest.Description description) { + description.appendText("a Uri of " + expected); + } + }; + } +} diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/error/WalkingThroughTheErrorPageTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/error/WalkingThroughTheErrorPageTest.java new file mode 100644 index 00000000..08998171 --- /dev/null +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/error/WalkingThroughTheErrorPageTest.java @@ -0,0 +1,188 @@ +package dev.wander.android.opentagviewer.ui.error; + +import static androidx.test.espresso.Espresso.onView; +import static androidx.test.espresso.action.ViewActions.click; +import static androidx.test.espresso.action.ViewActions.scrollTo; +import static androidx.test.espresso.assertion.ViewAssertions.matches; +import static androidx.test.espresso.intent.Intents.intended; +import static androidx.test.espresso.intent.Intents.intending; +import static androidx.test.espresso.intent.matcher.IntentMatchers.hasAction; +import static androidx.test.espresso.matcher.RootMatchers.isDialog; +import static androidx.test.espresso.matcher.ViewMatchers.isDisplayed; +import static androidx.test.espresso.matcher.ViewMatchers.withId; +import static androidx.test.espresso.matcher.ViewMatchers.withText; +import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation; +import static org.hamcrest.Matchers.anyOf; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; + +import android.app.Activity; +import android.app.Instrumentation.ActivityResult; +import android.content.Intent; + +import androidx.test.core.app.ActivityScenario; +import androidx.test.espresso.intent.Intents; +import androidx.test.ext.junit.runners.AndroidJUnit4; +import androidx.test.filters.LargeTest; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +import dev.wander.android.opentagviewer.Eventually; +import dev.wander.android.opentagviewer.R; +import dev.wander.android.opentagviewer.Shot; +import dev.wander.android.opentagviewer.TestPace; +import dev.wander.android.opentagviewer.python.AppDependencies; +import dev.wander.android.opentagviewer.python.LogRedactor; + +/** + * The error page as a person meets it, at a pace a person can follow. + * + *

Its sibling {@code TheErrorPageIsReportableTest} asserts; this one is for watching. + * Six separate assertions each open and close the screen, so run in slow motion they are six + * flickers rather than a journey. This walks the whole thing once, paced with {@link TestPace}, so + * {@code slowMotion=2000} shows what somebody actually sees. + * + *

It still asserts as it goes - a demo that could pass while showing the wrong screen is + * decoration - but the assertions are the ones a viewer is looking at anyway, and the two states + * it covers are the ones that matter: a log that can be shared, and a log that cannot. + * + *

See {@code AGENTS.md} under "Showing a UI test to a person" for how to run it on a device + * with a window. + */ +@LargeTest +@RunWith(AndroidJUnit4.class) +public class WalkingThroughTheErrorPageTest { + + private static final String A_CAUSE = + "KeychainSessionError: No keychain keys are held, so nothing can be decrypted."; + + /** What a real logcat would carry and a public issue must not. */ + private static final String AN_EMAIL = "someone@example.com"; + + private ActivityScenario scenario; + + @Before + public void catchWhatLeavesTheApp() { + Intents.init(); + // **Only what leaves the app, named explicitly.** + // + // "anything that is not ACTION_MAIN" reads as the same thing and is not: the intent that + // launches the activity under test matches it too, so ActivityScenario.launch was + // answered by the stub, the activity never started, and the scenario waited for a RESUMED + // state that could not arrive. A hang rather than a failure, at 0 of 8 tests, with + // nothing in the output naming the cause. + // CANCELED for the picker: RESULT_OK with no data would have the page treat a + // cancelled save as a successful one, which is the opposite of what a stub should model. + intending(hasAction(Intent.ACTION_VIEW)) + .respondWith(new ActivityResult(Activity.RESULT_OK, null)); + intending(hasAction(Intent.ACTION_CREATE_DOCUMENT)) + .respondWith(new ActivityResult(Activity.RESULT_CANCELED, null)); + } + + @After + public void putTheRealOnesBack() { + if (this.scenario != null) { + this.scenario.close(); + } + Intents.release(); + AppDependencies.reset(); + } + + /** + * The whole page, in the order somebody reads it. + * + *

Arrive on a failure nobody can act on, see what it was, see which build and which bundle + * it happened to, learn that the log is cleaned and what came out of it, share it, then open + * the report form. + */ + @Test + public void awholeReportFromAFailureNobodyCanAct0n() { + // Stands in for exporter.redact: takes the address out, and says it did. + AppDependencies.replaceLogRedactor(log -> new LogRedactor.Redacted( + log.replace(AN_EMAIL, ""), "1 email address, 2 device names")); + + this.scenario = ActivityScenario.launch(ErrorReportActivity.intentFor( + getInstrumentation().getTargetContext(), A_CAUSE)); + + // 1. It says plainly that this is a bug rather than something to retry. + Eventually.check(() -> onView(withId(R.id.error_report_title)) + .check(matches(isDisplayed()))); + TestPace.afterAStep(); + + // 2. And what actually failed, verbatim - the thing a maintainer searches for. + onView(withId(R.id.error_report_cause)).check(matches(withText(A_CAUSE))); + TestPace.afterAStep(); + + // 3. The two questions the report form opens with, already answered on screen. This is + // what stops somebody opening their export zip to read a version line out of it. + onView(withId(R.id.error_report_build)) + .check(matches(withText(containsString("OpenTagViewer app")))); + onView(withId(R.id.error_report_imported_from)).check(matches(isDisplayed())); + TestPace.afterAStep(); + + // 4. The log is offered only once it has been through the redactor, and the page says + // what came out rather than asking to be trusted. + Eventually.check(() -> onView(withId(R.id.error_report_share_log)) + .check(matches(isDisplayed()))); + onView(withId(R.id.error_report_log_note)) + .check(matches(withText(containsString("1 email address, 2 device names")))); + Shot.ofTheScreen("the_error_page-log_can_be_shared"); + TestPace.afterAStep(); + + // 5. And it asks which of the two things is wanted, because they are not the same + // thing: pasting into the form's log box, or producing a file to attach. This was a + // share sheet, which served neither - with text and no stream, Drive and Files do not + // appear as targets at all. + onView(withId(R.id.error_report_share_log)).perform(scrollTo(), click()); + onView(withText(getInstrumentation().getTargetContext() + .getString(R.string.error_report_log_save))) + .inRoot(isDialog()).check(matches(isDisplayed())); + Shot.ofTheScreen("the_error_page-how_do_you_want_the_log"); + TestPace.afterAStep(); + + // 6. Saving goes to the document picker, so the file lands where the user chose - which + // is the only place a browser's file picker can find it again. + onView(withText(getInstrumentation().getTargetContext() + .getString(R.string.error_report_log_save))).inRoot(isDialog()).perform(click()); + intended(hasAction(Intent.ACTION_CREATE_DOCUMENT)); + TestPace.afterAStep(); + + // 7. And the report button lands on the form with the questions already in it. + onView(withId(R.id.error_report_button)).perform(scrollTo(), click()); + intended(hasAction(Intent.ACTION_VIEW)); + TestPace.afterAStep(); + } + + /** + * And the same page when the log cannot be cleaned. + * + *

Worth watching rather than only asserting, because the correct behaviour is an + * absence - no share button - and an absence is the kind of thing that looks like a + * layout bug until you know it is deliberate. The page says why, and reporting still works. + */ + @Test + public void andwhatItLooksLikeWhenTheLogCannotBeCleaned() { + AppDependencies.replaceLogRedactor(log -> null); + + this.scenario = ActivityScenario.launch(ErrorReportActivity.intentFor( + getInstrumentation().getTargetContext(), A_CAUSE)); + + Eventually.check(() -> onView(withId(R.id.error_report_log_note)) + .check(matches(withText(getInstrumentation().getTargetContext() + .getString(R.string.error_report_log_unavailable))))); + TestPace.afterAStep(); + + // No button, rather than a button that hands over an unredacted log. + onView(withId(R.id.error_report_share_log)).check(matches(not(isDisplayed()))); + Shot.ofTheScreen("the_error_page-log_cannot_be_cleaned"); + TestPace.afterAStep(); + + // Reporting without a log still helps, so it is still offered. + onView(withId(R.id.error_report_button)).perform(scrollTo(), click()); + intended(hasAction(Intent.ACTION_VIEW)); + TestPace.afterAStep(); + } +} diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/importing/WalkingThroughAnImportThatFailedTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/importing/WalkingThroughAnImportThatFailedTest.java new file mode 100644 index 00000000..3449b77c --- /dev/null +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/importing/WalkingThroughAnImportThatFailedTest.java @@ -0,0 +1,264 @@ +package dev.wander.android.opentagviewer.ui.importing; + +import static androidx.test.espresso.Espresso.onView; +import static androidx.test.espresso.action.ViewActions.click; +import static androidx.test.espresso.action.ViewActions.scrollTo; +import static androidx.test.espresso.assertion.ViewAssertions.matches; +import static androidx.test.espresso.intent.Intents.intended; +import static androidx.test.espresso.intent.Intents.intending; +import static androidx.test.espresso.intent.matcher.IntentMatchers.hasAction; +import static androidx.test.espresso.matcher.RootMatchers.isDialog; +import static androidx.test.espresso.matcher.ViewMatchers.isDisplayed; +import static androidx.test.espresso.matcher.ViewMatchers.withId; +import static androidx.test.espresso.matcher.ViewMatchers.withText; +import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation; +import static org.hamcrest.Matchers.anyOf; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import android.app.Activity; +import android.app.Instrumentation.ActivityResult; +import android.content.Context; +import android.content.Intent; + +import androidx.test.core.app.ActivityScenario; +import androidx.appcompat.app.AlertDialog; +import androidx.test.espresso.Espresso; +import androidx.test.espresso.NoMatchingViewException; +import androidx.test.espresso.intent.Intents; +import androidx.test.ext.junit.runners.AndroidJUnit4; +import androidx.test.filters.LargeTest; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +import java.util.concurrent.atomic.AtomicInteger; + +import dev.wander.android.opentagviewer.Eventually; +import dev.wander.android.opentagviewer.R; +import dev.wander.android.opentagviewer.Shot; +import dev.wander.android.opentagviewer.TestPace; +import dev.wander.android.opentagviewer.python.AppDependencies; +import dev.wander.android.opentagviewer.python.LogRedactor; +import dev.wander.android.opentagviewer.ui.TestHostActivity; +import dev.wander.android.opentagviewer.ui.error.ErrorReportActivity; + +/** + * The two halves of a failed import, side by side, at a pace a person can follow. + * + *

They used to be one screen saying one thing, and that was the bug. Picking a zip + * starts two operations - reading the file and storing the tags, then asking Apple where those + * tags are - and a single error handler covered both. So an Anisette server being down produced + * "Error occurred while importing new devices. Try to restart the app and retry", for an import + * that had succeeded and whose tags were visible in the device list. Issues + * #19 and + * #26 are 34 comments of + * people discovering that for themselves. + * + *

Watching the two in sequence is the point: one says this is a bug, report it, the + * other says your tags are fine, this is probably your Anisette server. If they ever read + * alike again, the fix has been undone. + * + *

Run it slowly on a device with a window - see {@code AGENTS.md}, "Showing a UI test to a + * person". It asserts as it goes, because a demo that can pass while showing the wrong screen is + * decoration. + */ +@LargeTest +@RunWith(AndroidJUnit4.class) +public class WalkingThroughAnImportThatFailedTest { + + /** What a zip failing below the importer actually looks like. */ + private static final String A_ZIP_CAUSE = + "EOFException: Unexpected end of ZLIB input stream"; + + /** And what a first fetch failing looks like - nothing to do with a file. */ + private static final String A_FETCH_CAUSE = + "PythonAppleFindMyException: Anisette server at https://ani.example.com returned 502"; + + private ActivityScenario scenario; + + @Before + public void catchWhatLeavesTheApp() { + Intents.init(); + // Only what leaves the app, named explicitly - a matcher broad enough to catch the + // launch intent stubs ActivityScenario itself and hangs the run. See the note in + // TheErrorPageIsReportableTest. + intending(anyOf(hasAction(Intent.ACTION_VIEW), hasAction(Intent.ACTION_CHOOSER))) + .respondWith(new ActivityResult(Activity.RESULT_OK, null)); + } + + @After + public void putTheRealOnesBack() { + if (this.scenario != null) { + this.scenario.close(); + } + Intents.release(); + AppDependencies.reset(); + } + + /** + * First half: the zip really could not be read, and nothing here can say why. + * + *

This is the only failure that now claims to be about the file, and the only one that + * offers to open a bug report. Everything the reporter would otherwise be asked for is + * already on the screen. + */ + @Test + public void azipNobodyCanReadIsTheOneThingWorthReporting() { + AppDependencies.replaceLogRedactor(log -> new LogRedactor.Redacted( + log.replace("someone@example.com", ""), "1 email address")); + + final Context context = getInstrumentation().getTargetContext(); + this.scenario = ActivityScenario.launch(ErrorReportActivity.intentFor( + context, A_ZIP_CAUSE, R.string.error_report_body_import)); + + // 1. It says this is a bug rather than something to retry. + Eventually.check(() -> onView(withId(R.id.error_report_title)) + .check(matches(isDisplayed()))); + TestPace.afterAStep(); + + // 2. And it describes the right phase. The protocol wording - "something came back that + // this app does not know how to read" - is false here: nothing came back from + // anywhere, somebody chose a file. + onView(withId(R.id.error_report_body)).check(matches(withText( + context.getString(R.string.error_report_body_import)))); + onView(withId(R.id.error_report_body)).check(matches(not(withText( + context.getString(R.string.error_report_body))))); + TestPace.afterAStep(); + + // 3. The failure verbatim, which is what a maintainer searches for. + onView(withId(R.id.error_report_cause)).check(matches(withText(A_ZIP_CAUSE))); + TestPace.afterAStep(); + + // 4. The log, cleaned, and said to have been cleaned. + Eventually.check(() -> onView(withId(R.id.error_report_share_log)) + .check(matches(isDisplayed()))); + onView(withId(R.id.error_report_log_note)) + .check(matches(withText(containsString("1 email address")))); + Shot.ofTheScreen("an_import_that_failed-the_zip_could_not_be_read"); + TestPace.afterAStep(); + + // 5. And the form, with the questions already in it. + onView(withId(R.id.error_report_button)).perform(scrollTo(), click()); + intended(hasAction(Intent.ACTION_VIEW)); + TestPace.afterAStep(); + } + + /** + * Second half: the tags arrived and their locations did not. + * + *

Everything about this is the opposite. The import is not retracted, there is nothing to + * retry, and the likely fix is a setting rather than a bug report - so it names the setting. + * Three reporters across #19 and #26 reached that conclusion unaided, one of them 25 + * comments in. + */ + @Test + public void btagsThatArrivedWithoutTheirLocationsSayExactlyThat() { + final AtomicInteger settingsOpened = new AtomicInteger(); + + final ActivityScenario host = + ActivityScenario.launch(TestHostActivity.class); + this.scenario = host; + + host.onActivity(activity -> ImportedButNotLocatedDialog.show( + activity, A_FETCH_CAUSE, settingsOpened::incrementAndGet)); + + final Context context = getInstrumentation().getTargetContext(); + + // 1. The headline is the reassurance, because the first thing somebody does here is + // look to see whether their tags survived. + onView(withText(context.getString(R.string.imported_but_not_located_title))) + .inRoot(isDialog()).check(matches(isDisplayed())); + TestPace.afterAStep(); + + // 2. It names Anisette, and it names the failure. The app always knew which exception + // it was; it used to throw that away and say "error occurred while importing". + onView(withText(containsString("Anisette"))).inRoot(isDialog()) + .check(matches(isDisplayed())); + onView(withText(containsString(A_FETCH_CAUSE))).inRoot(isDialog()) + .check(matches(isDisplayed())); + Shot.ofTheScreen("an_import_that_failed-the_tags_arrived_the_locations_did_not"); + TestPace.afterAStep(); + + // 3. **No offer to report it.** This fails when somebody else's server is down or a + // phone is on a train, and it retries by itself every minute. Inviting a bug report + // for that fills the tracker with weather. + assertTrue("the fetch failure must not offer a bug report", + nothingOnScreenSays(context.getString(R.string.error_report_button))); + TestPace.afterAStep(); + + // 4. What it offers instead is the setting that actually fixes it. + onView(withText(context.getString(R.string.imported_but_not_located_open_settings))) + .inRoot(isDialog()).perform(click()); + assertEquals(1, settingsOpened.get()); + TestPace.afterAStep(); + } + + /** + * And both of the ways out of it work, without doing anything. + * + *

OK and the back gesture are defaults - {@code MaterialAlertDialogBuilder} makes a + * cancelable dialog and a null listener dismisses - which is exactly why they are pinned. A + * later {@code setCancelable(false)}, added for some other reason, would take the back + * gesture away silently, and nothing else here would notice. + * + *

Neither must open settings. Dismissing is not consent to be sent somewhere. + */ + @Test + public void cthedialogCanBeDismissedBothWays() { + final Context context = getInstrumentation().getTargetContext(); + final AtomicInteger settingsOpened = new AtomicInteger(); + final AlertDialog[] dialog = new AlertDialog[1]; + + final ActivityScenario host = + ActivityScenario.launch(TestHostActivity.class); + this.scenario = host; + + // **Asked of the dialog, not of the view tree.** Written the obvious way - press back, + // then assert the title is not on screen - this class took 15 minutes and still failed: + // proving a view absent means inRoot(isDialog()) against a screen with no dialog, and + // Espresso's root picker retries for seconds before conceding. A boolean is instant, and + // it is also the thing actually being claimed. + host.onActivity(activity -> dialog[0] = ImportedButNotLocatedDialog.show( + activity, A_FETCH_CAUSE, settingsOpened::incrementAndGet)); + Eventually.check(() -> assertTrue("the dialog never appeared", dialog[0].isShowing())); + + // perform, not a bare pressBack: if the dialog has not taken focus yet the key reaches + // the activity instead and finishes it, and Espresso reports NoActivityResumedException + // from the same call that would have worked a moment later. + Eventually.perform("back", () -> !dialog[0].isShowing(), Espresso::pressBack); + + host.onActivity(activity -> dialog[0] = ImportedButNotLocatedDialog.show( + activity, A_FETCH_CAUSE, settingsOpened::incrementAndGet)); + Eventually.check(() -> assertTrue("the dialog never reappeared", dialog[0].isShowing())); + + onView(withText(context.getString(R.string.ok))).inRoot(isDialog()).perform(click()); + Eventually.check(() -> assertFalse("OK did not dismiss the dialog", + dialog[0].isShowing())); + + assertEquals("dismissing must not open settings", 0, settingsOpened.get()); + } + + /** + * Whether a piece of text is absent from the dialog. + * + *

Phrased as a caught {@link NoMatchingViewException} rather than + * {@code matches(not(isDisplayed()))} because the assertion here is that the view does not + * exist - a button never added, not one hidden. The usual warning about expecting + * that exception applies to views that are present and GONE, which is the opposite case. + */ + private static boolean nothingOnScreenSays(final String text) { + try { + onView(withText(text)).inRoot(isDialog()).check(matches(isDisplayed())); + return false; + } catch (final NoMatchingViewException expected) { + return true; + } + } + +} diff --git a/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/mydevices/WhichExporterMadeThisTagTest.java b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/mydevices/WhichExporterMadeThisTagTest.java new file mode 100644 index 00000000..88ee97cd --- /dev/null +++ b/app/src/androidTest/java/dev/wander/android/opentagviewer/ui/mydevices/WhichExporterMadeThisTagTest.java @@ -0,0 +1,163 @@ +package dev.wander.android.opentagviewer.ui.mydevices; + +import static androidx.test.espresso.Espresso.onView; +import static androidx.test.espresso.assertion.ViewAssertions.matches; +import static androidx.test.espresso.matcher.ViewMatchers.hasDescendant; +import static androidx.test.espresso.matcher.ViewMatchers.isDisplayed; +import static androidx.test.espresso.matcher.ViewMatchers.withId; +import static androidx.test.espresso.matcher.ViewMatchers.withText; +import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation; +import static org.hamcrest.Matchers.allOf; + +import android.content.Context; +import android.content.Intent; + +import androidx.test.core.app.ActivityScenario; +import androidx.test.ext.junit.runners.AndroidJUnit4; +import androidx.test.filters.LargeTest; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +import dev.wander.android.opentagviewer.DeviceInfoActivity; +import dev.wander.android.opentagviewer.Eventually; +import dev.wander.android.opentagviewer.R; +import dev.wander.android.opentagviewer.Shot; +import dev.wander.android.opentagviewer.db.room.OpenTagViewerDatabase; +import dev.wander.android.opentagviewer.db.room.entity.BeaconNamingRecord; +import dev.wander.android.opentagviewer.db.room.entity.Import; +import dev.wander.android.opentagviewer.db.room.entity.OwnedBeacon; +import dev.wander.android.opentagviewer.python.AppDependencies; + +/** + * Which exporter produced this tag, on the tag's own page. + * + *

The Information screen cannot answer this, and that is why both exist. It lists every + * producer on the install - correct, and no use when a report is about one tag out of twelve + * behaving oddly. Which program wrote a bundle decides what to expect of the tags in it: whether + * a key alignment record came with them, which format the plists are, whether a known exporter + * bug applies. Per tag, that is a fact; aggregated, it is a shortlist. + * + *

The row costs nothing to fill. This page already reads the {@code Import} for its "Exported + * by" and "Exported at" rows, so {@code exportedVia} was in the object it already had. + */ +@LargeTest +@RunWith(AndroidJUnit4.class) +public class WhichExporterMadeThisTagTest { + + private static final String A_TAG = "test-provenance-tag"; + private static final String A_TEST_USER = "whichexportermadethistag@example.invalid"; + private static final String AN_EXPORTER = "OpenTagViewer.cli:1.3.0"; + + private static final String A_PLIST = "" + + "" + + "batteryLevel1" + + "model" + + "pairingDate2025-02-27T20:03:32Z" + + "privateKeykey" + + "databm90LWEtcmVhbC1rZXk=" + + "productId4660" + + "stableIdentifier2001~#0~#A0" + + "systemVersion1.0" + + "vendorId76" + + ""; + + private OpenTagViewerDatabase db; + private ActivityScenario scenario; + + @Before + public void clearTheDecks() { + this.db = OpenTagViewerDatabase.getInstance(getInstrumentation().getTargetContext()); + this.forgetIt(); + } + + @After + public void putEverythingBack() { + if (this.scenario != null) { + this.scenario.close(); + } + AppDependencies.reset(); + this.forgetIt(); + } + + /** The one this exists for. */ + @Test + public void athetagSaysWhichExporterWroteIt() { + this.seed(AN_EXPORTER); + this.open(); + + Eventually.check(() -> onView(allOf( + withId(R.id.device_settings_exported_with), + hasDescendant(withText(AN_EXPORTER)))) + .check(matches(isDisplayed()))); + + Shot.ofTheScreen("the_device_page-exported_with"); + } + + /** + * And an export from before {@code via:} existed says so, rather than vanishing. + * + *

Format 0.0.1 predates the field, so null is a real value here. Hiding the row would read + * as a gap in the page; saying it was not recorded dates the bundle, which is itself the + * answer to "how old is this export". + */ + @Test + public void banexportTooOldToRecordItselfSaysThatInstead() { + this.seed(null); + this.open(); + + final Context context = getInstrumentation().getTargetContext(); + Eventually.check(() -> onView(allOf( + withId(R.id.device_settings_exported_with), + hasDescendant(withText( + context.getString(R.string.exported_with_something_unrecorded))))) + .check(matches(isDisplayed()))); + } + + private void seed(final String via) { + final long importId = this.db.importDao().insert(Import.builder() + .version("0.0.2") + .importedAt(1_700_000_000_000L) + .exportedAt(1_699_000_000_000L) + .sourceUser(A_TEST_USER) + .exportedVia(via) + .build()); + + this.db.ownedBeaconDao().insertAll(OwnedBeacon.builder() + .id(A_TAG) + .importId(importId) + .content(A_PLIST) + .version("0.0.2") + .fromAccount(false) + .isRemoved(false) + .build()); + + this.db.beaconNamingRecordDao().insertAll(BeaconNamingRecord.builder() + .id(A_TAG) + .importId(importId) + .version("0.0.2") + .isRemoved(false) + .content("" + + "identifier" + A_TAG + "" + + "nameA Tag With A History" + + "") + .build()); + } + + private void open() { + final Intent intent = new Intent( + getInstrumentation().getTargetContext(), DeviceInfoActivity.class); + intent.putExtra("beaconId", A_TAG); + this.scenario = ActivityScenario.launch(intent); + } + + private void forgetIt() { + this.db.ownedBeaconDao().delete(OwnedBeacon.builder().id(A_TAG).build()); + this.db.beaconNamingRecordDao().delete(BeaconNamingRecord.builder().id(A_TAG).build()); + for (final Import stale : this.db.importDao().getImportsFromUser(A_TEST_USER)) { + this.db.importDao().delete(stale); + } + } +} diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml index 76b9d8c0..255ad158 100644 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -67,6 +67,12 @@ android:foregroundServiceType="location"> + + +

Because the issue template asks, and the honest alternative was worse. The + * {@code via:} line is inside {@code OPENTAGVIEWER.yml} in the export zip - a file holding + * the private keys to somebody's tags, which the template tells them in bold not to open. + * Asking a question whose answer is in a file you have told people not to open is asking + * them to ignore you. + * + *

All of them, not the most recent one. Importing twice is ordinary, and naming + * only the newest bundle would state one producer as though it accounted for every tag on + * the phone. Where a report says 1.3.0 and half the tags came out of 1.1.0, whoever reads it + * goes looking in the wrong place - which is the same failure as the import error that named + * the wrong phase, one level up. + * + *

Off the main thread: Room refuses a query on it, so doing this inline would not be slow, + * it would throw. + */ + private void showWhereTheTagsCameFrom() { + final TextView importedFrom = this.findViewById(R.id.appImportedFrom); + if (importedFrom == null) { + return; + } + + var async = Observable.fromCallable(() -> OpenTagViewerDatabase + .getInstance(this.getApplicationContext()) + .importDao().getDistinctProducers()) + .subscribeOn(Schedulers.io()) + .observeOn(AndroidSchedulers.mainThread()) + .subscribe( + producers -> importedFrom.setText(producers.isEmpty() + ? this.getString(R.string.imported_from_nothing) + : this.getString(R.string.imported_from_x, + String.join(", ", producers))), + error -> { + // A line that cannot be read is not worth a broken screen; the + // version above it is still the more important half. + Log.w(TAG, "Could not read where the tags were imported from", error); + importedFrom.setVisibility(View.GONE); + }); + } + /** * Fills the avatar grid from the list bundled at build time. *
diff --git a/app/src/main/java/dev/wander/android/opentagviewer/MapsActivity.java b/app/src/main/java/dev/wander/android/opentagviewer/MapsActivity.java index e04c202a..d8b88d0f 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/MapsActivity.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/MapsActivity.java @@ -49,6 +49,8 @@ import dev.wander.android.opentagviewer.ui.compat.WindowPaddingUtil; import dev.wander.android.opentagviewer.ui.importing.BundlePasscodeDialog; +import dev.wander.android.opentagviewer.ui.importing.ImportOutcome; +import dev.wander.android.opentagviewer.ui.importing.ImportedButNotLocatedDialog; import dev.wander.android.opentagviewer.ui.login.TwoFactorAgainOverlay; import dev.wander.android.opentagviewer.ui.settings.AnisetteUpgradeDialog; import dev.wander.android.opentagviewer.ui.settings.ICloudSetupOfferDialog; @@ -69,6 +71,8 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; +import dev.wander.android.opentagviewer.db.room.entity.Import; +import dev.wander.android.opentagviewer.ui.error.ErrorReportActivity; import dev.wander.android.opentagviewer.db.room.entity.OwnedBeacon; import java.util.Locale; import java.util.HashMap; @@ -97,6 +101,7 @@ import dev.wander.android.opentagviewer.db.util.BeaconCombinerUtil; import dev.wander.android.opentagviewer.python.AccessoryRequest; import dev.wander.android.opentagviewer.python.AppDependencies; +import dev.wander.android.opentagviewer.python.LogRedactor; import dev.wander.android.opentagviewer.ui.BeaconIcon; import dev.wander.android.opentagviewer.python.PythonAppleService; import dev.wander.android.opentagviewer.python.PythonAccountLoginException; @@ -827,25 +832,62 @@ private void onImportFilePicked(Intent data) { private void onImportFilePicked(Intent data, final String passcode) { Log.d(TAG, "File has been picked"); - // combine them into the current list of beaconLocations & show this list + // **Two chains, not one, and the split is the whole point.** + // + // Reading the zip and committing the tags is the import. Going to Apple for their + // locations is a separate operation that happens to run next, and it fails for + // completely unrelated reasons - a network, a session, an Anisette server. Joined into + // one chain they shared an error handler, so every one of those reported "Error + // occurred while importing new devices. Try to restart the app and retry" for an import + // that had already succeeded and could be seen in the device list. + // + // Issues #19 and #26 are 34 comments of people working that out for themselves; three + // of them fixed their "import error" by changing their Anisette server, which is not + // consulted while reading a zip. A marker saying how far the chain got would fix the + // message, but decoupling makes the wrong message impossible to write. var async = this.extractImportedData(data, passcode) .flatMap(this.beaconRepo::addNewImport) - .doOnNext((importData) -> { - this.runOnUiThread(() -> { + .observeOn(AndroidSchedulers.mainThread()) + .subscribe( + importData -> { Toast.makeText( this, this.getString(R.string.loading_location_data_for_x_new_imported_devices, importData.getOwnedBeacons().size()), LENGTH_LONG).show(); + this.locateFreshlyImportedTags(importData); + }, + error -> { + // Nothing was stored, so this really is an import failure and can say so. + // The fetch never runs, and the periodic refresh is still waiting on this. + this.initialFetchComplete = true; + this.onImportFailed(data, error); }); - }) - /* - * Fetching and parsing still run concurrently, but they are merged rather than - * zipped: zip completed as soon as the single-emission parse branch did, which - * cancelled every accessory after the first. See RxFlows#allThen for the full - * account. The two doOnNext handlers write to different maps, so their order - * relative to each other does not matter. - */ - .flatMapCompletable(storedBeacons -> RxFlows.allThen( + } + + /** + * The first fetch for tags that are already on the phone. + * + *

Started by a successful import, and answerable for nothing about it. Whatever + * happens here, the tags are stored and stay stored - so a failure is about locations, and + * the message says that rather than retracting an import that worked. + */ + private void locateFreshlyImportedTags(final ImportData storedBeacons) { + /* + * Fetching and parsing still run concurrently, but they are merged rather than + * zipped: zip completed as soon as the single-emission parse branch did, which + * cancelled every accessory after the first. See RxFlows#allThen for the full + * account. The two doOnNext handlers write to different maps, so their order + * relative to each other does not matter. + */ + // **Deferred, because this is now called from the main thread.** + // + // `subscribeOn` moves the *subscription*, not the construction - and building this + // expression is not free: `combine` streams every beacon into two maps and + // `plistFallbacks` walks them again, both evaluated as arguments before `allThen` is + // even entered. While this hung off the import chain it inherited that chain's IO + // thread; splitting the two handed it to whatever called it, which is the UI thread. + // Nothing here belongs there. + var async = Completable.defer(() -> RxFlows.allThen( // A last pass once everything has landed, for anything the per-accessory // passes below could not resolve. this.updateBeaconGeocodings(), @@ -871,6 +913,9 @@ private void onImportFilePicked(Intent data, final String passcode) { BeaconDataParser.parseAsync(BeaconCombinerUtil.combine(storedBeacons)) .doOnNext(this::addBeaconToCurrent) )) + // **Explicit now that this is its own chain.** It used to inherit whatever thread + // `addNewImport` emitted on; nothing here is safe to start on the main one. + .subscribeOn(Schedulers.io()) .observeOn(AndroidSchedulers.mainThread()) // An import is a first fetch too. Without this the periodic refresh stays disabled // for the rest of the session whenever the app started with nothing stored, which @@ -879,23 +924,92 @@ private void onImportFilePicked(Intent data, final String passcode) { .subscribe(() -> { this.showLastDeviceLocations(); Log.i(TAG, "Finished visualising new location reports!"); - }, error -> { - Log.e(TAG, "Error occurred while importing new devices!", error); - - final ZipImporterException.Reason reason = ZipImporterException.reasonOf(error); - if (reason == ZipImporterException.Reason.LOCKED - || reason == ZipImporterException.Reason.WRONG_PASSCODE) { - // A question, not a failure. The exporter locks bundles by default, so this - // is the ordinary path rather than something having gone wrong. - BundlePasscodeDialog.show( - this, - reason == ZipImporterException.Reason.WRONG_PASSCODE, - code -> this.onImportFilePicked(data, code)); - return; - } + }, error -> this.onFirstFetchAfterImportFailed(error)); + } + /** + * What to say when the zip could not be read, or the tags could not be stored. + * + * @param data the picked file, kept so a locked bundle can be retried with a code rather + * than sending the user back to the picker + */ + private void onImportFailed(final Intent data, final Throwable error) { + Log.e(TAG, "Error occurred while importing new devices!", error); + + switch (ImportOutcome.of(error)) { + case ASK_FOR_THE_PASSCODE: + // A question, not a failure. The exporter locks bundles by default, so this is + // the ordinary path rather than something having gone wrong. + BundlePasscodeDialog.show( + this, + ZipImporterException.reasonOf(error) + == ZipImporterException.Reason.WRONG_PASSCODE, + code -> this.onImportFilePicked(data, code)); + return; + + case REPORT_THE_IMPORT: + // **An import nobody can explain goes to the report page, not to a toast.** + // + // Every named reason has advice worth giving - the wrong file, a damaged zip, a + // passcode. This one has none, and what it used to say was "try to restart the + // app and retry", which cannot help and asks somebody to repeat the thing that + // just failed. + Log.w(TAG, "Import failed for a reason nothing here can name", error); + this.startActivity(ErrorReportActivity.intentFor( + this, describe(error), R.string.error_report_body_import)); + return; + + default: Toast.makeText(this, importFailureMessage(error), LENGTH_LONG).show(); - }); + } + } + + /** + * What to say when the tags are stored and Apple could not be asked where they are. + * + *

Never that the import failed. It did not - the tags are in the database and in + * the device list, and telling somebody otherwise sends them to re-import a bundle that + * imported fine. That was the state of issues #19 and #26. + * + *

The two causes with real handling keep it: a session Apple has stopped accepting goes + * back to sign-in, and one wanting a code gets asked for one. What is left is overwhelmingly + * the Anisette server, which is why the dialog names it - it is the answer three reporters + * arrived at themselves, one of them 25 comments in. + * + *

And no report button, deliberately. The bug page belongs to the zip half. This + * half fails for reasons that are somebody's server being down or their phone being on a + * train, it retries by itself every minute, and inviting a report each time it happens would + * fill the tracker with weather. + */ + private void onFirstFetchAfterImportFailed(final Throwable error) { + Log.e(TAG, "Tags were imported, but their first location fetch failed", error); + + if (isAccountRestoreFailure(error)) { + handleAccountRestoreFailureOnUiThread(); + return; + } + + // Self-handling and asynchronous: it asks Apple whether this session actually wants a + // code, and does nothing if it does not. Harmless to call alongside the dialog. + this.askForACodeIfTheSessionNeedsOne(); + + ImportedButNotLocatedDialog.show( + this, describe(error), this::showSettingsPage); + } + + /** + * The failure in the words it arrived in, for pasting into a report. + * + *

Class name and message rather than a stack trace: the trace is in the log the page + * offers, and a screenful of frames is not something anybody reads off a phone. + */ + private static String describe(final Throwable error) { + if (error == null) { + return "unknown"; + } + return error.getMessage() == null + ? error.getClass().getSimpleName() + : error.getClass().getSimpleName() + ": " + error.getMessage(); } /** @@ -938,29 +1052,84 @@ private void onExportLogsToLocationPicked(@lombok.NonNull Intent data) { return; } - String logLines = LogCollectorUtil.getLastLogs(); - - try (OutputStream os = this.getContentResolver().openOutputStream(writeTarget)) { - BufferedWriter bw = new BufferedWriter(new OutputStreamWriter(os)); - bw.write(logLines); - bw.flush(); - bw.close(); - - Log.d(TAG, "Logs export to " + writeTarget + " complete!"); + // **Off the main thread, which it was not.** Four blocking things happen here - reading + // the database for the import provenance, spawning `logcat` and reading it out, running + // it through the redactor, and writing the file. All of them were on the UI thread. The + // database one would throw outright (Room refuses main-thread queries), and the rest + // were an ANR waiting for a slow enough device. + var async = Observable.fromCallable(() -> { + final Import lastImport = + OpenTagViewerDatabase.getInstance(this.getApplicationContext()) + .importDao().getMostRecent(); + + final String logLines = LogCollectorUtil.getLastLogsWithHeader( + BuildConfig.VERSION_NAME, + BuildConfig.BUILD_COMMIT, + lastImport == null ? null : lastImport.exportedVia); + + // **Redacted here too, and not because this button is more dangerous than + // the other one - because it is the same log.** This wrote a raw logcat + // while the error page's copy went through the redactor, which is two + // answers to "is my Apple ID in this file" from one app. The file from this + // button is the one that historically got attached to issues. + final LogRedactor.Redacted cleaned = + AppDependencies.logRedactor().redact(logLines); + if (cleaned == null) { + // Nothing written, deliberately. A raw log cannot be un-posted, and + // somebody exporting one is on their way to attaching it somewhere. + return new ExportedLog(null, null); + } - Toast.makeText( - this, - R.string.log_file_has_been_exported_successfully, - LENGTH_LONG - ).show(); + try (OutputStream os = + this.getContentResolver().openOutputStream(writeTarget)) { + final BufferedWriter bw = + new BufferedWriter(new OutputStreamWriter(os)); + bw.write(cleaned.getText()); + bw.flush(); + bw.close(); + } + return new ExportedLog(writeTarget, cleaned.getSummary()); + }) + .subscribeOn(Schedulers.io()) + .observeOn(AndroidSchedulers.mainThread()) + .subscribe( + written -> { + if (written.target == null) { + Log.w(TAG, "The log could not be cleaned, so none was written"); + Toast.makeText( + this, + R.string.export_logs_cannot_be_cleaned, + LENGTH_LONG + ).show(); + return; + } - } catch (IOException e) { - Log.e(TAG, "Failed to save file", e); - Toast.makeText( - this, - R.string.failed_to_export_log_file, - LENGTH_LONG - ).show(); + Log.d(TAG, "Logs export to " + written.target + " complete!"); + Toast.makeText( + this, + this.getString( + R.string.export_logs_cleaned, written.summary), + LENGTH_LONG + ).show(); + }, + error -> { + Log.e(TAG, "Failed to save file", error); + Toast.makeText( + this, + R.string.failed_to_export_log_file, + LENGTH_LONG + ).show(); + }); + } + + /** Where the log went and what came out of it, or a null target meaning nothing was written. */ + private static final class ExportedLog { + private final Uri target; + private final String summary; + + private ExportedLog(final Uri target, final String summary) { + this.target = target; + this.summary = summary; } } diff --git a/app/src/main/java/dev/wander/android/opentagviewer/db/room/dao/ImportDao.java b/app/src/main/java/dev/wander/android/opentagviewer/db/room/dao/ImportDao.java index 0a8c5a8d..2fb4dc9a 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/db/room/dao/ImportDao.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/db/room/dao/ImportDao.java @@ -20,6 +20,39 @@ public interface ImportDao { @Query("SELECT * FROM Import WHERE id = :importId") Import getById(long importId); + /** + * The most recent import, or null if nothing has ever been imported. + * + *

Read for one thing: naming the bundle a log came from. Which program wrote an export + * decides what to expect of it, and until this the only way to find out was to open the zip - + * a bad instruction generally and a dangerous one now that bundles are password-protected by + * default. + * + *

Null is a real answer, not a gap: an install connected straight to an Apple account has + * no bundle behind it at all. + */ + @Query("SELECT * FROM Import ORDER BY imported_at DESC LIMIT 1") + Import getMostRecent(); + + /** + * Every distinct exporter that produced a bundle on this install, newest first. + * + *

Distinct, because {@link #getMostRecent()} answers a different question than it + * looks like it does. Importing twice is ordinary - a second Mac, a re-export after a + * new tag, a bundle from an older wizard alongside a current one - and asking only for the + * latest names one producer while implying it accounts for everything on the phone. A report + * saying "exported with 1.3.0" when half the tags came out of 1.1.0 sends whoever reads it + * looking in the wrong place. + * + *

{@code GROUP BY} rather than {@code SELECT DISTINCT}, because the ordering is by + * something not in the result: SQLite rejects an {@code ORDER BY} on a column a + * {@code DISTINCT} query does not select, and {@code MAX(imported_at)} per producer is the + * sort that actually means "most recently used". + */ + @Query("SELECT via FROM Import WHERE via IS NOT NULL AND via != ''" + + " GROUP BY via ORDER BY MAX(imported_at) DESC") + List getDistinctProducers(); + @Insert long insert(Import importData); diff --git a/app/src/main/java/dev/wander/android/opentagviewer/python/AppDependencies.java b/app/src/main/java/dev/wander/android/opentagviewer/python/AppDependencies.java index 94bddce8..4b74cb1b 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/python/AppDependencies.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/python/AppDependencies.java @@ -74,6 +74,16 @@ public interface AnisetteFactory { */ private static HardwareDescriber hardwareDescriber = new ChaquopyHardwareDescriber(); + /** + * Strips personal identifiers out of a log before it is offered to anybody. + * + *

Here for the usual reason and one sharper one: the screen that offers a log is the error + * page, which exists because something already broke. A test of it has to be able to + * produce a working redactor and one that cannot run, and the second is the case that decides + * whether an unredacted log can escape. + */ + private static LogRedactor logRedactor = new ChaquopyLogRedactor(); + /** * Turns coordinates into something a person recognises. * @@ -149,6 +159,10 @@ public static HardwareDescriber hardwareDescriber() { return hardwareDescriber; } + public static LogRedactor logRedactor() { + return logRedactor; + } + public static AnisetteServerTesterService serverTester(final CronetEngine engine) { return serverTesterFactory.apply(engine); } @@ -173,6 +187,11 @@ public static void replaceHardwareDescriber(final HardwareDescriber replacement) hardwareDescriber = replacement; } + @VisibleForTesting + public static void replaceLogRedactor(final LogRedactor replacement) { + logRedactor = replacement; + } + @VisibleForTesting public static void replaceAnisette(final Function replacement) { anisetteFactory = (context, settings, hasSession) -> replacement.apply(settings); @@ -185,6 +204,7 @@ public static void reset() { anisetteFactory = LocalAnisette::new; serverTesterFactory = AnisetteServerTesterService::new; hardwareDescriber = new ChaquopyHardwareDescriber(); + logRedactor = new ChaquopyLogRedactor(); icloudFactory = AppDependencies::openRealICloud; geocoderFactory = (context, locale) -> AddressLookup.through(new Geocoder(context, locale)); diff --git a/app/src/main/java/dev/wander/android/opentagviewer/python/ChaquopyLogRedactor.java b/app/src/main/java/dev/wander/android/opentagviewer/python/ChaquopyLogRedactor.java new file mode 100644 index 00000000..e2fbce00 --- /dev/null +++ b/app/src/main/java/dev/wander/android/opentagviewer/python/ChaquopyLogRedactor.java @@ -0,0 +1,44 @@ +package dev.wander.android.opentagviewer.python; + +import android.util.Log; + +import com.chaquo.python.PyObject; +import com.chaquo.python.Python; + +/** + * {@link LogRedactor} over {@code exporter.redact}, the same module the desktop wizard's + * Save logs button runs. + * + *

Blocking, and needs a started interpreter. Never call it on the main thread. + */ +public class ChaquopyLogRedactor implements LogRedactor { + private static final String TAG = ChaquopyLogRedactor.class.getSimpleName(); + + private static final String MODULE = "exporter.redact"; + + @Override + public Redacted redact(final String log) { + if (log == null) { + return null; + } + + try { + final PyObject module = Python.getInstance().getModule(MODULE); + + // redact() hands back (text, Counter); summarise() turns the second into a sentence. + final PyObject result = module.callAttr("redact", log); + final PyObject cleaned = result.asList().get(0); + final PyObject counts = result.asList().get(1); + + return new Redacted( + cleaned.toString(), + module.callAttr("summarise", counts).toString()); + } catch (final Exception e) { + // **Null, not the log.** The caller is about to hand this to somebody who will attach + // it to a public issue. A redactor that could not run is a reason to withhold the + // file, never a reason to send the unredacted one - see LogRedactor#redact. + Log.w(TAG, "Could not redact the log, so it will not be offered", e); + return null; + } + } +} diff --git a/app/src/main/java/dev/wander/android/opentagviewer/python/LogRedactor.java b/app/src/main/java/dev/wander/android/opentagviewer/python/LogRedactor.java new file mode 100644 index 00000000..b9aca2f5 --- /dev/null +++ b/app/src/main/java/dev/wander/android/opentagviewer/python/LogRedactor.java @@ -0,0 +1,48 @@ +package dev.wander.android.opentagviewer.python; + +import lombok.AllArgsConstructor; +import lombok.Getter; + +/** + * Takes the personal identifiers out of a log before anybody sends it somewhere public. + * + *

The rules live in Python and are shared with the desktop exporter - + * {@code exporter/redact.py}, whitelisted into the APK. Not ported to Java on purpose: the wizard's + * Save logs button already runs them, they are patterns that need adding to as new identifiers turn + * up, and two sets would mean two answers to "is my Apple ID in this file" with only one of them + * being maintained. + * + *

Behind an interface for the usual reason - the real one needs a running interpreter, so a + * screen that called it directly could not be tested without one. + */ +public interface LogRedactor { + + /** A cleaned log, and one line saying what came out of it. */ + @AllArgsConstructor + @Getter + class Redacted { + private final String text; + + /** + * What was removed, in words - "3 email addresses, 1 serial number". + * + *

Shown to the person about to send the file. It is the difference between trusting a + * claim that something was cleaned and being told what was found, and it costs nothing: + * the redactor counts as it goes. + */ + private final String summary; + } + + /** + * @return the redacted log, or null if it could not be redacted. + * + *

Null means do not send this. The caller's job is to withhold the log, not to fall + * back to the raw one - this runs at the moment somebody is about to attach a file to a public + * issue, and the failure mode of guessing wrong is their Apple ID on the internet + * permanently. Refusing is recoverable; the alternative is not. + * + *

It can genuinely fail: the error page that offers this exists because something + * broke, and "Python did not start" is one of the things that might have. + */ + Redacted redact(String log); +} diff --git a/app/src/main/java/dev/wander/android/opentagviewer/ui/error/ErrorReportActivity.java b/app/src/main/java/dev/wander/android/opentagviewer/ui/error/ErrorReportActivity.java new file mode 100644 index 00000000..4cd7c65a --- /dev/null +++ b/app/src/main/java/dev/wander/android/opentagviewer/ui/error/ErrorReportActivity.java @@ -0,0 +1,279 @@ +package dev.wander.android.opentagviewer.ui.error; + +import static android.view.View.GONE; +import static android.widget.Toast.LENGTH_LONG; + +import android.content.ClipData; +import android.content.ClipboardManager; +import android.content.Context; +import android.content.Intent; +import android.net.Uri; +import android.os.Build; +import android.os.Bundle; +import android.util.Log; +import android.widget.Button; +import android.widget.Toast; +import android.widget.TextView; + +import androidx.activity.result.ActivityResultLauncher; +import androidx.activity.result.contract.ActivityResultContracts; +import androidx.appcompat.app.AppCompatActivity; + +import com.google.android.material.dialog.MaterialAlertDialogBuilder; + +import java.io.OutputStream; +import java.io.OutputStreamWriter; +import java.io.Writer; +import java.nio.charset.StandardCharsets; + +import dev.wander.android.opentagviewer.BuildConfig; +import dev.wander.android.opentagviewer.R; +import dev.wander.android.opentagviewer.db.room.OpenTagViewerDatabase; +import dev.wander.android.opentagviewer.db.room.entity.Import; +import dev.wander.android.opentagviewer.python.AppDependencies; +import dev.wander.android.opentagviewer.python.LogRedactor; +import dev.wander.android.opentagviewer.util.LogCollectorUtil; +import dev.wander.android.opentagviewer.util.android.WebLink; +import io.reactivex.rxjava3.android.schedulers.AndroidSchedulers; +import io.reactivex.rxjava3.core.Observable; +import io.reactivex.rxjava3.schedulers.Schedulers; + +/** + * "This one is a bug" - the screen for a failure nobody here can fix. + * + *

Shown only when the app cannot name the cause. An {@code UnhandledProtocolError} from + * Python, or anything that reaches {@code REASON_UNKNOWN}. Never for a rejected passcode, an + * account with no tags, a network that is down, or a session wanting a verification code - each of + * those has a screen that says what to do, and this one would be worse than the advice they + * already give. A page that turns up for ordinary mistakes is one people learn to dismiss, and + * then it is worth nothing on the day it is right. + * + *

It carries the three things a report needs and a person otherwise has to hunt for: which + * build this is, which exporter made their bundle, and the failure verbatim. The last was the + * reason somebody would open an export zip - a file holding their tags' private keys - to read a + * version line out of it. + */ +public class ErrorReportActivity extends AppCompatActivity { + private static final String TAG = ErrorReportActivity.class.getSimpleName(); + + /** What went wrong, in the words the failure arrived in. Never translated - it is evidence. */ + public static final String EXTRA_CAUSE = "cause"; + + /** + * Which explanation to show above the cause. + * + *

Because "something came back that this app cannot read" is false for half the callers. + * It is exactly right for a protocol failure and wrong for a bundle that would not parse - + * nothing came back from anywhere, somebody chose a file. A page that misdescribes what + * happened is worse than a generic one, because the reader corrects for it and stops trusting + * the rest. + */ + public static final String EXTRA_BODY = "body"; + + /** The protocol case: Apple sent something the library does not understand. */ + public static Intent intentFor(final Context context, final String cause) { + return intentFor(context, cause, R.string.error_report_body); + } + + public static Intent intentFor( + final Context context, final String cause, final int bodyRes) { + return new Intent(context, ErrorReportActivity.class) + .putExtra(EXTRA_CAUSE, cause) + .putExtra(EXTRA_BODY, bodyRes); + } + + /** The redacted log, held once prepared so the share button is instant and cannot re-fail. */ + private LogRedactor.Redacted log; + + @Override + protected void onCreate(final Bundle savedInstanceState) { + super.onCreate(savedInstanceState); + this.setContentView(R.layout.activity_error_report); + + if (this.getSupportActionBar() != null) { + this.getSupportActionBar().hide(); + } + + this.findViewById(R.id.error_report_build).setText( + "OpenTagViewer app " + LogCollectorUtil.describeBuild( + BuildConfig.VERSION_NAME, BuildConfig.BUILD_COMMIT)); + + this.findViewById(R.id.error_report_cause) + .setText(this.getIntent().getStringExtra(EXTRA_CAUSE)); + + this.findViewById(R.id.error_report_body).setText(this.getIntent() + .getIntExtra(EXTRA_BODY, R.string.error_report_body)); + + this.findViewById(R.id.error_report_button).setOnClickListener( + v -> WebLink.open(this, IssueReport.NEW_APP_BUG)); + + // finish() rather than a navigate-up: this page is always arrived at from somewhere, and + // that somewhere is where closing it should land - the map, mid-import, wherever it was. + this.findViewById(R.id.error_report_close).setOnClickListener(v -> this.finish()); + + // Nothing to share until the log has been through the redactor, and that is a Python call + // on a screen that exists because something already broke. + this.findViewById(R.id.error_report_share_log).setVisibility(GONE); + + this.prepareTheEvidence(); + } + + /** + * Reads the provenance and the log, off the main thread, and only then offers the log. + * + *

The share button is hidden until there is something safe to share. Redaction runs + * through Chaquopy, and this screen is reached because something failed - "Python did + * not start" being one of the candidates. A button that appeared regardless and then handed + * over a raw logcat would put somebody's Apple ID on a public issue at the moment they are + * least inclined to read it first. + */ + private void prepareTheEvidence() { + final TextView importedFrom = this.findViewById(R.id.error_report_imported_from); + final TextView note = this.findViewById(R.id.error_report_log_note); + final Button share = this.findViewById(R.id.error_report_share_log); + + var async = Observable.fromCallable(() -> { + final Import last = OpenTagViewerDatabase + .getInstance(this.getApplicationContext()).importDao().getMostRecent(); + + final String via = last == null ? null : last.exportedVia; + final String raw = LogCollectorUtil.getLastLogsWithHeader( + BuildConfig.VERSION_NAME, BuildConfig.BUILD_COMMIT, via); + + return new Evidence(via, AppDependencies.logRedactor().redact(raw)); + }) + .subscribeOn(Schedulers.io()) + .observeOn(AndroidSchedulers.mainThread()) + .subscribe( + evidence -> { + importedFrom.setText(evidence.via == null + ? this.getString(R.string.imported_from_nothing) + : this.getString(R.string.imported_from_x, evidence.via)); + + if (evidence.log == null) { + note.setText(R.string.error_report_log_unavailable); + return; + } + + this.log = evidence.log; + note.setText(this.getString( + R.string.error_report_log_cleaned, evidence.log.getSummary())); + share.setVisibility(android.view.View.VISIBLE); + share.setOnClickListener(v -> this.offerTheLog()); + }, + error -> { + Log.w(TAG, "Could not prepare the log for reporting", error); + note.setText(R.string.error_report_log_unavailable); + }); + } + + /** + * Asks which of the two things somebody actually wants, because they are not the same thing. + * + *

It shipped as a share sheet, and a share sheet serves neither well. With text and + * no stream, Drive and Files do not appear as targets at all - so the file half of the sheet + * was simply absent, on a button whose main purpose is producing a file to attach. And the + * copy half depended on whichever clipboard target the phone happened to have. + * + *

Two named choices instead. Attaching a file to a GitHub issue on a phone goes through + * the browser's file picker, which reads storage - so a file has to exist somewhere the user + * chose, which is what the document picker is for. Pasting into the form's + * {@code render: shell} box just wants the clipboard. + */ + private void offerTheLog() { + new MaterialAlertDialogBuilder(this) + .setTitle(R.string.error_report_log_how) + .setItems( + new CharSequence[] { + this.getString(R.string.error_report_log_copy), + this.getString(R.string.error_report_log_save)}, + (dialog, which) -> { + if (which == 0) { + this.copyTheLog(); + } else { + this.saveTheLog(); + } + }) + .setNegativeButton(R.string.cancel, null) + .show(); + } + + /** Straight to the clipboard, for pasting into the form's log box. */ + private void copyTheLog() { + final ClipboardManager clipboard = this.getSystemService(ClipboardManager.class); + if (clipboard == null) { + Toast.makeText(this, R.string.failed_to_export_log_file, LENGTH_LONG).show(); + return; + } + + clipboard.setPrimaryClip(ClipData.newPlainText("OpenTagViewer log", this.log.getText())); + + // **Android 13 shows its own confirmation, and a toast on top of it reads as a bug.** + // Below that there is nothing at all, and silence after a tap is indistinguishable from + // a dead button. + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.TIRAMISU) { + Toast.makeText(this, R.string.error_report_log_copied, LENGTH_LONG).show(); + } + } + + /** + * Somewhere the user picks, through the document picker. + * + *

The same route as Settings' own Export Logs button, deliberately: a file the user chose + * the location of is one the browser's file picker can find again, which a cache file handed + * over by a share sheet is not. + */ + private void saveTheLog() { + this.saveLogLauncher.launch(new Intent(Intent.ACTION_CREATE_DOCUMENT) + .addCategory(Intent.CATEGORY_OPENABLE) + .setType("text/plain") + .putExtra(Intent.EXTRA_TITLE, "opentagviewer-log.txt")); + } + + /** + * Writes the redacted log where the picker said, off the main thread. + * + *

Registered as a field rather than made at click time: registering has to happen before + * the activity is started, so a launcher created inside a click listener throws. + */ + private final ActivityResultLauncher saveLogLauncher = this.registerForActivityResult( + new ActivityResultContracts.StartActivityForResult(), + result -> { + final Uri target = result.getData() == null ? null : result.getData().getData(); + if (result.getResultCode() != RESULT_OK || target == null || this.log == null) { + return; // cancelled, which is not a failure and needs no message + } + + var async = Observable.fromCallable(() -> { + try (OutputStream out = this.getContentResolver() + .openOutputStream(target); + Writer writer = new OutputStreamWriter( + out, StandardCharsets.UTF_8)) { + writer.write(this.log.getText()); + } + return target; + }) + .subscribeOn(Schedulers.io()) + .observeOn(AndroidSchedulers.mainThread()) + .subscribe( + written -> Toast.makeText(this, + R.string.log_file_has_been_exported_successfully, + LENGTH_LONG).show(), + error -> { + Log.w(TAG, "Could not write the log out", error); + Toast.makeText(this, R.string.failed_to_export_log_file, + LENGTH_LONG).show(); + }); + }); + + /** What the background read produced, so the UI thread does one hand-off rather than two. */ + private static final class Evidence { + private final String via; + private final LogRedactor.Redacted log; + + private Evidence(final String via, final LogRedactor.Redacted log) { + this.via = via; + this.log = log; + } + } +} diff --git a/app/src/main/java/dev/wander/android/opentagviewer/ui/error/IssueReport.java b/app/src/main/java/dev/wander/android/opentagviewer/ui/error/IssueReport.java new file mode 100644 index 00000000..7cb1075d --- /dev/null +++ b/app/src/main/java/dev/wander/android/opentagviewer/ui/error/IssueReport.java @@ -0,0 +1,42 @@ +package dev.wander.android.opentagviewer.ui.error; + +import lombok.AccessLevel; +import lombok.NoArgsConstructor; + +/** + * Where the app sends somebody who has hit a bug. + * + *

One constant, because nothing tests a link. The desktop exporter keeps its own as + * {@code GITHUB_ISSUES_LINK} in {@code exporter/version.py} for the same reason: a URL inlined at + * two call sites goes stale at one of them, and the symptom is a worse bug report months later + * with nothing to connect it to the change. + */ +@NoArgsConstructor(access = AccessLevel.PRIVATE) +public final class IssueReport { + + /** + * The template file {@link #NEW_APP_BUG} names, relative to {@code .github/ISSUE_TEMPLATE/}. + * + *

Named separately so a test can check it exists. GitHub does not error on an + * unknown {@code ?template=} - it quietly drops the reporter on a blank issue with none of the + * questions and none of the labels. So renaming the file breaks this with no error anywhere, + * and the only symptom is worse reports, indefinitely. + */ + public static final String TEMPLATE = "app-bug.yml"; + /** + * The issue form for app problems. + * + *

{@code ?template=} and not {@code ?labels=}. Labels in a URL are applied only for + * somebody with permission to label the repository, which a person reporting a bug is not - + * the template's own front matter applies them whoever files. The template is also what puts + * the questions in front of the reporter at all. + * + *

An unauthenticated visitor is redirected to a sign-in page and returned here afterwards, + * so the query survives the round trip. There is no way to file anonymously and GitHub has no + * social sign-in, which is why the screen says an account is needed rather than letting + * somebody discover it after writing everything out. + */ + public static final String NEW_APP_BUG = + "https://github.com/parawanderer/OpenTagViewer/issues/new?template=" + TEMPLATE; + +} diff --git a/app/src/main/java/dev/wander/android/opentagviewer/ui/importing/ImportOutcome.java b/app/src/main/java/dev/wander/android/opentagviewer/ui/importing/ImportOutcome.java new file mode 100644 index 00000000..819a655c --- /dev/null +++ b/app/src/main/java/dev/wander/android/opentagviewer/ui/importing/ImportOutcome.java @@ -0,0 +1,50 @@ +package dev.wander.android.opentagviewer.ui.importing; + +import dev.wander.android.opentagviewer.util.parse.ZipImporterException; + +/** + * What was wrong with an import that did not complete, and so what to say about it. + * + *

Only reachable now for failures that really are the import's. Reading the zip and + * committing the tags is one chain; going to Apple for their locations is another, started only + * once the first has succeeded. They used to be joined, sharing an error handler, so a network + * timeout or a bad Anisette server - neither consulted while reading a zip - reported "Error + * occurred while importing new devices" for an import that had already succeeded and could be + * seen in the device list. Issues + * #19 and + * #26 are 34 comments of + * people working that out for themselves. + * + *

So the trap this replaces is {@code reasonOf} returning {@code UNKNOWN}. It is a reasonable + * answer from a function about zip problems - "not one of mine" - and the caller read it as "an + * unknown import problem", which is a different claim entirely. + */ +public enum ImportOutcome { + + /** Locked, or the code was wrong. A question rather than a failure - ask again. */ + ASK_FOR_THE_PASSCODE, + + /** A named problem with the file, which has advice worth giving. */ + EXPLAIN_THE_FILE, + + /** + * Nothing was stored and nothing here can name why. + * + *

The only case where "the app could not read that file" is true, and now the only case + * that says it. Rare, by construction: the file has already been through the importer's own + * checks, so what is left is something below it going wrong. + */ + REPORT_THE_IMPORT; + + public static ImportOutcome of(final Throwable error) { + switch (ZipImporterException.reasonOf(error)) { + case LOCKED: + case WRONG_PASSCODE: + return ASK_FOR_THE_PASSCODE; + case UNKNOWN: + return REPORT_THE_IMPORT; + default: + return EXPLAIN_THE_FILE; + } + } +} diff --git a/app/src/main/java/dev/wander/android/opentagviewer/ui/importing/ImportedButNotLocatedDialog.java b/app/src/main/java/dev/wander/android/opentagviewer/ui/importing/ImportedButNotLocatedDialog.java new file mode 100644 index 00000000..37d20a1d --- /dev/null +++ b/app/src/main/java/dev/wander/android/opentagviewer/ui/importing/ImportedButNotLocatedDialog.java @@ -0,0 +1,60 @@ +package dev.wander.android.opentagviewer.ui.importing; + +import android.app.Activity; + +import androidx.appcompat.app.AlertDialog; + +import com.google.android.material.dialog.MaterialAlertDialogBuilder; + +import dev.wander.android.opentagviewer.R; + +/** + * "Your tags are here. Where they are is not, yet." + * + *

The message that replaces a two-year-old lie. What used to appear at this moment was + * a toast saying the import had failed and to restart the app - for an import that had committed + * its tags to the database and could be seen in the device list. Three reporters across issues + * #19 and + * #26 resolved that + * "import error" by changing their Anisette server, which tells you both that the message named + * the wrong phase and what the real cause usually is. + * + *

A dialog rather than a toast because the first thing a person does here is look at the + * screen to see whether their tags arrived, and a toast is gone by then. It is also the only + * place the exception is ever put in front of them: the app knew which failure it was and + * replaced it with a sentence about importing, which is why none of those reports can be + * attributed to a cause. + * + *

No report button. This fails when a public Anisette server is down or a phone is on a + * train, and the fetch retries by itself every minute - so it is weather, and asking for a bug + * report about weather fills a tracker with reports nobody can act on. The bug page belongs to + * the half of the import that reads the zip. + */ +public final class ImportedButNotLocatedDialog { + + private ImportedButNotLocatedDialog() {} + + /** + * @param cause the failure in the words it arrived in. Shown, not hidden in the log: + * it is the difference between a report that names a cause and 25 + * comments of guessing. + * @param onSettings opens settings, where the Anisette choice lives + * @return the dialog, so a test can ask whether it is still showing. Asking Espresso to + * prove a view is absent means {@code inRoot(isDialog())} against a screen with no + * dialog, and its root picker retries for seconds before admitting there isn't one. + */ + public static AlertDialog show( + final Activity activity, + final String cause, + final Runnable onSettings) { + + return new MaterialAlertDialogBuilder(activity) + .setTitle(R.string.imported_but_not_located_title) + .setMessage(activity.getString( + R.string.imported_but_not_located_body, cause)) + .setNegativeButton(R.string.ok, null) + .setPositiveButton(R.string.imported_but_not_located_open_settings, + (dialog, which) -> onSettings.run()) + .show(); + } +} diff --git a/app/src/main/java/dev/wander/android/opentagviewer/util/LogCollectorUtil.java b/app/src/main/java/dev/wander/android/opentagviewer/util/LogCollectorUtil.java index a97f6ad8..b1f67fd0 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/util/LogCollectorUtil.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/util/LogCollectorUtil.java @@ -29,4 +29,57 @@ public static String getLastLogs() { throw new RuntimeException(e); } } + + /** + * The log, with a few lines at each end saying what produced it. + * + *

Because the questions a bug report opens with are all answerable here. The issue + * template asks for the app version and for which exporter wrote the bundle, and both were + * things the log already knew and never said - so every report either guessed, or went and + * opened an export zip full of private keys to read one line out of it. + * + *

At both ends, which is not belt and braces. Five hundred lines is more than most + * people paste: somebody who has found the interesting part copies the tail around it, and a + * header is exactly the part that gets left behind. Repeating it costs three lines and means + * either end of an excerpt still says what it came from. + * + *

Kept to what identifies the build and the data, and nothing about the person: no account, + * no tag names, no identifiers. A header that leaked would be worse than none, because it + * arrives above content people have been told to read before posting, in the position they + * skim past as boilerplate. + * + * @param appVersion what the app calls itself - {@code BuildConfig.VERSION_NAME}. + * @param importedVia the {@code via:} of the most recent import, or null if nothing has been + * imported: an account-connected install genuinely has no bundle behind it, + * and saying so is an answer rather than a gap. + */ + /** + * What to call this build in a log, so a report says which one produced it. + * + *

{@code versionName} alone is not the answer on a checkout. It is a committed + * literal, so every commit after a release reports the old version perfectly confidently - + * and {@code build-debug.yml} publishes a debug APK artifact, so somebody can be running a + * build whose version string is months stale. {@code BUILD_COMMIT} is set for debug builds + * only and is what identifies those. + * + *

The same three cases the exporter's {@code describe_build()} distinguishes, for the same + * reason: a release is exactly what its version says, a checkout is its commit, and anything + * without one falls back to the version rather than inventing something. + */ + public static String describeBuild(final String appVersion, final String buildCommit) { + return buildCommit == null ? appVersion : appVersion + " (" + buildCommit + ")"; + } + + public static String getLastLogsWithHeader( + final String appVersion, final String buildCommit, final String importedVia) { + final String what = "OpenTagViewer app " + describeBuild(appVersion, buildCommit) + + " | tags imported from: " + + (importedVia == null ? "nothing - no bundle imported" : importedVia); + + return what + "\n" + + "The last " + NUM_LINES_UP + " lines of this device's log follow, " + + "unfiltered by the app.\n\n" + + getLastLogs() + + "\n" + what + "\n"; + } } diff --git a/app/src/main/java/dev/wander/android/opentagviewer/util/parse/AppleZipImporterUtil.java b/app/src/main/java/dev/wander/android/opentagviewer/util/parse/AppleZipImporterUtil.java index e24b1538..a86afee7 100644 --- a/app/src/main/java/dev/wander/android/opentagviewer/util/parse/AppleZipImporterUtil.java +++ b/app/src/main/java/dev/wander/android/opentagviewer/util/parse/AppleZipImporterUtil.java @@ -614,6 +614,18 @@ private static ImportData convert( private static Import parseImportInfo(final String importInfo) throws JsonProcessingException { OpenTagViewerYamlContent content = YamlParser.MAPPER.readValue(importInfo, OpenTagViewerYamlContent.class); + // **Said once, here, so every log from now on answers it.** + // + // Which program wrote a bundle decides what to expect of it - the wizard, its CLI and + // the app all stamp their own `via:` - and it was stored and never mentioned again. The + // only way a user could answer "which exporter made this zip" was to open the bundle, + // which is a bad instruction generally and a dangerous one now that exports are + // password-protected by default: it walks somebody through decrypting a file holding + // their tags' private keys, on the way to reading one version line. + Log.i(TAG, String.format( + "Importing a bundle of format %s, written by %s", + content.getVersion(), content.getVia())); + return Import.builder() .importedAt(System.currentTimeMillis()) .exportedAt(content.getExportTimestamp()) diff --git a/app/src/main/res/layout/activity_device_info.xml b/app/src/main/res/layout/activity_device_info.xml index c589cd2d..5d32857b 100644 --- a/app/src/main/res/layout/activity_device_info.xml +++ b/app/src/main/res/layout/activity_device_info.xml @@ -37,6 +37,10 @@ name="exportedBy" type="String" /> + + @@ -314,6 +318,14 @@ app:sectionSubtitle="@{exportedBy}" app:title="@{@string/exported_by}" /> + + + + + + + + + + + + + + + + + + + + + + + + + + + + +