amadan
abono: a sync that kept some subscriptions does not say nothing changed
Summary
Report gains Failed, the subscriptions Reconcile could not sync. The generated members page answers a failed Reconcile with the new key abono.error.sync_partial when at least one subscription did sync (Subscriptions > Failed), and keeps the approved abono.error.sync_failed, which says nothing was changed, for a run that synced none.
Agent notes
Final review M3, ruling R43. abono.error.sync_partial is rendered as a key and in pendingReview and newKeys for the third mini copy review. The comp sweep and price refresh are not counted as 'changed': the sweep re-tells the app state abono already had, and a price refresh changes no holding. Example pages regenerated. Regression: example TestSyncSaysWhenItOnlyPartlyWorked (a proxy refuses one subscription, then all of them); TestReconcileCarriesOnPastOneBadSubscription checks Failed.
7 files changed,
+84
−9
cmd/abono/main_test.go+1 −1cmd/abono/pages/abono_pages.go.tmpl+8 −2example/abono_pages.go+8 −2example/app_test.go+55 −0reconcile.go+9 −2reconcile_test.go+2 −2templates_test.go+1 −0
CI passed — run details
CI log
=== vet === go vet ./... === fmt === === test === go test ./... -count=1 ok amadan.net/rastrillo/abono 2.649s ok amadan.net/rastrillo/abono/abonotest 0.056s ok amadan.net/rastrillo/abono/cmd/abono 0.662s ok amadan.net/rastrillo/abono/internal/fakestripe 0.019s ok amadan.net/rastrillo/abono/internal/stripeapi 0.007s ok example 0.213s === race === CGO_ENABLED=1 go test ./... -race -count=1 ok amadan.net/rastrillo/abono 8.725s ok amadan.net/rastrillo/abono/abonotest 1.443s ok amadan.net/rastrillo/abono/cmd/abono 1.979s ok amadan.net/rastrillo/abono/internal/fakestripe 1.093s ok amadan.net/rastrillo/abono/internal/stripeapi 1.018s ok example 2.439s === browser === ok example 0.832s
Intent
diff --git a/cmd/abono/main_test.go b/cmd/abono/main_test.go| index bec0f11..4ca7618 100644 |
| --- a/cmd/abono/main_test.go |
| +++ b/cmd/abono/main_test.go |
| @@ -286,7 +286,7 @@ var partialKeys = []string{"abono.price.per_month", "abono.price.per_year"} |
| // going through their own copy review. The pages must use each. |
| var newKeys = []string{"abono.owner.free_no_member", "abono.pages.done_processing", "abono.owner.price_unsupported", |
| "abono.owner.add_price_title", "abono.owner.add_price_id", "abono.button.add_price", "abono.button.make_sold", |
| - "abono.error.add_price_failed"} |
| + "abono.error.add_price_failed", "abono.error.sync_partial"} |
| var keyLiteral = regexp.MustCompile(`"(abono\.[a-z][a-z_.]*[a-z])"`) |
diff --git a/cmd/abono/pages/abono_pages.go.tmpl b/cmd/abono/pages/abono_pages.go.tmpl| index be108d5..767851f 100644 |
| --- a/cmd/abono/pages/abono_pages.go.tmpl |
| +++ b/cmd/abono/pages/abono_pages.go.tmpl |
| @@ -484,12 +484,18 @@ func (p *AbonoPages) renderMembers(w http.ResponseWriter, r *http.Request, statu |
| }) |
| } |
| -// POST /admin/members/sync: Reconcile now. |
| +// POST /admin/members/sync: Reconcile now. Reconcile keeps every |
| +// subscription it could sync when others fail, so the page says nothing |
| +// changed only when no subscription was synced. |
| func (p *AbonoPages) sync(w http.ResponseWriter, r *http.Request) { |
| rep, err := p.Service.Reconcile(r.Context()) |
| if err != nil { |
| slog.ErrorContext(r.Context(), "abono pages: sync", "err", err) |
| - p.renderMembers(w, r, http.StatusBadGateway, abonoCompForm{}, "abono.error.sync_failed") |
| + key := "abono.error.sync_failed" |
| + if rep.Subscriptions > rep.Failed { |
| + key = "abono.error.sync_partial" |
| + } |
| + p.renderMembers(w, r, http.StatusBadGateway, abonoCompForm{}, key) |
| return |
| } |
| http.Redirect(w, r, abonoMembersPath+"?done=sync&n="+strconv.Itoa(rep.Subscriptions), http.StatusSeeOther) |
diff --git a/example/abono_pages.go b/example/abono_pages.go| index acb9ce4..69481d6 100644 |
| --- a/example/abono_pages.go |
| +++ b/example/abono_pages.go |
| @@ -484,12 +484,18 @@ func (p *AbonoPages) renderMembers(w http.ResponseWriter, r *http.Request, statu |
| }) |
| } |
| -// POST /admin/members/sync: Reconcile now. |
| +// POST /admin/members/sync: Reconcile now. Reconcile keeps every |
| +// subscription it could sync when others fail, so the page says nothing |
| +// changed only when no subscription was synced. |
| func (p *AbonoPages) sync(w http.ResponseWriter, r *http.Request) { |
| rep, err := p.Service.Reconcile(r.Context()) |
| if err != nil { |
| slog.ErrorContext(r.Context(), "abono pages: sync", "err", err) |
| - p.renderMembers(w, r, http.StatusBadGateway, abonoCompForm{}, "abono.error.sync_failed") |
| + key := "abono.error.sync_failed" |
| + if rep.Subscriptions > rep.Failed { |
| + key = "abono.error.sync_partial" |
| + } |
| + p.renderMembers(w, r, http.StatusBadGateway, abonoCompForm{}, key) |
| return |
| } |
| http.Redirect(w, r, abonoMembersPath+"?done=sync&n="+strconv.Itoa(rep.Subscriptions), http.StatusSeeOther) |
diff --git a/example/app_test.go b/example/app_test.go| index 50dc79b..f482eaa 100644 |
| --- a/example/app_test.go |
| +++ b/example/app_test.go |
| @@ -20,6 +20,7 @@ import ( |
| "strconv" |
| "strings" |
| "sync" |
| + "sync/atomic" |
| "testing" |
| "time" |
| @@ -764,3 +765,57 @@ func TestOwnerAddsAndPromotesPrices(t *testing.T) { |
| st, _, body = boss.post("/admin/offers/prices", url.Values{"plan": {"monthly"}, "price": {" "}}) |
| want(t, "no price", st, http.StatusUnprocessableEntity, body, say("abono.error.add_price_failed")) |
| } |
| + |
| +// TestSyncSaysWhenItOnlyPartlyWorked is final review M3 (ruling R43): |
| +// Reconcile keeps what it could sync when another subscription fails, so |
| +// "nothing was changed" is only true when nothing was. Stripe, through a |
| +// proxy, refuses to give out one subscription, then everything. |
| +func TestSyncSaysWhenItOnlyPartlyWorked(t *testing.T) { |
| + stripe := abonotest.New(t) |
| + stripe.AddPrice("price_annual", "annual", 6000, "eur", "year") |
| + stripe.AddPrice("price_monthly", "monthly", 600, "eur", "month") |
| + target, err := url.Parse(stripe.URL) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + var refuse atomic.Value // "", "one" or "all" |
| + refuse.Store("") |
| + proxy := httputil.NewSingleHostReverseProxy(target) |
| + front := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| + switch mode := refuse.Load().(string); { |
| + case mode == "one" && r.URL.Path == "/v1/subscriptions/sub_bad", |
| + mode == "all" && strings.HasPrefix(r.URL.Path, "/v1/subscriptions"): |
| + w.Header().Set("Content-Type", "application/json") |
| + w.WriteHeader(http.StatusInternalServerError) |
| + w.Write([]byte(`{"error":{"type":"api_error","message":"down"}}`)) |
| + return |
| + } |
| + proxy.ServeHTTP(w, r) |
| + })) |
| + t.Cleanup(front.Close) |
| + app := newApp(t, func(c *Config) { |
| + c.Abono = func(ac abono.Config) abono.Config { |
| + ac = stripe.Config(ac) |
| + ac.StripeBase = front.URL |
| + return ac |
| + } |
| + }) |
| + now := stripe.Now() |
| + for _, id := range []string{"sub_good", "sub_bad"} { |
| + stripe.AddSubscription(abonotest.Subscription{ID: id, Status: "active", Price: "price_monthly", |
| + Email: id + "@example.com", PeriodStart: now, PeriodEnd: now.Add(30 * 24 * time.Hour)}) |
| + } |
| + boss := newClient(t, app) |
| + boss.signin(owner) |
| + |
| + refuse.Store("one") |
| + st, _, body := boss.post("/admin/members/sync", nil) |
| + want(t, "a sync that partly worked", st, http.StatusBadGateway, body, say("abono.error.sync_partial"), "sub_good@example.com") |
| + if strings.Contains(body, say("abono.error.sync_failed")) { |
| + t.Error("a sync that kept sub_good says nothing was changed") |
| + } |
| + |
| + refuse.Store("all") |
| + st, _, body = boss.post("/admin/members/sync", nil) |
| + want(t, "a sync that could not reach Stripe", st, http.StatusBadGateway, body, say("abono.error.sync_failed")) |
| +} |
diff --git a/reconcile.go b/reconcile.go| index c57686c..e065495 100644 |
| --- a/reconcile.go |
| +++ b/reconcile.go |
| @@ -10,10 +10,16 @@ import ( |
| "amadan.net/rastrillo/abono/internal/stripeapi" |
| ) |
| -// Report is what a Reconcile looked at. |
| +// Report is what a Reconcile looked at. When Reconcile also returns an |
| +// error, the Report says what was done all the same: the subscriptions |
| +// that synced (Subscriptions - Failed of them) are committed, so a page |
| +// must not say that nothing was synced when that is more than none. |
| type Report struct { |
| - // Subscriptions is how many Stripe subscriptions were synced. |
| + // Subscriptions is how many Stripe subscriptions were listed and |
| + // synced, or tried. |
| Subscriptions int |
| + // Failed is how many of them could not be synced. |
| + Failed int |
| // Subjects is how many subjects OnChange was run for. |
| Subjects int |
| } |
| @@ -47,6 +53,7 @@ func (s *Service) Reconcile(ctx context.Context) (Report, error) { |
| // One subscription failing (the app's Holder or OnChange, say) |
| // must not stop the rest being refreshed on every tick after. |
| if err := s.reconcileOne(ctx, listed.ID); err != nil { |
| + rep.Failed++ |
| s.noteRefusedKey(err) |
| errs = append(errs, fmt.Errorf("abono: reconcile: subscription %s: %w", listed.ID, err)) |
| } |
diff --git a/reconcile_test.go b/reconcile_test.go| index 1d78a0a..d6623b4 100644 |
| --- a/reconcile_test.go |
| +++ b/reconcile_test.go |
| @@ -185,8 +185,8 @@ func TestReconcileCarriesOnPastOneBadSubscription(t *testing.T) { |
| if strings.Contains(err.Error(), "sub_2") { |
| t.Errorf("the error blames the good subscription: %v", err) |
| } |
| - if rep.Subscriptions != 2 || rep.Subjects != 1 { |
| - t.Errorf("Report = %+v, want 2 subscriptions seen and 1 subject told", rep) |
| + if rep.Subscriptions != 2 || rep.Failed != 1 || rep.Subjects != 1 { |
| + t.Errorf("Report = %+v, want 2 subscriptions seen, 1 failed and 1 subject told", rep) |
| } |
| if got := db.dump(subsQuery); got != "sub_2|bo|monthly|active|bo@example.com\n" { |
| t.Errorf("subscriptions:\n%s", got) |
diff --git a/templates_test.go b/templates_test.go| index 7d3006b..6f38a89 100644 |
| --- a/templates_test.go |
| +++ b/templates_test.go |
| @@ -718,6 +718,7 @@ var pendingReview = []string{ |
| // form's failure needed words of their own. |
| "abono.button.make_sold", |
| "abono.error.add_price_failed", |
| + "abono.error.sync_partial", |
| } |
| // Every key a consumer uses is in en.toml (bar the pendingReview ones), and |