mirror of
https://github.com/saphid/frame-control.git
synced 2026-10-06 09:00:35 +02:00
Contact email: follow-up given with a report is kept and removable; match by rev
- Ticking follow-up questions on a report makes that address the contact email (follow-up ticked, update choice unchanged), so Settings shows it and Remove my email withdraws it like any other. - Reports carry contact_rev; the inbox takes a report's follow-up permission back when a later change from that copy (higher rev) no longer agrees, whatever the clocks say. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
1 parent
01d5c612c0
commit
08d75e3ffb
6 files changed
+117
-75
No files matched your search
+10
-6
@@ -107,9 +107,12 @@ wrote, a short reference shown after sending, and the diagnostics below. Your
|
||||
email address goes with it only if you tick **The maintainer may contact me
|
||||
with follow-up questions** (the report then carries `contact_followup: true`);
|
||||
it's filled in from **Contact email** below when you've agreed there. It has its own random id, so it isn't linked to
|
||||
your analytics events. With that box ticked it also carries this copy's contact
|
||||
id (`contact_id`, see below), so removing or changing the address later takes
|
||||
back the follow-up permission given with the report too.
|
||||
your analytics events. With that box ticked, the address also becomes your
|
||||
**Contact email** below with follow-up questions ticked (your update choice
|
||||
stays as it was), so you remove it there like any other. The report then also
|
||||
carries this copy's contact id and change number (`contact_id`, `contact_rev`,
|
||||
see below), so removing or changing the address later takes back the
|
||||
follow-up permission given with the report too.
|
||||
|
||||
With **Include diagnostics** ticked (the default), the report adds:
|
||||
|
||||
@@ -167,9 +170,10 @@ log (in earlier contact events and problem reports), and sends a `withdraw`
|
||||
event with no address in it. The maintainer's list only uses the newest event from each copy, so from
|
||||
then on the address isn't listed for either choice. Unticking one choice
|
||||
works the same way for that choice. This also covers problem reports you sent
|
||||
from this copy with follow-up questions ticked: if your newest choice no longer
|
||||
agrees to follow-up questions at that address, the maintainer's inbox shows the
|
||||
permission as withdrawn and leaves the address out. If you're offline, the change waits on
|
||||
from this copy with follow-up questions ticked: if your newest choice since the
|
||||
report (by change number, not the clock) no longer agrees to follow-up
|
||||
questions at that address, the maintainer's inbox shows the permission as
|
||||
withdrawn and leaves the address out. If you're offline, the change waits on
|
||||
this computer and is sent when PostHog can be reached. The earlier event
|
||||
stays in PostHog until its data retention removes it; to have it deleted
|
||||
sooner, ask the maintainer (for example in a problem report).
|
||||
|
||||
+64
-41
@@ -249,69 +249,92 @@ class Contact(Base):
|
||||
|
||||
# ---- reports and the maintainer's list
|
||||
|
||||
def reports(self):
|
||||
return [e["properties"] for e in self.events() if e["event"] == "problem_report"]
|
||||
|
||||
def test_a_report_carries_the_address_only_with_follow_up_consent(self):
|
||||
fr.send({**REPORT, "contact": "me@example.com"})
|
||||
self.assertFalse(fc.FILE.exists()) # no follow-up: nothing kept, nothing linked
|
||||
fr.send({**REPORT, "contact": "me@example.com", "contactFollowup": True})
|
||||
without, with_ = (e["properties"] for e in self.events())
|
||||
self.assertEqual((without["contact"], without["contact_followup"]), ("", False))
|
||||
without, with_ = self.reports()
|
||||
self.assertEqual((without["contact"], without["contact_followup"], without["contact_id"]), ("", False, ""))
|
||||
self.assertEqual((with_["contact"], with_["contact_followup"]), ("me@example.com", True))
|
||||
self.assertEqual((with_["contact_id"], with_["contact_rev"]), (fc.load()["id"], fc.load()["rev"]))
|
||||
self.assertNotEqual(with_["contact_id"], tm.settings()["id"]) # not the analytics id
|
||||
with self.assertRaisesRegex(ValueError, "email address"):
|
||||
fr.send({**REPORT, "contact": "discord:me", "contactFollowup": True})
|
||||
|
||||
def test_a_report_with_follow_up_carries_the_kept_contact_id(self):
|
||||
fr.send({**REPORT, "contact": "me@example.com"})
|
||||
self.assertFalse(fc.FILE.exists()) # no follow-up, nothing kept or linked
|
||||
def test_follow_up_given_with_a_report_is_kept_and_removed_in_settings(self):
|
||||
fr.send({**REPORT, "contact": "me@example.com", "contactFollowup": True})
|
||||
without, with_ = (e["properties"] for e in self.events())
|
||||
self.assertEqual(without["contact_id"], "")
|
||||
self.assertEqual(with_["contact_id"], fc.load()["id"])
|
||||
self.assertNotEqual(with_["contact_id"], tm.settings()["id"]) # not the analytics id
|
||||
fc.save({"email": "me@example.com", "updates": True})
|
||||
self.assertEqual(self.events()[-1]["distinct_id"], with_["contact_id"]) # same copy, same id
|
||||
s = fc.state()
|
||||
self.assertEqual((s["email"], s["updates"], s["followup"]), ("me@example.com", False, True))
|
||||
consent = [e for e in self.events() if e["event"] == "contact_consent"]
|
||||
self.assertEqual([(e["properties"]["action"], e["properties"]["rev"]) for e in consent], [("set", 1)])
|
||||
self.assertEqual(consent[0]["distinct_id"], self.reports()[0]["contact_id"])
|
||||
fr.send({**REPORT, "contact": "ME@example.com", "contactFollowup": True}) # already agreed
|
||||
self.assertEqual(len([e for e in self.events() if e["event"] == "contact_consent"]), 1)
|
||||
self.assertEqual(self.reports()[1]["contact_rev"], 1)
|
||||
fc.save({"email": ""}) # Remove my email
|
||||
last = self.events()[-1]
|
||||
self.assertEqual((last["properties"]["action"], last["properties"]["email"], last["properties"]["rev"]),
|
||||
("withdraw", "", 2))
|
||||
logged = [e["properties"].get("contact") for e in tm._read_lines(tm.SENT) if e["event"] == "problem_report"]
|
||||
self.assertEqual(logged, ["<removed>", "<removed>"])
|
||||
|
||||
def report_row(self, ts, contact="me@example.com", followup=True, cid="copy"):
|
||||
return [ts, "AB12CD34", "bug", "RDP", "It never connects.", contact, "0.4.0", "Windows", "", "", followup, cid]
|
||||
def test_a_report_to_another_address_keeps_the_update_choice(self):
|
||||
fc.save({"email": "old@example.com", "updates": True})
|
||||
fr.send({**REPORT, "contact": "new@example.com", "contactFollowup": True})
|
||||
s = fc.state()
|
||||
self.assertEqual((s["email"], s["updates"], s["followup"]), ("new@example.com", True, True))
|
||||
self.assertEqual(self.reports()[0]["contact_rev"], 2)
|
||||
|
||||
def test_a_later_withdrawal_takes_back_a_reports_follow_up_permission(self):
|
||||
reports = [self.report_row("2026-09-10T10:00:00Z"), # removed later
|
||||
self.report_row("2026-09-12T10:00:00Z", cid="other"), # another copy
|
||||
self.report_row("2026-09-20T10:00:00Z"), # after the withdrawal
|
||||
self.report_row("2026-09-11T10:00:00Z", cid="", followup=True), # sent before contact_id
|
||||
self.report_row("2026-09-15T10:00:00Z", cid="same-second")]
|
||||
consents = [["copy", "me@example.com", True, 1, "2026-09-01T10:00:00Z"],
|
||||
["copy", "", False, 2, "2026-09-14T10:00:00Z"],
|
||||
["other", "me@example.com", True, 1, "2026-09-13T10:00:00Z"], # still agrees
|
||||
["same-second", "", False, 1, "2026-09-15T10:00:00Z"], ["short"]]
|
||||
def report_row(self, contact="me@example.com", followup=True, cid="copy", rev=1):
|
||||
return ["2026-09-10T10:00:00Z", "AB12CD34", "bug", "RDP", "It never connects.", contact,
|
||||
"0.4.0", "Windows", "", "", followup, cid, rev]
|
||||
|
||||
def test_a_later_change_takes_back_a_reports_follow_up_permission(self):
|
||||
reports = [self.report_row(), # removed later
|
||||
self.report_row(cid="other"), # another copy, still agrees
|
||||
self.report_row(rev=3), # sent after the removal
|
||||
self.report_row(cid="moved"), # address changed later
|
||||
self.report_row(cid="news-only"), # follow-up unticked later
|
||||
self.report_row(contact="Me@Example.com", cid="case"), # same address, any case
|
||||
self.report_row(cid="", followup=True), # no contact id: left alone
|
||||
self.report_row(cid="bad", rev="x")] # malformed rev: treated as 0
|
||||
consents = [["copy", "me@example.com", True, 1], ["copy", "", False, 2],
|
||||
["other", "me@example.com", True, 1], ["other", "me@example.com", True, 2],
|
||||
["moved", "new@example.com", True, 2], ["news-only", "me@example.com", False, 2],
|
||||
["case", "me@example.com", True, 2], ["bad", "", False, 1], ["short"], ["x", "", False, "?"]]
|
||||
fr.mark_withdrawn(reports, consents)
|
||||
self.assertEqual([r[10] for r in reports], ["withdrawn", True, True, True, "withdrawn"])
|
||||
self.assertEqual([r[10] for r in reports],
|
||||
["withdrawn", True, True, "withdrawn", "withdrawn", True, True, "withdrawn"])
|
||||
|
||||
def test_changing_the_address_or_unticking_follow_up_takes_it_back_too(self):
|
||||
reports = [self.report_row("2026-09-10T10:00:00Z", cid="moved"),
|
||||
self.report_row("2026-09-10T10:00:00Z", cid="news-only"),
|
||||
self.report_row("2026-09-10T10:00:00Z", contact="Me@Example.com", cid="case")]
|
||||
consents = [["moved", "new@example.com", True, 1, "2026-09-11T10:00:00Z"],
|
||||
["news-only", "me@example.com", False, 1, "2026-09-11T10:00:00Z"],
|
||||
["case", "me@example.com", True, 1, "2026-09-11T10:00:00Z"],
|
||||
# rev, not the clock, decides which later choice is newest
|
||||
["case", "", False, 2, "2026-09-11T09:59:00Z"]]
|
||||
fr.mark_withdrawn(reports, consents)
|
||||
self.assertEqual([r[10] for r in reports], ["withdrawn", "withdrawn", "withdrawn"])
|
||||
reports[2][10] = True
|
||||
fr.mark_withdrawn(reports[2:], consents[2:3])
|
||||
self.assertIs(reports[2][10], True) # same address, any case, still agrees
|
||||
def test_the_change_number_decides_not_the_clock(self):
|
||||
"""The clock went back between the report and the removal: the removal still counts."""
|
||||
fr.send({**REPORT, "contact": "me@example.com", "contactFollowup": True})
|
||||
with mock.patch.object(fc.time, "gmtime", return_value=time.gmtime(0)):
|
||||
fc.save({"email": ""})
|
||||
report = self.reports()[0]
|
||||
row = self.report_row(cid=report["contact_id"], rev=report["contact_rev"])
|
||||
consents = [[e["distinct_id"], e["properties"]["email"], e["properties"]["followup"], e["properties"]["rev"]]
|
||||
for e in self.events() if e["event"] == "contact_consent"]
|
||||
self.assertEqual(self.events()[-1]["timestamp"], "1970-01-01T00:00:00Z")
|
||||
fr.mark_withdrawn([row], consents)
|
||||
self.assertEqual(row[10], "withdrawn")
|
||||
|
||||
def test_the_inbox_shows_withdrawn_follow_up_without_the_address(self):
|
||||
reports = [self.report_row("2026-09-10T10:00:00Z"), ["short"]]
|
||||
consents = [["copy", "", False, 2, "2026-09-14T10:00:00Z"]]
|
||||
reports = [self.report_row(), ["short"]]
|
||||
consents = [["copy", "", False, 2]]
|
||||
with mock.patch.object(db, "_posthog_query", side_effect=[{"results": reports}, {"results": consents}]) as q, \
|
||||
mock.patch.object(sys, "argv", ["frame_report.py", "inbox", "30"]), \
|
||||
mock.patch("builtins.print") as out:
|
||||
fr.main()
|
||||
self.assertIn("properties.contact_rev", q.call_args_list[0].args[0])
|
||||
self.assertIn("event = 'contact_consent'", q.call_args_list[1].args[0])
|
||||
printed = " ".join(str(c.args[0]) for c in out.call_args_list if c.args)
|
||||
self.assertIn("follow-up permission since withdrawn", printed)
|
||||
self.assertNotIn("me@example.com", printed)
|
||||
with mock.patch.object(db, "_posthog_query", return_value={"results": [self.report_row("x", followup=False)]}) as q:
|
||||
with mock.patch.object(db, "_posthog_query", return_value={"results": [self.report_row(followup=False)]}) as q:
|
||||
fr.inbox()
|
||||
self.assertEqual(q.call_count, 1) # nothing to reconcile, no second query
|
||||
|
||||
|
||||
@@ -391,15 +391,16 @@ class ReportProblem(Base):
|
||||
def test_send_is_a_private_posthog_event_whatever_the_settings(self):
|
||||
got = self.serve()
|
||||
tm.update_settings({"usage": False}) # analytics off: a deliberate report still goes
|
||||
res = fr.send({"kind": "idea", "title": "Live view stops", "message": "It stops after a minute.",
|
||||
"contact": "me@example.com", "contactFollowup": True})
|
||||
with mock.patch.object(fr.frame_contact, "from_report", return_value=("contact-id", 1)): # test_contact
|
||||
res = fr.send({"kind": "idea", "title": "Live view stops", "message": "It stops after a minute.",
|
||||
"contact": "me@example.com", "contactFollowup": True})
|
||||
path, body = got[0]
|
||||
event = body["batch"][0]
|
||||
self.assertEqual((path, body["api_key"], event["event"]), ("/batch/", "phc_test", "problem_report"))
|
||||
props = event["properties"]
|
||||
self.assertEqual((props["kind"], props["title"], props["message"], props["contact"], props["report_id"]),
|
||||
("idea", "Live view stops", "It stops after a minute.", "me@example.com", res["id"]))
|
||||
self.assertIs(props["contact_followup"], True)
|
||||
self.assertEqual((props["contact_followup"], props["contact_id"], props["contact_rev"]), (True, "contact-id", 1))
|
||||
self.assertEqual((props["$process_person_profile"], props["$geoip_disable"]), (False, True))
|
||||
self.assertNotEqual(event["distinct_id"], tm.settings()["id"]) # not linked to the analytics
|
||||
self.assertIn(res["id"], res["message"])
|
||||
@@ -422,8 +423,8 @@ class ReportProblem(Base):
|
||||
|
||||
def test_the_inbox_skips_malformed_reports(self):
|
||||
good = ["2026-09-28T09:50:00Z", "AB12CD34", "bug", "Live view stops", "It stops.", None,
|
||||
"0.4.0", "macOS", "", "", None, None]
|
||||
rows = [["2026-09-28T10:00:00Z", "X", "bug", "Hand-made", None, None, None, None, None, None, None, None],
|
||||
"0.4.0", "macOS", "", "", None, None, None]
|
||||
rows = [["2026-09-28T10:00:00Z", "X", "bug", "Hand-made", None, None, None, None, None, None, None, None, None],
|
||||
["short"], good]
|
||||
with mock.patch.object(db, "_posthog_query", return_value={"results": rows}), \
|
||||
mock.patch.object(sys, "argv", ["frame_report.py", "inbox"]), \
|
||||
|
||||
+11
-7
@@ -79,14 +79,18 @@ def flag(body, key):
|
||||
return v is True
|
||||
|
||||
|
||||
def contact_id():
|
||||
"""This copy's contact id, kept from now on. A report with follow-up consent carries it, so
|
||||
removing the address later takes back the follow-up permission given with the report too."""
|
||||
with _lock:
|
||||
def from_report(email):
|
||||
"""Follow-up questions agreed to with a problem report: the address becomes the contact
|
||||
email with that choice ticked (the update choice stays as it was), so it shows in Settings
|
||||
and is removed the same way. Returns (contact id, rev) for the report to carry: a later
|
||||
change from this copy has a higher rev, and the newest such change decides whether the
|
||||
report's follow-up permission still stands, whatever the clocks say."""
|
||||
s = load()
|
||||
if s['email'].lower() != email.lower() or not s['followup']:
|
||||
save({'email': s['email'] if s['email'].lower() == email.lower() else email,
|
||||
'updates': s['updates'], 'followup': True})
|
||||
s = load()
|
||||
if not FILE.exists():
|
||||
_save(s)
|
||||
return s['id']
|
||||
return s['id'], s['rev']
|
||||
|
||||
|
||||
def state():
|
||||
|
||||
+24
-15
@@ -116,11 +116,13 @@ def send(body):
|
||||
contact = str(body.get('contact') or '').strip() if followup else ''
|
||||
if followup and not frame_contact.valid_email(contact):
|
||||
raise ValueError('add your email address for follow-up questions, or untick that box')
|
||||
# It becomes the contact email in Settings, where it's changed or removed like any other.
|
||||
contact_id, contact_rev = frame_contact.from_report(contact) if followup else ('', 0)
|
||||
ref = uuid.uuid4().hex[:8].upper()
|
||||
props = {**frame_telemetry.common(), 'kind': kind, 'title': title, 'message': text,
|
||||
'contact': contact, 'contact_followup': followup, 'diagnostics': diag,
|
||||
# Only with an address: so removing it later (Settings) also takes this permission back.
|
||||
'contact_id': frame_contact.contact_id() if followup else '',
|
||||
# Only with an address: a later change from this copy (higher rev) can take it back.
|
||||
'contact_id': contact_id, 'contact_rev': contact_rev,
|
||||
'report_id': ref, 'steamos': str(frame.get('build') or '')[:120], 'level': 'report'}
|
||||
# Its own random id: a report can carry contact details, so it isn't linked to this copy's analytics.
|
||||
event = {'event': 'problem_report', 'distinct_id': str(uuid.uuid4()), 'uuid': str(uuid.uuid4()),
|
||||
@@ -152,36 +154,43 @@ def inbox(days=30):
|
||||
res = frame_compat_db._posthog_query(
|
||||
"SELECT timestamp, properties.report_id, properties.kind, properties.title, properties.message, "
|
||||
"properties.contact, properties.app_version, properties.os, properties.steamos, properties.diagnostics, "
|
||||
"properties.contact_followup, properties.contact_id "
|
||||
"properties.contact_followup, properties.contact_id, properties.contact_rev "
|
||||
f"FROM events WHERE event = 'problem_report' AND timestamp > now() - INTERVAL {days} DAY "
|
||||
"ORDER BY timestamp DESC LIMIT 200")
|
||||
rows = [r for r in res.get('results') or [] if isinstance(r, list) and len(r) == 12]
|
||||
rows = [r for r in res.get('results') or [] if isinstance(r, list) and len(r) == 13]
|
||||
if any(r[11] and _yes(r[10]) for r in rows):
|
||||
later = frame_compat_db._posthog_query(
|
||||
"SELECT distinct_id, properties.email, properties.followup, ifNull(toInt(properties.rev), 0), timestamp "
|
||||
f"FROM events WHERE event = 'contact_consent' AND timestamp > now() - INTERVAL {days} DAY LIMIT 100000")
|
||||
"SELECT distinct_id, properties.email, properties.followup, ifNull(toInt(properties.rev), 0) "
|
||||
"FROM events WHERE event = 'contact_consent' LIMIT 100000")
|
||||
mark_withdrawn(rows, later.get('results') or [])
|
||||
return rows
|
||||
|
||||
|
||||
def mark_withdrawn(reports, consents):
|
||||
"""Mark reports whose follow-up permission was taken back: the newest contact choice from
|
||||
the same copy made at or after the report (same second counts, so a withdrawal wins) no
|
||||
the same copy made after the report (a higher rev than it carries, not a later clock) no
|
||||
longer agrees to follow-up questions at that address."""
|
||||
newest = {}
|
||||
for c in consents:
|
||||
if not isinstance(c, list) or len(c) != 5:
|
||||
if not isinstance(c, list) or len(c) != 4:
|
||||
continue
|
||||
cid, email, followup, rev, ts = c
|
||||
newest.setdefault(str(cid), []).append(((int(rev or 0), str(ts or '')), str(email or ''), followup))
|
||||
cid, email, followup, rev = c
|
||||
try:
|
||||
rev = int(rev or 0)
|
||||
except (TypeError, ValueError):
|
||||
continue
|
||||
if rev > newest.get(str(cid), (-1,))[0]:
|
||||
newest[str(cid)] = (rev, str(email or ''), followup)
|
||||
for r in reports:
|
||||
if not (r[11] and _yes(r[10])):
|
||||
continue
|
||||
after = [c for c in newest.get(str(r[11]), []) if c[0][1] >= str(r[0] or '')]
|
||||
if after:
|
||||
_, email, followup = max(after, key=lambda c: c[0])
|
||||
if not (_yes(followup) and email.strip().lower() == str(r[5] or '').strip().lower()):
|
||||
r[10] = 'withdrawn'
|
||||
try:
|
||||
sent_at = int(r[12] or 0)
|
||||
except (TypeError, ValueError):
|
||||
sent_at = 0
|
||||
rev, email, followup = newest.get(str(r[11]), (-1, '', None))
|
||||
if rev > sent_at and not (_yes(followup) and email.strip().lower() == str(r[5] or '').strip().lower()):
|
||||
r[10] = 'withdrawn'
|
||||
|
||||
|
||||
def _yes(v):
|
||||
|
||||
+2
-1
@@ -1262,7 +1262,7 @@
|
||||
<label class="field">What happened?<textarea id="bugText" maxlength="5000" required minlength="10"
|
||||
placeholder="What you did, what happened, and what you expected."></textarea></label>
|
||||
<label class="popt"><input type="checkbox" id="bugFollowup"><b>The maintainer may contact me with follow-up questions</b>
|
||||
<span class="sub">Optional. Your email address goes with this report only when this is ticked.</span></label>
|
||||
<span class="sub">Optional. Your email address goes with this report only when this is ticked, and is kept as your contact email in Privacy & updates, where you can remove it.</span></label>
|
||||
<label class="field">Your email address<input type="email" id="bugContact" maxlength="254" placeholder="you@example.com" disabled></label>
|
||||
<label class="popt"><input type="checkbox" id="bugDiag" checked><b>Include diagnostics</b>
|
||||
<span class="sub">Frame Control's version, your OS and the Frame's SteamOS build.</span></label>
|
||||
@@ -4125,6 +4125,7 @@ $("bugForm").onsubmit = async e => {
|
||||
$("bugMsg").textContent = `Couldn't send it: ${err.message}. Try again later, or use Copy report.`;
|
||||
$("bugSend").disabled = false;
|
||||
}
|
||||
if ($("bugFollowup").checked) api("/api/contact").then(renderContact).catch(() => {});
|
||||
};
|
||||
if (window.frameApp && window.frameApp.onReportProblem) window.frameApp.onReportProblem(openBugReport);
|
||||
|
||||
|
||||
Reference in new issue
Block a user