diff --git a/docs/privacy.md b/docs/privacy.md index 5619075..1ec1755 100644 --- a/docs/privacy.md +++ b/docs/privacy.md @@ -141,8 +141,8 @@ are two separate choices, both off until you tick them: | **The maintainer may contact me with follow-up questions** | Questions about problem reports you send, mostly | You're asked once, in a bar at the top of the page, after the Frame has -connected for the first time (never while the first-run privacy notice is -showing). **No thanks** hides it for good, and it isn't +connected for the first time, and never while or straight after the +first-run privacy notice is showing. **No thanks** hides it for good, and it isn't shown again even if you ignore it. **Contact email** in **Privacy & updates** is where you add, change or remove the address and either choice at any time. @@ -181,8 +181,9 @@ the environment that starts Frame Control. A copy run from a source checkout never sends analytics unless `FRAME_CONTROL_TELEMETRY=1` is set. These switches cover the analytics above. A problem report or a contact email -is only ever sent when you press its Send or Save button, so those still go -when you choose to send them; if you don't, nothing is sent. +is sent only because you pressed its Send or Save button, so those still go +when you choose to send them (a contact change saved while offline is sent +by itself once PostHog can be reached); if you don't, nothing is sent. ## Update checks diff --git a/tests/test_contact.py b/tests/test_contact.py index 330fb3a..52fe010 100644 --- a/tests/test_contact.py +++ b/tests/test_contact.py @@ -30,6 +30,7 @@ class Contact(Base): def setUp(self): super().setUp() + self.addCleanup(fc._removed.clear) for name, value in (("STATE", tm.STATE / "contact"), ("FILE", tm.STATE / "contact" / "contact.json")): p = mock.patch.object(fc, name, value) p.start() @@ -157,6 +158,33 @@ class Contact(Base): self.assertFalse(fc.state()["waiting"]) self.assertNotIn("me@example.com", tm.SENT.read_text()) + def test_a_report_still_sending_when_its_address_is_removed_is_logged_without_it(self): + fc.save({"email": "me@example.com", "followup": True}) + real = tm.post + + def remove_meanwhile(batch, timeout=20): + real(batch, timeout) + fc.save({"email": ""}) # removed while the report is on its way, before it's logged + + with mock.patch.object(tm, "post", side_effect=remove_meanwhile): + fr.send({**REPORT, "contact": "me@example.com", "contactFollowup": True}) + self.assertNotIn("me@example.com", tm.SENT.read_text()) + fc._removed.clear() + fr.send({**REPORT, "contact": "other@example.com", "contactFollowup": True}) + self.assertIn("other@example.com", tm.SENT.read_text()) # other reports are logged as sent + + def test_saving_during_a_slow_send_returns_at_once(self): + busy = fc._send_lock + busy.acquire() + try: + s = fc.save({"email": "me@example.com", "updates": True}) + finally: + busy.release() + self.assertTrue(s["waiting"]) # left for the send under way (or the retry) to take + self.assertEqual(self.got, []) + self.assertTrue(fc._send_pending()) + self.assertEqual(len(self.got), 1) + # ---- the one-time prompt def test_the_prompt_waits_for_a_working_setup_then_stays_dismissed(self): diff --git a/ui/frame_contact.py b/ui/frame_contact.py index 0a7eb87..3586337 100644 --- a/ui/frame_contact.py +++ b/ui/frame_contact.py @@ -18,6 +18,7 @@ A change that can't be sent (offline) waits in the state file and is retried in background, so a withdrawal is never lost. The page's one-time prompt is remembered here too: once it has been shown or dismissed it never comes back. """ +import calendar import json import os import re @@ -37,6 +38,8 @@ RETRY_EVERY = 600 _lock = threading.RLock() _send_lock = threading.Lock() # one send at a time, so events reach PostHog in rev order +_removed = {} # address (lower case) -> when it was removed, for reports still being sent then +_wake = threading.Event() _retrier = None @@ -87,19 +90,24 @@ def _event(s): 'action': 'set' if email else 'withdraw', 'rev': s['rev'], 'level': 'contact'}} -def _send_pending(): - """Send the waiting change. True if nothing is left waiting.""" - with _send_lock: - with _lock: - event = load()['pending'] - if event is None: - return True - try: - frame_telemetry.post([event], timeout=30) - except frame_telemetry.SendError: - return False - _sent(event) - return True +def _send_pending(block=True): + """Send what's waiting, including changes made while sending. True if nothing is left + waiting. Without block, a send already under way is left to pick up the newest change.""" + if not _send_lock.acquire(blocking=block): + return False + try: + while True: + with _lock: + event = load()['pending'] + if event is None: + return True + try: + frame_telemetry.post([event], timeout=30) + except frame_telemetry.SendError: + return False + _sent(event) + finally: + _send_lock.release() def _sent(event): @@ -119,6 +127,7 @@ def _sent(event): def _forget_locally(email): """Take a removed address out of the log of what was sent (contact events and reports).""" with frame_telemetry._lock: + _removed[email.lower()] = time.time() rows = frame_telemetry._read_lines(frame_telemetry.SENT) hit = False for e in rows: @@ -130,6 +139,20 @@ def _forget_locally(email): frame_telemetry._write_lines(frame_telemetry.SENT, rows) +def redact_removed(event): + """Before logging a report sent while its address was being removed: take the address out. + Call with frame_telemetry._lock held, so a removal can't slip between this and the log.""" + p = event.get('properties') or {} + removed_at = _removed.get(str(p.get('contact') or '').strip().lower()) + if removed_at is not None: + try: + started = calendar.timegm(time.strptime(event['timestamp'], '%Y-%m-%dT%H:%M:%SZ')) + except (KeyError, ValueError): + started = 0 + if started <= removed_at: + p['contact'] = '' + + def save(body): """Set, change or remove the address and the two choices. An address needs at least one choice ticked; an empty address (or neither ticked) removes it and withdraws both.""" @@ -159,8 +182,8 @@ def save(body): _forget_locally(old) except OSError: pass - if changed: - _send_pending() + if changed and not _send_pending(block=False): + _wake.set() # offline, or a send under way that will take this change with it return state() @@ -189,7 +212,8 @@ def start(): _send_pending() except Exception: pass - time.sleep(RETRY_EVERY) + _wake.wait(RETRY_EVERY) + _wake.clear() _retrier = threading.Thread(target=loop, name='contact', daemon=True) _retrier.start() diff --git a/ui/frame_report.py b/ui/frame_report.py index a39d36d..1c3f8f2 100644 --- a/ui/frame_report.py +++ b/ui/frame_report.py @@ -128,7 +128,9 @@ def send(body): except frame_telemetry.SendError as e: raise ReportError(str(e)) try: - frame_telemetry.record_sent([event]) + with frame_telemetry._lock: # the lock a removal holds while wiping its address + frame_contact.redact_removed(event) + frame_telemetry.record_sent([event]) except OSError: pass # it was sent; failing to log it here mustn't make the person send it again return {'id': ref, 'message': f'Sent privately to the Frame Control developer (report {ref}).'} diff --git a/ui/index.html b/ui/index.html index df27d06..972a867 100644 --- a/ui/index.html +++ b/ui/index.html @@ -4015,13 +4015,13 @@ async function loadContact() { try { renderContact(await api("/api/contact")); } catch { return; } checkContactPrompt(); } -// One time only, never on top of the privacy notice, and only once the Frame has connected: -// checked at load and again whenever the Frame connects. +// One time only, only once the Frame has connected, and never on top of the privacy notice or +// straight after it (two asks in a row is nagging): checked at load and whenever the Frame connects. async function checkContactPrompt() { if (!$("contactNotice").hidden || !$("privacyNotice").hidden) return; let s; try { s = await api("/api/contact"); } catch { return; } - if (!s.showPrompt) return; + if (!s.showPrompt || !$("contactNotice").hidden || !$("privacyNotice").hidden) return; $("contactNotice").hidden = false; api("/api/contact/prompt", { prompt: "shown" }).catch(() => {}); }