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

Show converting datasets #5597

Merged
merged 13 commits into from
Aug 25, 2021
Merged

Show converting datasets #5597

merged 13 commits into from
Aug 25, 2021

Conversation

youri-k
Copy link
Contributor

@youri-k youri-k commented Jul 1, 2021

URL of deployed dev instance (used for testing):

Steps to test:

  • upload dataset that needs conversion
  • go to dashboard
  • => see blue alert box which shows the uploaded dataset
  • => when upload is complete, alert box should be removed

Issues:


@youri-k youri-k requested a review from philippotto July 1, 2021 14:26
@youri-k youri-k self-assigned this Jul 1, 2021
@philippotto philippotto requested review from MichaelBuessemeyer and daniel-wer and removed request for philippotto July 2, 2021 13:11
@philippotto
Copy link
Member

I won't be able to get to this PR before my vacation. @MichaelBuessemeyer could you do the review and @daniel-wer maybe you could have a look at the UX and give some feedback? :)

@daniel-wer
Copy link
Member

@youri-k What would be a good way to test this? The deployed dev instances don't seem to have the conversion ability 🤔

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Some UX suggestions :)

Although I set up webknossos-worker as described in the Readme, didn't get any errors and celery is running, the conversions don't seem to run. I probably misconfigured something or ran the wrong celery command. Still, I did see the blue banner (just with an UNKNOWN state).

frontend/javascripts/dashboard/dataset_view.js Outdated Show resolved Hide resolved
frontend/javascripts/dashboard/dataset_view.js Outdated Show resolved Hide resolved
frontend/javascripts/dashboard/dataset_view.js Outdated Show resolved Hide resolved
<Row key={job.id} gutter={16}>
<Col span={10}>
<Tooltip title={job.state}>{stateToIcon[job.state]}</Tooltip>
{` ${job.datasetName || "UNKNOWN"}`}
Copy link
Member

Choose a reason for hiding this comment

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

Is there any additional info that could be displayed here? For example, when the job was triggered and/or who triggered it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your suggestion. I included the trigger date. All of the jobs that are visible are triggered by yourself, so there is not a benefit of displaying the "owner" of the job

Copy link
Member

Choose a reason for hiding this comment

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

are you sure about this? won't admins see all jobs?

Copy link
Member

Choose a reason for hiding this comment

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

(I think, you are right; even as a super admin, the jobs page also shows only my jobs)

@philippotto philippotto self-requested a review August 5, 2021 13:33
@philippotto philippotto assigned philippotto and unassigned youri-k Aug 19, 2021
@philippotto philippotto marked this pull request as ready for review August 20, 2021 13:22
@philippotto
Copy link
Member

Great work, @youri-k! I made a couple of changes (e.g., I'm also showing successful conversions, since I think, that this will also be a nice feedback). I also added an auto refresh of the running jobs.

Here's a screenshot:
image

@MichaelBuessemeyer could you have a look at my last commit and give a final go?

Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

@philippotto I only found two minor things. After they are resolved, this pr should be ready to go 🚢

Comment on lines 27 to 52
export const TOOLTIP_MESSAGES_AND_ICONS = {
UNKNOWN: {
tooltip:
"The status information for this job could not be retreived. Please try again in a few minutes, or contact us if you need assistance.",
icon: <QuestionCircleTwoTone twoToneColor="grey" />,
},
SUCCESS: {
tooltip: "This job has successfully been executed.",
icon: <CheckCircleTwoTone twoToneColor="#49b21b" />,
},
PENDING: {
tooltip: "This job will run as soon as a worker becomes available.",
icon: <ClockCircleTwoTone twoToneColor="orange" />,
},
STARTED: { tooltip: "This job is currently running.", icon: <LoadingOutlined /> },
FAILURE: {
tooltip:
"Something went wrong when executing this job. Feel free to contact us if you need assistance.",
icon: <CloseCircleTwoTone twoToneColor="red" />,
},
MANUAL: {
tooltip:
"The job will be handled by an admin shortly, since it could not be finished automatically. Please check back here soon.",
icon: <ToolTwoTone twoToneColor="orange" />,
},
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest using the predefined colors:
image

Copy link
Member

Choose a reason for hiding this comment

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

I re-used these colors, but unfortunately I had to repeat the hex values, as the icon components don't support css variables (such as --ant-success). I checked that the icons look okay in light and dark themes. So, I think, this should be ready now.

Copy link
Contributor

Choose a reason for hiding this comment

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

as the icon components don't support css variables

What a bummer 😕

frontend/javascripts/dashboard/dataset_view.js Outdated Show resolved Hide resolved
frontend/javascripts/dashboard/dataset_view.js Outdated Show resolved Hide resolved
frontend/javascripts/dashboard/dataset_view.js Outdated Show resolved Hide resolved
@philippotto philippotto enabled auto-merge (squash) August 25, 2021 07:03
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@philippotto philippotto merged commit 79eef75 into master Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Datasets which are being converted should already be shown in dashboard
4 participants