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.

Sign up for GitHub

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

model evaluation panel permission ux tweaks #5253

Merged
merged 1 commit into from
Dec 19, 2024
Merged

model evaluation panel permission ux tweaks #5253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 16 additions & 4 deletions app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluate.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,25 @@
import { MuiButton } from "@fiftyone/components";
import { Add } from "@mui/icons-material";
import { Box } from "@mui/material";
import React from "react";

export default function Evaluate(props: EvaluateProps) {
const { onEvaluate } = props;
const { onEvaluate, permissions } = props;
const canEvaluate = permissions.can_evaluate;
Copy link
Contributor

Choose a reason for hiding this comment

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

How where is this permission set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Permissions are initialized on loading the panel in file fiftyone/operators/builtins/panels/model_evaluation/__init__.py

return (
<MuiButton onClick={onEvaluate} startIcon={<Add />} variant="contained">
Evaluate Model
</MuiButton>
<Box
Copy link
Contributor

Choose a reason for hiding this comment

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

curious: is it better to use TooltipProvider

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh, is that a new component? Yah, it might be better. Sorry, I saw the comment after merging. I'll follow-up

title={canEvaluate ? "" : "You do not have permission to evaluate model"}
sx={{ cursor: canEvaluate ? "pointer" : "not-allowed" }}
>
<MuiButton
onClick={onEvaluate}
startIcon={<Add />}
variant="contained"
disabled={!canEvaluate}
>
Evaluate Model
</MuiButton>
</Box>
);
}

Expand Down
34 changes: 20 additions & 14 deletions app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Evaluation.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,20 +611,26 @@ export default function Evaluation(props: EvaluationProps) {
<Card sx={{ p: 2 }}>
<Stack direction="row" sx={{ justifyContent: "space-between" }}>
<Typography color="secondary">Evaluation notes</Typography>
{can_edit_note && (
<Box>
<IconButton
size="small"
color="secondary"
sx={{ borderRadius: 16 }}
onClick={() => {
setEditNoteState((note) => ({ ...note, open: true }));
}}
>
<EditNote />
</IconButton>
</Box>
)}
<Box
title={
can_edit_note
? ""
: "You do not have permission to edit evaluation notes"
}
sx={{ cursor: can_edit_note ? "pointer" : "not-allowed" }}
>
<IconButton
size="small"
color="secondary"
sx={{ borderRadius: 16 }}
onClick={() => {
setEditNoteState((note) => ({ ...note, open: true }));
}}
disabled={!can_edit_note}
>
<EditNote />
</IconButton>
</Box>
</Stack>
<EvaluationNotes notes={evaluationNotes} variant="details" />
</Card>
Expand Down
2 changes: 1 addition & 1 deletion app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Overview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ function EvaluationCard(props: EvaluationCardProps) {
}
/>
)}
{status && <Status status={status} />}
{status && <Status status={status} readOnly />}
</Stack>
{note && <EvaluationNotes notes={note} variant="overview" />}
</Card>
Expand Down
13 changes: 10 additions & 3 deletions app/packages/core/src/plugins/SchemaIO/components/NativeModelEvaluationView/Status.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ import React from "react";
import { useTriggerEvent } from "./utils";

export default function Status(props: StatusProps) {
const { status, canEdit, setStatusEvent } = props;
const { status, canEdit, readOnly, setStatusEvent } = props;
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider simplifying permission logic.

The component uses both canEdit and readOnly props for permission handling, which could lead to confusing states. Consider consolidating these into a single permission control.

-  const { status, canEdit, readOnly, setStatusEvent } = props;
+  const { status, canEdit: maybeCanEdit, setStatusEvent } = props;
+  const canEdit = maybeCanEdit && !props.readOnly;

-  if (!readOnly) {
+  if (canEdit) {

Also applies to: 10-10

const triggerEvent = useTriggerEvent();

if (canEdit) {
if (!readOnly) {
return (
<Select
sx={{
Expand All @@ -22,6 +22,12 @@ export default function Status(props: StatusProps) {
onChange={(e) => {
triggerEvent(setStatusEvent, { status: e.target.value });
}}
title={
canEdit
? ""
: "You do not have permission to update evaluation status"
}
disabled={!canEdit}
>
{STATUSES.map((status) => {
const color = COLOR_BY_STATUS[status];
Expand Down Expand Up @@ -63,7 +69,8 @@ export default function Status(props: StatusProps) {
type StatusProps = {
status?: string;
canEdit?: boolean;
setStatusEvent?: string;
readOnly?: boolean;
setStatusEvent: string;
};

const STATUS_LABELS = {
Expand Down
Loading