diff --git a/README.md b/README.md index 55f5ae6..bef5e6a 100644 --- a/README.md +++ b/README.md @@ -25,7 +25,7 @@ The plugin ID, the table names, the config keys and the settings code are a cont - **CORS.** The widget's preflights are answered by the framework's path-scoped CORS layer, so the plugin registers no `OPTIONS` route. - **Rate limits.** The named buckets `feedback-config` (60 a minute) and `feedback-submit` (10 a minute) count per client IP behind the configured trusted proxies. - **Settings.** `models.Settings` is the singleton row of `golem15_feedback_settings`. `models.Instance` creates it on first access with the WinterCMS defaults and a new widget key (`wk_` plus 32 random letters and digits). Operators edit it under Settings › Feedback Widget (code `feedback`, permission `golem15.feedback.manage_settings`): the WinterCMS form with cabana's field types, the colors as text fields and the widget key read-only. A row saved without a key gets a new one. -- **G15Office tasks.** Every submission queues a `classes.SyncG15OfficeArgs` job (kind `golem15.feedback.sync_g15office`, queue `feedback`, 3 attempts, a `summer_jobs` row labelled with the WinterCMS job class). `classes.SyncG15Office` files it with `classes.G15OfficeClient`: `POST /_support/api/v1/projects//tasks` with the title (`Feedback: ` and the message collapsed and cut to 80 columns), a Markdown description (the message, the page, the reporter email and user agent, the console log cut to 4000 columns), the task type (`bug` → Bug, `feature` → Feature, `other` → Task), the configured task status and priority; then `POST /_support/api/v1/tasks//attachments` with the screenshot as the multipart field `file`. Requests carry `Authorization: Bearer ` and `Accept: application/json` and go through a `fetchguard.Client` limited to the base URL's host (https only). Success marks the submission `sent` with the task id; a failure marks it `failed` with the error and the job is retried. Without a base URL, token and project nothing is sent: the job fails with the WinterCMS "not configured" message. +- **G15Office tasks.** Every submission queues a `classes.SyncG15OfficeArgs` job (kind `golem15.feedback.sync_g15office`, queue `feedback`, 3 attempts, a `summer_jobs` row labelled with the WinterCMS job class). `classes.SyncG15Office` files it with `classes.G15OfficeClient`: `POST /_support/api/v1/projects//tasks` with the title (`Feedback: ` and the message collapsed and cut to 80 columns), a Markdown description (the message, the page, the reporter email and user agent, the console log cut to 4000 columns), the task type (`bug` → Bug, `feature` → Feature, `other` → Task), the configured task status and priority; then `POST /_support/api/v1/tasks//attachments` with the screenshot as the multipart field `file`. Requests carry `Authorization: Bearer ` and `Accept: application/json` and go through a `fetchguard.Client` limited to the base URL's host (https only). Success marks the submission `sent` with the task id; a failure marks it `failed` with the error and the job is retried. The task id is stored as soon as the task is created, so a retry re-attempts only the attachment, and a submission already `sent` with a task id is never filed again. Without a base URL, token and project nothing is sent: the job fails with the WinterCMS "not configured" message. - **Submissions list.** Admin › Feedback › Submissions (controller `golem15.feedback.submissions`) is a read-only list: type, status, reporter email, page, message, task id and date, all as plain text. - **Widget script.** `GET /plugins/golem15/feedback/assets/js/embed.js` serves the WinterCMS plugin's `embed.js` byte for byte (`application/javascript`, with an ETag), at the path embedding pages already use. - **Settings import.** `feedback:import-settings` copies a WinterCMS installation's widget settings, the widget key included, into the singleton once. diff --git a/classes/classes_test.go b/classes/classes_test.go index 7685aca..cb38e5b 100644 --- a/classes/classes_test.go +++ b/classes/classes_test.go @@ -274,17 +274,49 @@ func TestSyncG15Office(t *testing.T) { failing(t, parityClient(), roundTrip(func(*http.Request) (*http.Response, error) { return nil, errors.New("dial failed") }), "g15office create task failed: connection error.") }) t.Run("attach-failure", func(t *testing.T) { - calls := 0 + // The task id is kept from the first attempt, so the retry only + // re-attempts the attachment and files no second task (WR-06). + tasks, attaches := 0, 0 + attachStatus := 413 rt := roundTrip(func(r *http.Request) (*http.Response, error) { - calls++ if strings.HasSuffix(r.URL.Path, "/tasks") { + tasks++ return jsonAnswer(201, `{"data":{"id":"tskX"}}`)(r) } - return jsonAnswer(413, `{"error":true,"message":"File too large"}`)(r) + attaches++ + if !strings.Contains(r.URL.Path, "/tasks/tskX/") { + t.Errorf("attachment to %s", r.URL.Path) + } + if attachStatus != 201 { + return jsonAnswer(attachStatus, `{"error":true,"message":"File too large"}`)(r) + } + return jsonAnswer(201, `{"data":{"id":"att"}}`)(r) }) - failing(t, parityClient(), rt, "g15office attach file failed: File too large") - if calls != 2 { - t.Fatalf("%d requests", calls) + sub := recordedSubmission(t, app, gdb, bucket) + deps := classes.SyncDeps{DB: gdb, Bucket: bucket, Client: parityClient()} + ctx := fetchguard.WithTransport(t.Context(), rt) + const wantErr = "g15office attach file failed: File too large" + if err := classes.SyncG15Office(ctx, deps, classes.SyncG15OfficeArgs{SubmissionID: sub.ID}); err == nil || err.Error() != wantErr { + t.Fatalf("error %v, want %q", err, wantErr) + } + got := reload(t, gdb, sub.ID) + if got.Status != models.StatusFailed || got.G15OfficeError == nil || *got.G15OfficeError != wantErr || got.G15OfficeTaskID == nil || *got.G15OfficeTaskID != "tskX" { + t.Fatalf("submission %+v", got) + } + attachStatus = 201 + if err := classes.SyncG15Office(ctx, deps, classes.SyncG15OfficeArgs{SubmissionID: sub.ID}); err != nil { + t.Fatal(err) + } + got = reload(t, gdb, sub.ID) + if got.Status != models.StatusSent || got.G15OfficeError != nil || got.G15OfficeTaskID == nil || *got.G15OfficeTaskID != "tskX" { + t.Fatalf("retried submission %+v", got) + } + // A sent submission with its task is never filed again. + if err := classes.SyncG15Office(ctx, deps, classes.SyncG15OfficeArgs{SubmissionID: sub.ID}); err != nil { + t.Fatal(err) + } + if tasks != 1 || attaches != 2 { + t.Fatalf("%d task and %d attachment requests", tasks, attaches) } }) t.Run("http-base-url-is-refused", func(t *testing.T) { diff --git a/classes/sync_g15office.go b/classes/sync_g15office.go index f4dd36b..6605188 100644 --- a/classes/sync_g15office.go +++ b/classes/sync_g15office.go @@ -48,6 +48,11 @@ type SyncDeps struct { // error and returns the error, so the job is retried. A submission that is // gone is skipped. An unconfigured client fails before any request, so // nothing leaves the application. +// +// Unlike PHP, a retry never files a second task: a submission already sent +// with a task id is skipped, the task id is stored as soon as CreateTask +// returns it, and a retry of a submission that has one only re-attempts +// the screenshot attachment. func SyncG15Office(ctx context.Context, deps SyncDeps, args SyncG15OfficeArgs) error { if deps.DB == nil { return errors.New("feedback: database is not configured") @@ -61,6 +66,9 @@ func SyncG15Office(ctx context.Context, deps SyncDeps, args SyncG15OfficeArgs) e return nil } sub := rows[0] + if sub.Status == models.StatusSent && hasTaskID(&sub) { + return nil + } id, err := fileTask(ctx, deps, &sub) if err != nil { msg := err.Error() @@ -76,7 +84,32 @@ func SyncG15Office(ctx context.Context, deps SyncDeps, args SyncG15OfficeArgs) e }).Error } +// hasTaskID reports whether an earlier attempt already filed the task. +func hasTaskID(sub *models.Submission) bool { + return sub.G15OfficeTaskID != nil && *sub.G15OfficeTaskID != "" +} + func fileTask(ctx context.Context, deps SyncDeps, sub *models.Submission) (string, error) { + var id string + if hasTaskID(sub) { + id = *sub.G15OfficeTaskID + } else { + created, err := createTask(ctx, deps, sub) + if err != nil { + return "", err + } + id = created + } + if err := attachScreenshot(ctx, deps, sub, id); err != nil { + return "", err + } + return id, nil +} + +// createTask files the submission as a new G15Office task and stores the +// task id at once, so a later failure (the attachment) is retried against +// this task rather than filing another. +func createTask(ctx context.Context, deps SyncDeps, sub *models.Submission) (string, error) { settings, err := models.Instance(ctx, deps.DB) if err != nil { return "", err @@ -101,27 +134,37 @@ func fileTask(ctx context.Context, deps SyncDeps, sub *models.Submission) (strin if id == "" { return "", errors.New("g15office did not return a task id.") } - file, err := ScreenshotOf(ctx, deps.DB, sub.ID) - if err != nil { + if err := deps.DB.WithContext(ctx).Model(&models.Submission{}).Where("id = ?", sub.ID).Updates(map[string]any{ + "g15office_task_id": id, "updated_at": time.Now(), + }).Error; err != nil { return "", err } - if file != nil { - if deps.Bucket == nil { - return "", errors.New("feedback: upload bucket is not configured") - } - r, err := deps.Bucket.NewReader(ctx, attach.BlobKey(file.DiskName), nil) - if err != nil { - return "", err - } - _, err = deps.Client.AttachFile(ctx, id, file.FileName, file.ContentType, r) - _ = r.Close() - if err != nil { - return "", err - } - } + sub.G15OfficeTaskID = &id return id, nil } +// attachScreenshot uploads the submission's screenshot, when it has one, +// to the task. +func attachScreenshot(ctx context.Context, deps SyncDeps, sub *models.Submission, id string) error { + file, err := ScreenshotOf(ctx, deps.DB, sub.ID) + if err != nil { + return err + } + if file == nil { + return nil + } + if deps.Bucket == nil { + return errors.New("feedback: upload bucket is not configured") + } + r, err := deps.Bucket.NewReader(ctx, attach.BlobKey(file.DiskName), nil) + if err != nil { + return err + } + _, err = deps.Client.AttachFile(ctx, id, file.FileName, file.ContentType, r) + _ = r.Close() + return err +} + // whitespaceRun is PCRE's \s+ without the u flag: ASCII space, tab, LF, VT, FF // and CR (Go's \s leaves out VT). var whitespaceRun = regexp.MustCompile(`[\t\n\v\f\r ]+`) diff --git a/jobs.go b/jobs.go index b85fbaa..946fe1b 100644 --- a/jobs.go +++ b/jobs.go @@ -45,9 +45,13 @@ func (p *Plugin) syncG15Office(ctx context.Context, args classes.SyncG15OfficeAr slog.Default().Warn("golem15.feedback: filing a submission in G15Office failed", slog.Uint64("submission_id", uint64(args.SubmissionID))) return err } + // The task is filed: a failed completion is logged, not returned, so + // River does not retry a job whose side effect already happened. if id, ok := conga.JobID(ctx); ok { if m, err := conga.From(p.app); err == nil { - return m.CompleteJob(ctx, id, nil) + if err := m.CompleteJob(ctx, id, nil); err != nil { + slog.Default().Warn("golem15.feedback: completing the G15Office job failed", slog.Uint64("submission_id", uint64(args.SubmissionID)), slog.Uint64("job_id", uint64(id))) + } } } return nil