Adds configuration requests and new observation workflow #4253

toddburnside · 2024-10-25T20:05:54Z

Adds configuration requests to Explore. For now, the only place you can request approval is on the summary tab in the validations table. This will change will a future PR. When an approval is requested, we optimistically update the affected observations, but they don't get updated via the API. This will change when the ConfigurationRequest in the API has the affected observations added to it and we can request them as part of the configurationRequestEdit event.

It also incorporates the new observation workflow, which removes obsStatus and obsActiveStatus. A new ObservationWorkflow is present on the Observation in the API, which contains a new calculated ObservationWorkflowState. It also contains a list of the valid states to which the observation can be transitioned, and the observation validations have been moved within the workflow. The ObsBadge has been updated to remove the Active State and change the status dropdown to update the new state where invalid transitions have been disabled. Currently there is no mutation for updating the state via the API, so any changes made locally will be overwritten on reload of the observation.

bundlemon · 2024-10-25T20:14:15Z

BundleMon

Files updated (2)

Status	Path	Size	Limits
✅	index-(hash).js	1.74MB (+5.28KB +0.3%)	-
✅	index-(hash).css	65.86KB (-61B -0.09%)	-

Unchanged files (6)

Status	Path	Size	Limits
✅	exploreworkers-(hash).js	606.06KB	-
✅	workbox-window.prod.es5-(hash).js	2.07KB	-
✅	plotworker-(hash).js	88B	-
✅	agsworker-(hash).js	84B	-
✅	itcworker-(hash).js	77B	-
✅	catalogworker-(hash).js	76B	-

Total files change +5.22KB +0.21%

Final result: ✅

View report in BundleMon website ➡️

Current branch size history | Target branch size history

cquiroz · 2024-10-25T20:41:49Z

explore/src/clue/scala/queries/common/ConfigurationSubquery.scala

