Skip to content

Commit

Permalink
Do not filter requirements by kind and other fixes
Browse files Browse the repository at this point in the history
This commit ensures that the InsertApprovalModel no longer filters
requirements based on entity kind. Now, it shows a requirment per
entity and the labelling is expanded for this feature. This is NOT the
final state we want to be in due to showing potentially too many and
overwhelming results for the same approver groups and individuals, but
this is a step in the right direction.

This commit also ensures that the approval status data is up to date
on the requester's side when requesting approvals. This is due to a
potential race or missing edge case when the change set moves from an
"open" to a "needsapproval" state. A corresponding "TODO" has been
added to the problem area, but the current fix is sufficient in the
interim.

In addition to these changes, both the requirements and actions are now
scrollable. We now also show the email for a user (though truncation may
not work at this time) as well as right padding for the status text
(e.g. "Waiting..." which has now been capitalized to match the other
states).

One final change, we now always allow users to reject the changes, even
if all approvals have been met.

Co-authored-by: Wendy Bujalski <[email protected]>
Signed-off-by: Nick Gerace <[email protected]>
  • Loading branch information
nickgerace and wendybujalski committed Jan 30, 2025
1 parent 6022059 commit 2e1f5a0
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 23 deletions.
9 changes: 9 additions & 0 deletions app/web/src/components/ApprovalFlowModal.vue
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,15 @@ function applyButtonHandler() {
}
} else {
changeSetsStore.REQUEST_CHANGE_SET_APPROVAL();
// TODO(nick): we should remove this in favor of only the WsEvent fetching. It appears that
// requesting the approval itself is insufficient for getting the latest approval status at
// the time of writing and the reason appears to be that the change set is "open" by the
// time the inset modal opens. Fortunately, this will work since we are the requester.
if (changeSet.value) {
changeSetsStore.FETCH_APPROVAL_STATUS(changeSet.value.id);
}
closeModalHandler();
}
}
Expand Down
60 changes: 37 additions & 23 deletions app/web/src/components/InsetApprovalModal.vue
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@
v-if="mode !== 'error'"
:class="
clsx(
'w-1/2 flex flex-col gap-sm p-sm shadow-2xl',
'lg:w-1/2 flex flex-col gap-sm p-sm shadow-2xl max-h-full overflow-hidden',
themeClasses('bg-shade-0 border', 'bg-neutral-900'),
)
"
>
<div class="flex flex-row gap-md mb-sm items-center">
<div class="flex flex-row flex-none gap-md mb-sm items-center">
<div class="flex flex-col gap-2xs">
<TruncateWithTooltip class="font-bold italic pb-2xs">
{{ changeSetName }}
Expand Down Expand Up @@ -75,15 +75,15 @@
<div
:class="
clsx(
'flex flex-row',
'flex flex-row flex-1 overflow-hidden',
featureFlagsStore.WORKSPACE_FINE_GRAINED_ACCESS_CONTROL
? 'place-content-evenly'
: 'justify-center',
)
"
>
<template v-if="featureFlagsStore.WORKSPACE_FINE_GRAINED_ACCESS_CONTROL">
<div class="flex flex-col text-sm gap-sm">
<div class="flex flex-col text-sm gap-sm overflow-y-auto">
<div
v-for="group in requirementGroups"
:key="group.key"
Expand All @@ -99,12 +99,12 @@
:key="vote.user.id"
class="flex flex-row gap-xs place-content-between"
>
<span>{{ vote.user.name }}</span>
<span class="italic">
<template v-if="!vote.status">waiting...</template>
<template v-else>{{ vote.status }}...</template>
</span>
<span>{{ vote.user.name }} ({{ vote.user.email }})</span>
<span class="flex flex-row">
<span class="italic pr-xs">
<template v-if="!vote.status">Waiting...</template>
<template v-else>{{ vote.status }}...</template>
</span>
<Icon
size="md"
name="thumbs-up"
Expand All @@ -127,7 +127,7 @@
</div>
</div>
</template>
<div class="flex flex-col gap-xs">
<div class="flex flex-col gap-xs overflow-hidden">
<div
v-if="!featureFlagsStore.WORKSPACE_FINE_GRAINED_ACCESS_CONTROL"
class="text-sm"
Expand All @@ -141,7 +141,7 @@
</div>
</div>
</div>
<div class="flex flex-row gap-sm justify-center mt-sm">
<div class="flex flex-row flex-none gap-sm justify-center mt-sm">
<VButton
label="Withdraw Request"
tone="warning"
Expand All @@ -158,7 +158,7 @@
"
>
<VButton
:disabled="mode !== 'requested' || iRejected"
:disabled="iRejected"
label="Reject Request"
tone="destructive"
icon="thumbs-down"
Expand Down Expand Up @@ -211,6 +211,7 @@ import { useAuthStore, WorkspaceUser } from "@/store/auth.store";
import { ChangeSetStatus, ChangeSet } from "@/api/sdf/dal/change_set";
import { useFeatureFlagsStore } from "@/store/feature_flags.store";
import { useViewsStore } from "@/store/views.store";
import { useAssetStore } from "@/store/asset.store";
import ActionsList from "./Actions/ActionsList.vue";
export type InsetApprovalModalMode =
Expand All @@ -219,6 +220,7 @@ export type InsetApprovalModalMode =
| "rejected"
| "error";
const assetStore = useAssetStore();
const authStore = useAuthStore();
const changeSetsStore = useChangeSetsStore();
const featureFlagsStore = useFeatureFlagsStore();
Expand All @@ -232,10 +234,8 @@ const props = defineProps<{
changeSet: ChangeSet;
}>();
type ReqType = "SchemaVariant" | "View";
interface Requirement {
key: string;
type: ReqType;
label: string;
votes: Vote[];
satisfied: boolean;
Expand All @@ -249,8 +249,6 @@ interface Vote {
const requirementGroups = computed(() => {
const groups: Requirement[] = [];
props.approvalData?.requirements.forEach((r) => {
if (!["CategorySchema", "View"].includes(r.entityKind)) return;
const userIds = Object.values(r.approverGroups)
.flat()
.concat(r.approverIndividuals);
Expand All @@ -268,14 +266,30 @@ const requirementGroups = computed(() => {
if (submitted) vote.status = submitted.status;
votes.push(vote);
});
const label =
r.entityKind === "CategorySchema"
? "Asset Changes"
: viewStore.viewsById[r.entityId]?.name ?? "a View";
const key = r.entityKind === "CategorySchema" ? r.entityKind : r.entityId;
let label = r.entityKind;
if (r.entityKind === "ApprovalRequirementDefinition") {
label = "Approval Requirement change";
} else if (r.entityKind === "Schema") {
const variantForSchema = assetStore.schemaVariants.find(
(thing) => thing.schemaId === r.entityId,
);
label = variantForSchema?.schemaName
? `Asset named ${variantForSchema?.schemaName}`
: "an Asset";
} else if (r.entityKind === "SchemaVariant") {
let name = assetStore.variantFromListById[r.entityId]?.displayName;
if (!name) {
name = assetStore.variantFromListById[r.entityId]?.schemaName;
}
label = name ? `Asset named ${name}` : "Asset (name not found)";
} else if (r.entityKind === "View") {
const name = viewStore.viewsById[r.entityId]?.name;
label = name ? `View named ${name}` : "View (name not found)";
}
groups.push({
key,
type: r.entityId as ReqType,
key: r.entityId,
label,
votes,
satisfied: r.isSatisfied,
Expand Down
1 change: 1 addition & 0 deletions app/web/src/store/feature_flags.store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ export function useFeatureFlagsStore() {
// You can override feature flags while working on a feature by setting them to true/false here
// for example:
// this.FEATURE_FLAG_NAME = false;
this.WORKSPACE_FINE_GRAINED_ACCESS_CONTROL = true;
},
}),
)();
Expand Down

0 comments on commit 2e1f5a0

Please sign in to comment.