From df4f94c6428f976fc43d0e54d7d6bf0ff7af1e55 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= <sybren@blender.org>
Date: Mon, 29 Jul 2024 17:48:28 +0200
Subject: [PATCH] Manager: show worker tag in job details

Show the worker tag name (and its description in a tooltip) in the job
details. When no worker tag is assigned, "All Workers" is shown in a more
dimmed colour.

This also renames the "Type" field to "Job Type". "Tag" and "Type" could
be confused, and now they're displayed as "Worker Tag" and "Job Type".

The UI in the add-on's submission interface is also updated for this, so
that that also shows "Worker Tag" (instead of just "Tag").
---
 addon/flamenco/gui.py                        |  2 +-
 internal/manager/api_impl/jobs.go            |  2 +-
 internal/manager/api_impl/jobs_query_test.go | 54 ++++++++++++++++++++
 internal/manager/api_impl/jobs_test.go       |  5 ++
 internal/manager/api_impl/worker_mgt.go      | 21 +++++---
 internal/manager/persistence/jobs.go         | 12 +++++
 internal/manager/persistence/jobs_test.go    | 32 ++++++++++++
 internal/manager/persistence/worker_tag.go   | 10 ++++
 web/app/src/components/jobs/JobDetails.vue   | 12 ++++-
 9 files changed, 140 insertions(+), 10 deletions(-)

diff --git a/addon/flamenco/gui.py b/addon/flamenco/gui.py
index 0fef1f3d..e518ec13 100644
--- a/addon/flamenco/gui.py
+++ b/addon/flamenco/gui.py
@@ -51,7 +51,7 @@ class FLAMENCO_PT_job_submission(bpy.types.Panel):
         )
         if not job_types.are_job_types_available():
             return
-        col.prop(context.scene, "flamenco_worker_tag", text="Tag")
+        col.prop(context.scene, "flamenco_worker_tag", text="Worker Tag")
 
         # Job properties:
         job_col = layout.column(align=True)
diff --git a/internal/manager/api_impl/jobs.go b/internal/manager/api_impl/jobs.go
index 48ea8fe5..886d594a 100644
--- a/internal/manager/api_impl/jobs.go
+++ b/internal/manager/api_impl/jobs.go
@@ -687,7 +687,7 @@ func jobDBtoAPI(dbJob *persistence.Job) api.Job {
 		apiJob.DeleteRequestedAt = &dbJob.DeleteRequestedAt.Time
 	}
 	if dbJob.WorkerTag != nil {
-		apiJob.WorkerTag = &dbJob.WorkerTag.UUID
+		apiJob.WorkerTag = workerTagDBtoAPI(dbJob.WorkerTag)
 	}
 
 	return apiJob
diff --git a/internal/manager/api_impl/jobs_query_test.go b/internal/manager/api_impl/jobs_query_test.go
index 9be480b1..28a9e994 100644
--- a/internal/manager/api_impl/jobs_query_test.go
+++ b/internal/manager/api_impl/jobs_query_test.go
@@ -89,6 +89,60 @@ func TestQueryJobs(t *testing.T) {
 	assertResponseJSON(t, echoCtx, http.StatusOK, expectedJobs)
 }
 
+func TestFetchJob(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+
+	mf := newMockedFlamenco(mockCtrl)
+
+	dbJob := persistence.Job{
+		UUID:     "afc47568-bd9d-4368-8016-e91d945db36d",
+		Name:     "работа",
+		JobType:  "test",
+		Priority: 50,
+		Status:   api.JobStatusActive,
+		Settings: persistence.StringInterfaceMap{
+			"result": "/render/frames/exploding.kittens",
+		},
+		Metadata: persistence.StringStringMap{
+			"project": "/projects/exploding-kittens",
+		},
+		WorkerTag: &persistence.WorkerTag{
+			UUID:        "d86e1b84-5ee2-4784-a178-65963eeb484b",
+			Name:        "Tikkie terug Kees!",
+			Description: "",
+		},
+	}
+
+	echoCtx := mf.prepareMockedRequest(nil)
+	mf.persistence.EXPECT().FetchJob(gomock.Any(), dbJob.UUID).Return(&dbJob, nil)
+
+	require.NoError(t, mf.flamenco.FetchJob(echoCtx, dbJob.UUID))
+
+	expectedJob := api.Job{
+		SubmittedJob: api.SubmittedJob{
+			Name:     "работа",
+			Type:     "test",
+			Priority: 50,
+			Settings: &api.JobSettings{AdditionalProperties: map[string]interface{}{
+				"result": "/render/frames/exploding.kittens",
+			}},
+			Metadata: &api.JobMetadata{AdditionalProperties: map[string]string{
+				"project": "/projects/exploding-kittens",
+			}},
+		},
+		Id:     "afc47568-bd9d-4368-8016-e91d945db36d",
+		Status: api.JobStatusActive,
+		WorkerTag: &api.WorkerTag{
+			Id:          ptr("d86e1b84-5ee2-4784-a178-65963eeb484b"),
+			Name:        "Tikkie terug Kees!",
+			Description: nil, // Empty description should just be excluded from the JSON.
+		},
+	}
+
+	assertResponseJSON(t, echoCtx, http.StatusOK, expectedJob)
+}
+
 func TestFetchTask(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	defer mockCtrl.Finish()
diff --git a/internal/manager/api_impl/jobs_test.go b/internal/manager/api_impl/jobs_test.go
index b5134cab..32942cbe 100644
--- a/internal/manager/api_impl/jobs_test.go
+++ b/internal/manager/api_impl/jobs_test.go
@@ -415,6 +415,11 @@ func TestSubmitJobWithWorkerTag(t *testing.T) {
 		DeleteRequestedAt: nil,
 		Activity:          "",
 		Status:            api.JobStatusQueued,
+		WorkerTag: &api.WorkerTag{
+			Id:          ptr(workerTagUUID),
+			Name:        "first tag",
+			Description: ptr("my first tag"),
+		},
 	})
 }
 
