mirror of
https://github.com/saphid/frame-control.git
synced 2026-10-06 05:02:50 +02:00
Contact email: don't block Save on a slow send; redact reports still in flight
Second review follow-ups: - Saving returns once the choice is stored; a send already under way picks up the newest change, or the background retry is woken. - A problem report still being sent when its address is removed is logged as <removed>, checked under the same lock the removal holds. - The prompt re-checks the privacy notice after fetching its state. - docs/privacy.md: offline contact changes are sent later by themselves; the prompt never follows straight after the privacy notice. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
1 parent
b8ed53f2ff
commit
3f0f09b138
5 files changed
+79
-24
No files matched your search
+5
-4
@@ -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 |
|
| **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
|
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
|
connected for the first time, and never while or straight after the
|
||||||
showing). **No thanks** hides it for good, and it isn't
|
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**
|
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.
|
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.
|
never sends analytics unless `FRAME_CONTROL_TELEMETRY=1` is set.
|
||||||
|
|
||||||
These switches cover the analytics above. A problem report or a contact email
|
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
|
is sent only because you pressed its Send or Save button, so those still go
|
||||||
when you choose to send them; if you don't, nothing is sent.
|
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
|
## Update checks
|
||||||
|
|
||||||
|
|||||||
@@ -30,6 +30,7 @@ class Contact(Base):
|
|||||||
|
|
||||||
def setUp(self):
|
def setUp(self):
|
||||||
super().setUp()
|
super().setUp()
|
||||||
|
self.addCleanup(fc._removed.clear)
|
||||||
for name, value in (("STATE", tm.STATE / "contact"), ("FILE", tm.STATE / "contact" / "contact.json")):
|
for name, value in (("STATE", tm.STATE / "contact"), ("FILE", tm.STATE / "contact" / "contact.json")):
|
||||||
p = mock.patch.object(fc, name, value)
|
p = mock.patch.object(fc, name, value)
|
||||||
p.start()
|
p.start()
|
||||||
@@ -157,6 +158,33 @@ class Contact(Base):
|
|||||||
self.assertFalse(fc.state()["waiting"])
|
self.assertFalse(fc.state()["waiting"])
|
||||||
self.assertNotIn("me@example.com", tm.SENT.read_text())
|
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
|
# ---- the one-time prompt
|
||||||
|
|
||||||
def test_the_prompt_waits_for_a_working_setup_then_stays_dismissed(self):
|
def test_the_prompt_waits_for_a_working_setup_then_stays_dismissed(self):
|
||||||
|
|||||||
+40
-16
@@ -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
|
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.
|
too: once it has been shown or dismissed it never comes back.
|
||||||
"""
|
"""
|
||||||
|
import calendar
|
||||||
import json
|
import json
|
||||||
import os
|
import os
|
||||||
import re
|
import re
|
||||||
@@ -37,6 +38,8 @@ RETRY_EVERY = 600
|
|||||||
|
|
||||||
_lock = threading.RLock()
|
_lock = threading.RLock()
|
||||||
_send_lock = threading.Lock() # one send at a time, so events reach PostHog in rev order
|
_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
|
_retrier = None
|
||||||
|
|
||||||
|
|
||||||
@@ -87,19 +90,24 @@ def _event(s):
|
|||||||
'action': 'set' if email else 'withdraw', 'rev': s['rev'], 'level': 'contact'}}
|
'action': 'set' if email else 'withdraw', 'rev': s['rev'], 'level': 'contact'}}
|
||||||
|
|
||||||
|
|
||||||
def _send_pending():
|
def _send_pending(block=True):
|
||||||
"""Send the waiting change. True if nothing is left waiting."""
|
"""Send what's waiting, including changes made while sending. True if nothing is left
|
||||||
with _send_lock:
|
waiting. Without block, a send already under way is left to pick up the newest change."""
|
||||||
with _lock:
|
if not _send_lock.acquire(blocking=block):
|
||||||
event = load()['pending']
|
return False
|
||||||
if event is None:
|
try:
|
||||||
return True
|
while True:
|
||||||
try:
|
with _lock:
|
||||||
frame_telemetry.post([event], timeout=30)
|
event = load()['pending']
|
||||||
except frame_telemetry.SendError:
|
if event is None:
|
||||||
return False
|
return True
|
||||||
_sent(event)
|
try:
|
||||||
return True
|
frame_telemetry.post([event], timeout=30)
|
||||||
|
except frame_telemetry.SendError:
|
||||||
|
return False
|
||||||
|
_sent(event)
|
||||||
|
finally:
|
||||||
|
_send_lock.release()
|
||||||
|
|
||||||
|
|
||||||
def _sent(event):
|
def _sent(event):
|
||||||
@@ -119,6 +127,7 @@ def _sent(event):
|
|||||||
def _forget_locally(email):
|
def _forget_locally(email):
|
||||||
"""Take a removed address out of the log of what was sent (contact events and reports)."""
|
"""Take a removed address out of the log of what was sent (contact events and reports)."""
|
||||||
with frame_telemetry._lock:
|
with frame_telemetry._lock:
|
||||||
|
_removed[email.lower()] = time.time()
|
||||||
rows = frame_telemetry._read_lines(frame_telemetry.SENT)
|
rows = frame_telemetry._read_lines(frame_telemetry.SENT)
|
||||||
hit = False
|
hit = False
|
||||||
for e in rows:
|
for e in rows:
|
||||||
@@ -130,6 +139,20 @@ def _forget_locally(email):
|
|||||||
frame_telemetry._write_lines(frame_telemetry.SENT, rows)
|
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'] = '<removed>'
|
||||||
|
|
||||||
|
|
||||||
def save(body):
|
def save(body):
|
||||||
"""Set, change or remove the address and the two choices. An address needs at least one
|
"""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."""
|
choice ticked; an empty address (or neither ticked) removes it and withdraws both."""
|
||||||
@@ -159,8 +182,8 @@ def save(body):
|
|||||||
_forget_locally(old)
|
_forget_locally(old)
|
||||||
except OSError:
|
except OSError:
|
||||||
pass
|
pass
|
||||||
if changed:
|
if changed and not _send_pending(block=False):
|
||||||
_send_pending()
|
_wake.set() # offline, or a send under way that will take this change with it
|
||||||
return state()
|
return state()
|
||||||
|
|
||||||
|
|
||||||
@@ -189,7 +212,8 @@ def start():
|
|||||||
_send_pending()
|
_send_pending()
|
||||||
except Exception:
|
except Exception:
|
||||||
pass
|
pass
|
||||||
time.sleep(RETRY_EVERY)
|
_wake.wait(RETRY_EVERY)
|
||||||
|
_wake.clear()
|
||||||
|
|
||||||
_retrier = threading.Thread(target=loop, name='contact', daemon=True)
|
_retrier = threading.Thread(target=loop, name='contact', daemon=True)
|
||||||
_retrier.start()
|
_retrier.start()
|
||||||
+3
-1
@@ -128,7 +128,9 @@ def send(body):
|
|||||||
except frame_telemetry.SendError as e:
|
except frame_telemetry.SendError as e:
|
||||||
raise ReportError(str(e))
|
raise ReportError(str(e))
|
||||||
try:
|
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:
|
except OSError:
|
||||||
pass # it was sent; failing to log it here mustn't make the person send it again
|
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}).'}
|
return {'id': ref, 'message': f'Sent privately to the Frame Control developer (report {ref}).'}
|
||||||
|
|||||||
+3
-3
@@ -4015,13 +4015,13 @@ async function loadContact() {
|
|||||||
try { renderContact(await api("/api/contact")); } catch { return; }
|
try { renderContact(await api("/api/contact")); } catch { return; }
|
||||||
checkContactPrompt();
|
checkContactPrompt();
|
||||||
}
|
}
|
||||||
// One time only, never on top of the privacy notice, and only once the Frame has connected:
|
// One time only, only once the Frame has connected, and never on top of the privacy notice or
|
||||||
// checked at load and again whenever the Frame connects.
|
// straight after it (two asks in a row is nagging): checked at load and whenever the Frame connects.
|
||||||
async function checkContactPrompt() {
|
async function checkContactPrompt() {
|
||||||
if (!$("contactNotice").hidden || !$("privacyNotice").hidden) return;
|
if (!$("contactNotice").hidden || !$("privacyNotice").hidden) return;
|
||||||
let s;
|
let s;
|
||||||
try { s = await api("/api/contact"); } catch { return; }
|
try { s = await api("/api/contact"); } catch { return; }
|
||||||
if (!s.showPrompt) return;
|
if (!s.showPrompt || !$("contactNotice").hidden || !$("privacyNotice").hidden) return;
|
||||||
$("contactNotice").hidden = false;
|
$("contactNotice").hidden = false;
|
||||||
api("/api/contact/prompt", { prompt: "shown" }).catch(() => {});
|
api("/api/contact/prompt", { prompt: "shown" }).catch(() => {});
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in new issue
Block a user