fix(14): WR-06 never file a second G15Office task on a job retry

A retry after a failed attachment or a failed CompleteJob created another task for the same report. The task id is now stored as soon as the task is created, a retry with a stored id only re-attempts the attachment, a sent submission with a task id is skipped, and a CompleteJob failure is logged instead of returned.
2026-10-04 08:10:30 +02:00
parent 5a8b7f816f
commit e52b1ecd2c
4 changed files with 103 additions and 24 deletions
--- 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 <base>/_support/api/v1/projects/<project>/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 <base>/_support/api/v1/tasks/<id>/attachments` with the screenshot as the multipart field `file`. Requests carry `Authorization: Bearer <token>` 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 <base>/_support/api/v1/projects/<project>/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 <base>/_support/api/v1/tasks/<id>/attachments` with the screenshot as the multipart field `file`. Requests carry `Authorization: Bearer <token>` 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.
--- 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) {
--- 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 ]+`)
--- 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