diff --git a/internal/manager/api_impl/worker_mgt.go b/internal/manager/api_impl/worker_mgt.go
index 88ca016d..564a4a9b 100644
--- a/internal/manager/api_impl/worker_mgt.go
+++ b/internal/manager/api_impl/worker_mgt.go
@@ -313,8 +313,11 @@ func (f *Flamenco) FetchWorkerTag(e echo.Context, tagUUID string) error {
 		logger.Error().Err(err).Msg("fetching worker tag")
 		return sendAPIError(e, http.StatusInternalServerError, "error fetching worker tag: %v", err)
 	}
+	if tag == nil {
+		panic("Could fetch a worker tag without error, but then the returned tag was still nil")
+	}
 
-	return e.JSON(http.StatusOK, workerTagDBtoAPI(*tag))
+	return e.JSON(http.StatusOK, workerTagDBtoAPI(tag))
 }
 
 func (f *Flamenco) UpdateWorkerTag(e echo.Context, tagUUID string) error {
@@ -387,8 +390,8 @@ func (f *Flamenco) FetchWorkerTags(e echo.Context) error {
 
 	apiTags := []api.WorkerTag{}
 	for _, dbTag := range dbTags {
-		apiTag := workerTagDBtoAPI(*dbTag)
-		apiTags = append(apiTags, apiTag)
+		apiTag := workerTagDBtoAPI(dbTag)
+		apiTags = append(apiTags, *apiTag)
 	}
 
 	tagList := api.WorkerTagList{
@@ -443,7 +446,7 @@ func (f *Flamenco) CreateWorkerTag(e echo.Context) error {
 	sioUpdate := eventbus.NewWorkerTagUpdate(&dbTag)
 	f.broadcaster.BroadcastNewWorkerTag(sioUpdate)
 
-	return e.JSON(http.StatusOK, workerTagDBtoAPI(dbTag))
+	return e.JSON(http.StatusOK, workerTagDBtoAPI(&dbTag))
 }
 
 func workerSummary(w persistence.Worker) api.WorkerSummary {
@@ -479,7 +482,7 @@ func workerDBtoAPI(w persistence.Worker) api.Worker {
 	if len(w.Tags) > 0 {
 		tags := []api.WorkerTag{}
 		for i := range w.Tags {
-			tags = append(tags, workerTagDBtoAPI(*w.Tags[i]))
+			tags = append(tags, *workerTagDBtoAPI(w.Tags[i]))
 		}
 		apiWorker.Tags = &tags
 	}
@@ -487,7 +490,11 @@ func workerDBtoAPI(w persistence.Worker) api.Worker {
 	return apiWorker
 }
 
-func workerTagDBtoAPI(wc persistence.WorkerTag) api.WorkerTag {
+func workerTagDBtoAPI(wc *persistence.WorkerTag) *api.WorkerTag {
+	if wc == nil {
+		return nil
+	}
+
 	uuid := wc.UUID // Take a copy for safety.
 
 	apiTag := api.WorkerTag{
@@ -497,5 +504,5 @@ func workerTagDBtoAPI(wc persistence.WorkerTag) api.WorkerTag {
 	if len(wc.Description) > 0 {
 		apiTag.Description = &wc.Description
 	}
-	return apiTag
+	return &apiTag
 }
diff --git a/internal/manager/persistence/jobs.go b/internal/manager/persistence/jobs.go
index 8e17e73b..7941da4a 100644
--- a/internal/manager/persistence/jobs.go
+++ b/internal/manager/persistence/jobs.go
@@ -359,6 +359,18 @@ func (db *DB) FetchJob(ctx context.Context, jobUUID string) (*Job, error) {
 	if err != nil {
 		return nil, err
 	}
+
+	if sqlcJob.WorkerTagID.Valid {
+		workerTag, err := fetchWorkerTagByID(db.gormDB, uint(sqlcJob.WorkerTagID.Int64))
+		switch {
+		case errors.Is(err, sql.ErrNoRows):
+			return nil, ErrWorkerTagNotFound
+		case err != nil:
+			return nil, workerTagError(err, "fetching worker tag of job")
+		}
+		gormJob.WorkerTag = workerTag
+	}
+
 	return &gormJob, nil
 }
 
diff --git a/internal/manager/persistence/jobs_test.go b/internal/manager/persistence/jobs_test.go
index b7d352ee..7a2cb85e 100644
--- a/internal/manager/persistence/jobs_test.go
+++ b/internal/manager/persistence/jobs_test.go
@@ -75,6 +75,38 @@ func TestStoreAuthoredJobWithShamanCheckoutID(t *testing.T) {
 	assert.Equal(t, job.Storage.ShamanCheckoutID, fetchedJob.Storage.ShamanCheckoutID)
 }
 
+func TestStoreAuthoredJobWithWorkerTag(t *testing.T) {
+	ctx, cancel, db := persistenceTestFixtures(1 * time.Second)
+	defer cancel()
+
+	workerTagUUID := "daa811ac-6861-4004-8748-7700aebc244c"
+	require.NoError(t, db.CreateWorkerTag(ctx, &WorkerTag{
+		UUID:        workerTagUUID,
+		Name:        "🐈",
+		Description: "Mrieuw",
+	}))
+	workerTag, err := db.FetchWorkerTag(ctx, workerTagUUID)
+	require.NoError(t, err)
+
+	job := createTestAuthoredJobWithTasks()
+	job.WorkerTagUUID = workerTagUUID
+
+	err = db.StoreAuthoredJob(ctx, job)
+	require.NoError(t, err)
+
+	fetchedJob, err := db.FetchJob(ctx, job.JobID)
+	require.NoError(t, err)
+	require.NotNil(t, fetchedJob)
+
+	require.NotNil(t, fetchedJob.WorkerTagID)
+	assert.Equal(t, *fetchedJob.WorkerTagID, workerTag.ID)
+
+	require.NotNil(t, fetchedJob.WorkerTag)
+	assert.Equal(t, fetchedJob.WorkerTag.Name, workerTag.Name)
+	assert.Equal(t, fetchedJob.WorkerTag.Description, workerTag.Description)
+	assert.Equal(t, fetchedJob.WorkerTag.UUID, workerTagUUID)
+}
+
 func TestFetchTaskJobUUID(t *testing.T) {
 	ctx, cancel, db := persistenceTestFixtures(1 * time.Second)
 	defer cancel()
diff --git a/internal/manager/persistence/worker_tag.go b/internal/manager/persistence/worker_tag.go
index ed318a81..0fdebeec 100644
--- a/internal/manager/persistence/worker_tag.go
+++ b/internal/manager/persistence/worker_tag.go
@@ -53,6 +53,16 @@ func fetchWorkerTag(gormDB *gorm.DB, uuid string) (*WorkerTag, error) {
 	return &w, nil
 }
 
+// fetchWorkerTagByID fetches the worker tag using the given database instance.
+func fetchWorkerTagByID(gormDB *gorm.DB, id uint) (*WorkerTag, error) {
+	w := WorkerTag{}
+	tx := gormDB.First(&w, "id = ?", id)
+	if tx.Error != nil {
+		return nil, workerTagError(tx.Error, "fetching worker tag")
+	}
+	return &w, nil
+}
+
 func (db *DB) SaveWorkerTag(ctx context.Context, tag *WorkerTag) error {
 	if err := db.gormDB.WithContext(ctx).Save(tag).Error; err != nil {
 		return workerTagError(err, "saving worker tag")
diff --git a/web/app/src/components/jobs/JobDetails.vue b/web/app/src/components/jobs/JobDetails.vue
index 319c6ba4..6b1bd1cd 100644
--- a/web/app/src/components/jobs/JobDetails.vue
+++ b/web/app/src/components/jobs/JobDetails.vue
@@ -52,9 +52,15 @@
             {{ jobData.status }}
           </dd>
 
-          <dt class="field-type" title="Type">Type</dt>
+          <dt class="field-type" title="Job Type">Job Type</dt>
           <dd>{{ jobType ? jobType.label : jobData.type }}</dd>
 
+          <dt class="field-worker-tag" title="Worker Tag">Worker Tag</dt>
+          <dd v-if="jobData.worker_tag" :title="jobData.worker_tag.description">
+            {{ jobData.worker_tag.name }}
+          </dd>
+          <dd v-else class="no-worker-tag">All Workers</dd>
+
           <dt class="field-priority" title="Priority">Priority</dt>
           <dd>
             <PopoverEditableJobPriority :jobId="jobData.id" :priority="jobData.priority" />
@@ -289,4 +295,8 @@ export default {
   color: var(--indicator-color);
   font-weight: bold;
 }
+
+dd.no-worker-tag {
+  color: var(--color-text-muted);
+}
 </style>