+        instrument
+        mode
+        gmosNorthLongSlit {
+          grating


Why do we care only about grating?

🤷 Apparently that is the only piece of a gmos long slit observation that matters when they are handing out approvals.

cquiroz · 2024-10-25T20:42:46Z

explore/src/main/scala/explore/cache/ProgramCacheController.scala

+          ProgramSummaryQueriesGQL
+            .AllProgramConfigurationRequests[IO]
+            .query(props.programId, offset.orUnassign),
+        _.program.fold(List.empty)(_.configurationRequests.matches),


Ah. Good catch.

rpiaggio · 2024-10-25T20:44:07Z

explore/src/main/scala/explore/cache/ProgramCacheController.scala

@@ -132,12 +134,28 @@ object ProgramCacheController
        offset =>
          ProgramSummaryQueriesGQL
            .AllProgramObservations[IO]
-            .query(props.programId.toWhereObservation, offset.orUnassign),
+            .query(props.programId.toWhereObservation, offset.orUnassign)(using
+              ErrorPolicy.IgnoreOnData


Why is IgnoreOnData necessary?

I'm trying to remember. I'll get back to you shortly...

It's because we now get some errors with the observations for some things.

I guess we could review the ODB and see if that is really necessary. But, that is the way it is now,

Yeah, that one at least sounds more like a validation message than a query error.

But I agree that we want the IgnoreOnData in the presence of this. Maybe add a comment?

cquiroz · 2024-10-25T20:44:09Z

model/shared/src/main/scala/explore/model/ProgramSummaries.scala

  ): ProgramSummaries =
+    println(s"stati: ${obsList.map(_.workflow.state)}")


cquiroz

Lots of queries! LGTM

rpiaggio · 2024-10-25T20:47:24Z

explore/src/main/scala/explore/observationtree/ObsList.scala

@@ -301,13 +300,9 @@ object ObsList:
                      props.obsExecutionTimes.getPot(obsId).map(_.programTimeEstimate),
                      ObsBadge.Layout.ObservationsTab,
                      selected = selected,
-                      setStatusCB = obsEditStatus(obsId)
+                      setStateCB = obsEditStatus(obsId)


Do we want to rename to obsEditState too?

Yes. Good idea

I'll do that after I rebase on your PR. 👅

rpiaggio · 2024-10-25T20:47:47Z

explore/src/main/scala/explore/observationtree/ObsSummaryTable.scala

@@ -85,7 +85,7 @@ object ObsSummaryTable:
  private val GroupsColumnId          = ColumnId("groups")
  private val ObservationIdColumnId   = ColumnId("observation_id")
  private val ValidationCheckColumnId = ColumnId("validation_check")
-  private val StatusColumnId          = ColumnId("status")
+  private val StateColumnId           = ColumnId("status")


Do we want to rename to "state" too?

Yeah. I guess I missed this table for that somehow.

rpiaggio · 2024-10-25T20:55:17Z

model/shared/src/main/scala/explore/data/KeyedIndexedList.scala

@@ -99,6 +99,10 @@ case class KeyedIndexedList[K, A] private (private val list: TreeSeqMap[K, (A, N
  def updatedValueWith(key: K, f: A => A): KeyedIndexedList[K, A] =
    updatedWith(key, (v, i) => (f(v), i))

+  // WARNING - Do not use to update the part of the value used as a key
+  def unsafeMapValues(f: A => A): KeyedIndexedList[K, A] =


You could make this safe by passing a lens for the key so that it gets reinstated in case it's modified. In fact, this would be automatic if you use KIListMod(keyLens).mod

While digging through the KeyedIndexexList stuff to figure out how to make this work, I realized where I'm using this isn't participating in undo. 😢 I'll take care of that, and then figure this out.

Actually, I'm not sure this should participate in undo. We can't get rid of a ConfigurationRequest via the API. I guess once we get a mutation we could Withdraw the request on undo and set it back to Requested on redo? It's not quite the same as fully undoing it.

Sounds like we want to make those actions explicit. Like submit/retract a proposal.

Yeah. I think you're right. Undo doesn't seem appropriate here. Thanks for the input.

I'll deal with the unsafeMapValues in a new PR. I want to get this merged so Rob can continue cleaning up the odb/core.

rpiaggio · 2024-10-25T20:57:32Z

model/shared/src/main/scala/explore/model/Observation.scala

@@ -179,33 +183,76 @@ case class Observation(
  lazy val constraintsSummary: String =
    s"${constraints.imageQuality.label} ${constraints.cloudExtinction.label} ${constraints.skyBackground.label} ${constraints.waterVapor.label}"

+  private val executedStates =


ExecutedStates? Would it make sense to put it in Constants, or will it only be used here?

As far as I know, it's only needed here. I guess that could change in the future?

Ok, anyway we use CamelCase for constants, it's in the new code guidelines ;)

We have guidelines? 😮 Anyway, their just guidelines - kind of like the Pirates Code. 😆

rpiaggio · 2024-10-25T20:58:02Z

model/shared/src/main/scala/explore/model/ProgramSummaries.scala

  ): ProgramSummaries =
+    println(s"stati: ${obsList.map(_.workflow.state)}")


Too late. Carlos already reported that.

rpiaggio

Looks awesome, thank you.

LGTM, with some comments.

mergify bot added common explore model model-testkit labels Oct 25, 2024

cquiroz reviewed Oct 25, 2024

View reviewed changes

rpiaggio reviewed Oct 25, 2024

View reviewed changes

cquiroz reviewed Oct 25, 2024

View reviewed changes

cquiroz approved these changes Oct 25, 2024

View reviewed changes

rpiaggio reviewed Oct 25, 2024

View reviewed changes

rpiaggio approved these changes Oct 25, 2024

View reviewed changes

toddburnside added 2 commits October 27, 2024 18:34

Adds configuration requests and new observation workflow

88733d1

PR comments

39bd3e9

toddburnside force-pushed the config-requests branch from dbf709a to 39bd3e9 Compare October 28, 2024 01:37

toddburnside merged commit 0de1425 into master Oct 28, 2024
14 checks passed

toddburnside deleted the config-requests branch October 28, 2024 01:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds configuration requests and new observation workflow #4253

Adds configuration requests and new observation workflow #4253

toddburnside commented Oct 25, 2024

bundlemon bot commented Oct 25, 2024 •

edited

Loading

cquiroz Oct 25, 2024

toddburnside Oct 25, 2024

cquiroz Oct 25, 2024

toddburnside Oct 25, 2024

rpiaggio Oct 25, 2024

toddburnside Oct 25, 2024

toddburnside Oct 25, 2024

rpiaggio Oct 25, 2024

cquiroz Oct 25, 2024

cquiroz left a comment

rpiaggio Oct 25, 2024

toddburnside Oct 25, 2024

toddburnside Oct 25, 2024

rpiaggio Oct 25, 2024

toddburnside Oct 25, 2024

rpiaggio Oct 25, 2024

toddburnside Oct 25, 2024

toddburnside Oct 25, 2024 •

edited

Loading

rpiaggio Oct 25, 2024

toddburnside Oct 25, 2024

toddburnside Oct 28, 2024

rpiaggio Oct 25, 2024

toddburnside Oct 25, 2024

rpiaggio Oct 25, 2024

toddburnside Oct 25, 2024

rpiaggio Oct 25, 2024

toddburnside Oct 25, 2024

rpiaggio left a comment

		): ProgramSummaries =
		println(s"stati: ${obsList.map(_.workflow.state)}")

Adds configuration requests and new observation workflow #4253

Adds configuration requests and new observation workflow #4253

Conversation

toddburnside commented Oct 25, 2024

bundlemon bot commented Oct 25, 2024 • edited Loading

BundleMon

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cquiroz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toddburnside Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpiaggio left a comment

Choose a reason for hiding this comment

bundlemon bot commented Oct 25, 2024 •

edited

Loading

toddburnside Oct 25, 2024 •

edited

Loading