-
Notifications
You must be signed in to change notification settings - Fork 26
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
Add email notifications for processing jobs #6918
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Frontend looks good 👍 I left some feedback on wording ✍️
@emailBaseTemplate() { | ||
<p>Hi @{name},</p> | ||
|
||
<p>We wanted to let you know that WEBKNOSSOS has finished your background job for the dataset <i>@{datasetName}</i>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"We wanted to let you know that" could be removed in my opinion as it doesn't add much value.
border-radius: 4px; | ||
display: inline-block; | ||
"> | ||
Click here to open |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
open what? also, "click here" could be removed probably. maybe just "View results"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
<p>Hi @{name},</p> | ||
|
||
<p> | ||
oops, something went unexpected wrong when uploading and converting your dataset <i>@{datasetName}</i> for WEBKNOSSOS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like "unfortunately, your uploaded dataset could not be converted automatically for WEBKNOSSOS" would sound a bit more professional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
<p>Hi @{name},</p> | ||
|
||
<p> | ||
oops, something unexpected went wrong when working on your WEBKNOSSOS @{jobTitle} job for the <i>@{datasetName}</i> dataset. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, something unexpected went wrong when working on your WEBKNOSSOS @{jobTitle} job for the <i>@{datasetName}</i> dataset. | |
oops, something unexpected went wrong when executing your WEBKNOSSOS @{jobTitle} job for the <i>@{datasetName}</i> dataset. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backend LGTM :) I added only the suggestion we already talked about, to use the enum values directly and not wrap strings.
Also, I assume you’ll update the result link for meshes as discussed, without the return value
app/models/job/JobCommand.scala
Outdated
val COMPUTE_MESH_FILE = Value("compute_mesh_file") | ||
val CONVERT_TO_WKW = Value("convert_to_wkw") | ||
val EXPORT_TIFF = Value("export_tiff") | ||
val FIND_LARGEST_SEGMENT_ID = Value("find_largest_segment_id") | ||
val GLOBALIZE_FLOODFILLS = Value("globalize_floodfills") | ||
val INFER_NUCLEI = Value("infer_nuclei") | ||
val INFER_NEURONS = Value("infer_neurons") | ||
val MATERIALIZE_VOLUME_ANNOTATION = Value("materialize_volume_annotation") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
val COMPUTE_MESH_FILE = Value("compute_mesh_file") | |
val CONVERT_TO_WKW = Value("convert_to_wkw") | |
val EXPORT_TIFF = Value("export_tiff") | |
val FIND_LARGEST_SEGMENT_ID = Value("find_largest_segment_id") | |
val GLOBALIZE_FLOODFILLS = Value("globalize_floodfills") | |
val INFER_NUCLEI = Value("infer_nuclei") | |
val INFER_NEURONS = Value("infer_neurons") | |
val MATERIALIZE_VOLUME_ANNOTATION = Value("materialize_volume_annotation") | |
val compute_mesh_file, convert_to_wkw, export_tiff, find_largest_segment_id, globalize_floodfills, infer_nuclei, infer_neurons, materialize_volume_annotation = Value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
…sos into email-notification * 'email-notification' of github.com:scalableminds/webknossos: Update app/views/mail/jobSuccessfulSegmentation.scala.html Update PULL_REQUEST_TEMPLATE.md
…il-notification * 'master' of github.com:scalableminds/webknossos: Update screenshots (#6934) Support rendering negative floats (#6895) Fix loading of webworkers in dev mode (#6933) Restore cache buster for webworkers (#6932) Introduce data vault as storage backend abstraction (#6899) Fix download button for annotations when tiff export is disabled (#6931)
…sos into email-notification * 'email-notification' of github.com:scalableminds/webknossos:
@philippotto @fm3 I added your PR feedback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backend LGTM
This PR adds email notifications for long running WEBKNOSSOS worker jobs. The PR also adds a new API route for downloading Tiffs that does not require a user token (but handles authentication internally). This URI can easily be shared through email without "exposing" a user token.
Jobs with email notifications:
Jobs without email notifications:
Design Doc
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
### Requires WK worker update on deployment. See accompanying worker PR https://github.com/scalableminds/voxelytics/pull/3245(Please delete unneeded items, merge only when none are left open)