Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent asterism updates for executed observations, add undo/redo #4056

Merged
merged 1 commit into from
Aug 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,5 @@
}
},
"editor.formatOnSave": true,
"stylelint.validate": ["css", "scss"],
"metals.inlayHints.implicitArguments.enable": true
"stylelint.validate": ["css", "scss"]
}
21 changes: 21 additions & 0 deletions explore/src/clue/scala/queries/common/AsterismQueriesGQL.scala
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,25 @@ object AsterismQueriesGQL {
}
"""
}

// Group these 2 mutations together so that the targetEdit and observationEdit subscriptions come
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this guaranteed to work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we reduce the transit time involved, but they still aren't real close together. That timing depends on when the database updates happen for the 2 mutations, I guess. Still seemed better than doing them individually. 🤷‍♂️

// close together and get grouped by the programCache so we don't have an invalid state for the UI.
@GraphQL
trait UpdateTargetsAndAsterismsMutation extends GraphQLOperation[ObservationDB] {
val document = """
mutation($targetInput: UpdateTargetsInput!,$asterismInput: UpdateAsterismsInput!) {
updateTargets(input: $targetInput) {
targets {
id
}
}

updateAsterisms(input: $asterismInput) {
observations {
id
}
}
}
"""
}
}
31 changes: 31 additions & 0 deletions explore/src/main/scala/explore/common/AsterismQueries.scala
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import explore.DefaultErrorPolicy
import explore.model.Observation
import lucuma.core.model.Target
import lucuma.schemas.ObservationDB
import lucuma.schemas.ObservationDB.Enums.Existence
import lucuma.schemas.ObservationDB.Types.*
import lucuma.schemas.odb.input.*
import queries.common.AsterismQueriesGQL.*
Expand Down Expand Up @@ -60,3 +61,33 @@ object AsterismQueries:
SET = EditAsterismsPatchInput(ADD = toAdd.assign, DELETE = toRemove.assign)
)
UpdateAsterismsMutation[F].execute(input).void

def undeleteTargetsAndAddToAsterism[F[_]: Async](
obsIds: List[Observation.Id],
targetIds: List[Target.Id]
)(using FetchClient[F, ObservationDB]): F[Unit] =
val targetInput = UpdateTargetsInput(
WHERE = targetIds.toWhereTargets.assign,
SET = TargetPropertiesInput(existence = Existence.Present.assign),
includeDeleted = true.assign
)
val asterismInput = UpdateAsterismsInput(
WHERE = obsIds.toWhereObservation.assign,
SET = EditAsterismsPatchInput(ADD = targetIds.assign)
)
UpdateTargetsAndAsterismsMutation[F].execute(targetInput, asterismInput).void

def deleteTargetsAndRemoveFromAsterism[F[_]: Async](
obsIds: List[Observation.Id],
targetIds: List[Target.Id]
)(using FetchClient[F, ObservationDB]): F[Unit] =
val targetInput = UpdateTargetsInput(
WHERE = targetIds.toWhereTargets.assign,
SET = TargetPropertiesInput(existence = Existence.Deleted.assign),
includeDeleted = true.assign
)
val asterismInput = UpdateAsterismsInput(
WHERE = obsIds.toWhereObservation.assign,
SET = EditAsterismsPatchInput(DELETE = targetIds.assign)
)
UpdateTargetsAndAsterismsMutation[F].execute(targetInput, asterismInput).void
22 changes: 22 additions & 0 deletions explore/src/main/scala/explore/common/TargetQueries.scala
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,15 @@ package explore.common
import cats.effect.Sync
import cats.syntax.all.given
import clue.FetchClient
import clue.data.syntax.*
import explore.DefaultErrorPolicy
import explore.utils.*
import lucuma.core.model.Program
import lucuma.core.model.Target
import lucuma.schemas.ObservationDB
import lucuma.schemas.ObservationDB.Enums.Existence
import lucuma.schemas.ObservationDB.Types.TargetPropertiesInput
import lucuma.schemas.ObservationDB.Types.UpdateTargetsInput
import lucuma.schemas.odb.input.*
import queries.common.TargetQueriesGQL

Expand All @@ -27,3 +31,21 @@ object TargetQueries:
.execute(target.toCreateTargetInput(programId))
.map(_.createTarget.target.id)
.flatTap(id => ToastCtx[F].showToast(s"Created new target [$id]"))

def setTargetExistence[F[_]: Sync](
programId: Program.Id,
targetId: Target.Id,
existence: Existence
)(using FetchClient[F, ObservationDB]): F[Unit] =
TargetQueriesGQL
.UpdateTargetsMutation[F]
.execute(
UpdateTargetsInput(
WHERE = targetId.toWhereTarget
.copy(program = programId.toWhereProgram.assign)
.assign,
SET = TargetPropertiesInput(existence = existence.assign),
includeDeleted = true.assign
)
)
.void
7 changes: 4 additions & 3 deletions explore/src/main/scala/explore/tabs/AsterismEditorTile.scala
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@ import crystal.react.*
import explore.components.Tile
import explore.components.ui.ExploreStyles
import explore.config.ObsTimeEditor
import explore.model.AsterismIds
import explore.model.GlobalPreferences
import explore.model.ObsConfiguration
import explore.model.ObsIdSet
import explore.model.ObsTabTilesIds
import explore.model.ObservationsAndTargets
import explore.model.OnAsterismUpdateParams
import explore.model.OnCloneParameters
import explore.model.TargetEditObsInfo
import explore.model.enums.TileSizeState
Expand All @@ -42,14 +42,14 @@ object AsterismEditorTile:
userId: Option[User.Id],
programId: Program.Id,
obsIds: ObsIdSet,
asterismIds: View[AsterismIds],
obsAndTargets: UndoSetter[ObservationsAndTargets],
configuration: Option[BasicConfiguration],
vizTime: View[Option[Instant]],
obsConf: ObsConfiguration,
currentTarget: Option[Target.Id],
setTarget: (Option[Target.Id], SetRouteVia) => Callback,
onCloneTarget: OnCloneParameters => Callback,
onAsterismUpdate: OnAsterismUpdateParams => Callback,
obsInfo: Target.Id => TargetEditObsInfo,
searching: View[Set[Target.Id]],
title: String,
Expand All @@ -59,6 +59,7 @@ object AsterismEditorTile:
backButton: Option[VdomNode] = none
)(using FetchClient[IO, ObservationDB], Logger[IO]): Tile = {
// Save the time here. this works for the obs and target tabs
// It's OK to save the viz time for executed observations, I think.
val vizTimeView =
vizTime.withOnMod(t => ObsQueries.updateVisualizationTime[IO](obsIds.toList, t).runAsync)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs an update I think

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it looks like it is still called ObsQueries.updateVisualizationTime in your PR. 😄 So, I don't think this needs to change. Unless there is another reason.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lazy cquiroz


Expand All @@ -79,13 +80,13 @@ object AsterismEditorTile:
uid,
programId,
obsIds,
asterismIds,
obsAndTargets,
vizTime,
obsConf,
currentTarget,
setTarget,
onCloneTarget,
onAsterismUpdate,
obsInfo,
searching,
renderInTitle,
Expand Down
8 changes: 7 additions & 1 deletion explore/src/main/scala/explore/tabs/ObsTabTiles.scala
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import explore.model.LoadingState
import explore.model.Observation
import explore.model.Observation.observingMode
import explore.model.ObservationsAndTargets
import explore.model.OnAsterismUpdateParams
import explore.model.OnCloneParameters
import explore.model.ProgramSummaries
import explore.model.TargetEditObsInfo
Expand Down Expand Up @@ -477,19 +478,24 @@ object ObsTabTiles:
def onCloneTarget(params: OnCloneParameters): Callback =
setCurrentTarget(params.idToAdd.some, SetRouteVia.HistoryReplace)

def onAsterismUpdate(params: OnAsterismUpdateParams): Callback =
val targetForPage: Option[Target.Id] =
if (params.areAddingTarget) params.targetId.some else none
setCurrentTarget(targetForPage, SetRouteVia.HistoryReplace)

val targetTile: Tile =
AsterismEditorTile.asterismEditorTile(
props.vault.userId,
props.programId,
ObsIdSet.one(props.obsId),
asterismIds,
props.obsAndTargets,
basicConfiguration,
vizTimeView,
obsConf,
props.focusedTarget,
setCurrentTarget,
onCloneTarget,
onAsterismUpdate,
getObsInfo(props.obsId),
props.searching,
"Targets",
Expand Down
71 changes: 31 additions & 40 deletions explore/src/main/scala/explore/tabs/TargetTabContents.scala
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ import lucuma.ui.optics.*
import lucuma.ui.reusability.given
import lucuma.ui.syntax.all.given
import monocle.Iso
import monocle.Traversal
import queries.schemas.odb.ObsQueries

import java.time.Instant
Expand Down Expand Up @@ -128,27 +127,6 @@ object TargetTabContents extends TwoPanels:
setPage(Focused.None)
)(targetId => setPage(Focused.target(targetId)))

def onModAsterismsWithObs(
groupIds: ObsIdSet,
editedIds: ObsIdSet
)(ps: ProgramSummaries): Callback =
findAsterismGroup(editedIds, ps.asterismGroups).foldMap { tlg =>
// We should always find the group.
// If a group was edited while closed and it didn't create a merger, keep it closed,
// otherwise expand all affected groups.
props.expandedIds
.mod { eids =>
val withOld =
if (groupIds === editedIds) eids
else eids + groupIds.removeUnsafe(editedIds)
val withOldAndNew =
if (editedIds === tlg.obsIds && editedIds === groupIds) withOld
else withOld + tlg.obsIds

withOldAndNew.filter(ids => ps.asterismGroups.contains(ids)) // clean up
}
}

val backButton: VdomNode =
makeBackButton(props.programId, AppTab.Targets, selectedView, ctx)

Expand Down Expand Up @@ -191,8 +169,6 @@ object TargetTabContents extends TwoPanels:
idsToEdit: ObsIdSet,
asterismGroup: AsterismGroup
): List[Tile] = {
val groupIds = asterismGroup.obsIds

val getVizTime: ProgramSummaries => Option[Instant] = a =>
for
id <- idsToEdit.single
Expand All @@ -213,20 +189,6 @@ object TargetTabContents extends TwoPanels:
)
.getOrElse(ps)

val traversal: Traversal[ObservationList, AsterismIds] =
Iso
.id[ObservationList]
.filterIndex((id: Observation.Id) => idsToEdit.contains(id))
.andThen(KeyedIndexedList.value)
.andThen(Observation.scienceTargetIds)

val asterismView: View[AsterismIds] =
CloneListView(
props.programSummaries.model
.withOnMod(onModAsterismsWithObs(groupIds, idsToEdit))
.zoom(ProgramSummaries.observations.andThen(traversal))
)

val vizTimeView: View[Option[Instant]] =
props.programSummaries.model.zoom(getVizTime)(modVizTime)

Expand Down Expand Up @@ -276,7 +238,6 @@ object TargetTabContents extends TwoPanels:

def onCloneTarget4Asterism(params: OnCloneParameters): Callback =
// props.programSummaries.get will always contain the original groups. On creating,
// params.summaries would contain the groups after the clone, but that isn't as useful here.
val allOriginalGroups = props.programSummaries.get.asterismGroups
.filterForObsInSet(params.obsIds)
.map(_._1)
Expand Down Expand Up @@ -307,19 +268,49 @@ object TargetTabContents extends TwoPanels:
setCurrentTarget(idsToEdit.some)(params.originalId.some, SetRouteVia.HistoryReplace)
})

def onAsterismUpdate(params: OnAsterismUpdateParams): Callback =
val originalGroups = props.programSummaries.get.asterismGroups
// props.programSummaries.get will always contain the original groups, so we should find the group
originalGroups
.findContainingObsIds(params.obsIds)
.foldMap(group =>
val newAsterism =
if (params.isAddAction) group.targetIds + params.targetId
else group.targetIds - params.targetId
val existingGroup = originalGroups.findWithTargetIds(newAsterism)
val mergedObs = existingGroup.map(_.obsIds ++ params.obsIds)
val obsIdsAfterAction = mergedObs.getOrElse(params.obsIds)
val unmodified = group.obsIds -- params.obsIds
val setExpanded = unmodified.fold {
if (params.isUndo) props.expandedIds.mod(_ - obsIdsAfterAction + group.obsIds)
else props.expandedIds.mod(_ - group.obsIds + obsIdsAfterAction)
}(unmod =>
if (params.isUndo) props.expandedIds.mod(_ - obsIdsAfterAction - unmod + group.obsIds)
else props.expandedIds.mod(_ - group.obsIds + obsIdsAfterAction + unmod)
)
val targetForPage: Option[Target.Id] =
if (params.areAddingTarget) params.targetId.some
else none // if we're deleting, let UI focus the first one in the asterism
val setPage =
if (params.isUndo)
setCurrentTarget(idsToEdit.some)(targetForPage, SetRouteVia.HistoryReplace)
else setCurrentTarget(params.obsIds.some)(targetForPage, SetRouteVia.HistoryReplace)
setExpanded >> setPage
)

val asterismEditorTile =
AsterismEditorTile.asterismEditorTile(
props.userId,
props.programId,
idsToEdit,
asterismView,
props.obsAndTargets,
configuration,
vizTimeView,
ObsConfiguration(configuration, none, constraints, wavelength, none, none, none, none),
props.focused.target,
setCurrentTarget(idsToEdit.some),
onCloneTarget4Asterism,
onAsterismUpdate,
getObsInfo(idsToEdit.some),
props.searching,
title,
Expand Down
Loading
Loading