-
Notifications
You must be signed in to change notification settings - Fork 24
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
Folder And Dataset Metadata #7886
Conversation
* make login, registration and forgot password view more responsive to mobile devices - includes adding a link back to login for password reset view * make dashboard dataset view less awful * add changelog entry
* allow viewing a mapping * WIP: enable setting default active mappings in default view configuration * add default active mapping to default view configuration - add config settings to dataset settings default view config tab - auto load initial default mapping * fix bottom margin of layer settings - was broken in view mode with multiple segmentation layers; the "Save View Config..." Button had no spacing to towards * fix having the mapping settings enabled for layers with default mapping * WIP: move default mapping store proeprty to dataset layer config - undo moving mapping selection to a shared component * moved default active mapping from separate select to view config layer config - Also added validation check whether the mapping exists in the dataset view config settings * allow saving no default active mapping from view mode & model init to new dataset config version * rename defaultMapping to mapping * cache available mappings fetched from backend when checking config in dataset settings * remove logging statements * clean up * add changelog entry * apply pr feedback
* fix animation modal color layer validation * changelog * rename variable * apply pr feedback * improve error message of uint24 layers in animation modal --------- Co-authored-by: Michael Büßemeyer <[email protected]>
* validate BB size to be larger then 0 * changelog * appy PR feedback
73ae88a
to
1797a4e
Compare
…ontend dataset filtering - reason: ll.530 checks for sortedInfo.columnKey == null and columnKey is per default "" which makes this check fail and thus the chained map will never be executed
…der-dataset-properties
…der-dataset-properties
… WIP restyling of metadata / details table
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 looking good already :) I added a couple of comments.
I also tested a bit more and it’s looking pretty good :)
I did not yet review the migrations. I think we need to decide about the NULLability first, and also about what to do with the tags.
If I understand correctly, the dataset tags are now turned into the new metadata. If this is true, the tags should be removed from the SQL schema and the Dataset case class.
However, the dataset json still needs to serve the tags for the moment, until the libs client is adapted. I think it would be good if the old tags could be served there. Meaning that the json serialization should convert the relevant metadata to tags and serve them, alongside the new metadata attribute. In the same way, updating the tags from the libs should also stay possible, so an adapter for that is needed as well.
Also, could you create a libs issue to adapt the libs to the new metadata format and deprecate the tags there too?
Also note that #8006 was merged first so that the schema version in this PR needs to be increased to avoid conflicts.
for { | ||
dataset <- datasetDAO.findOneByNameAndOrganization(datasetName, request.identity._organization) ?~> notFoundMessage( | ||
datasetName) ~> NOT_FOUND | ||
maybeUpdatedMetadata = if (metadata.isDefined) metadata else dataset.metadata |
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.
should use getOrElse or just orElse if an option is required. I’d also inline it below, just like folderId.getOrElse(dataset._folder)
app/models/folder/Folder.scala
Outdated
Fox.successful(Folder(ObjectId(r._Id), r.name)) | ||
for { | ||
metadata <- parseMetadata(r.metadata) | ||
folder <- Fox.successful(Folder(ObjectId(r._Id), r.name, metadata)) |
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.
folder <- Fox.successful(Folder(ObjectId(r._Id), r.name, metadata)) | |
folder = Folder(ObjectId(r._Id), r.name, metadata) |
app/models/folder/Folder.scala
Outdated
private def parseWithParent(t: (String, String, Option[String], Option[String])): Fox[FolderWithParent] = | ||
for { | ||
metadata <- parseMetadata(t._3) | ||
folderWithParent <- Fox.successful(FolderWithParent(ObjectId(t._1), t._2, metadata, t._4.map(ObjectId(_)))) |
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.
folderWithParent <- Fox.successful(FolderWithParent(ObjectId(t._1), t._2, metadata, t._4.map(ObjectId(_)))) | |
folderWithParent = FolderWithParent(ObjectId(t._1), t._2, metadata, t._4.map(ObjectId(_))) |
tools/postgres/schema.sql
Outdated
@@ -123,14 +123,14 @@ CREATE TABLE webknossos.datasets( | |||
sharingToken CHAR(256), | |||
logoUrl VARCHAR(2048), | |||
sortingKey TIMESTAMPTZ NOT NULL, | |||
details JSONB, | |||
metadata JSONB DEFAULT '[]', |
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.
Why the DEFAULT '[]'
? The backend code seems to be able to handle NULL/None. If we want to go with DEFAULT '[]'
, we should do so everywhere and make the property NOT NULL in sql and also non-optional in scala/ts code
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.
🤔 My thought here was that it might be easier to work with at least an empty array instead of null. I added the null / None support to the code to ensure easier compatibility. This includes update calls not necessarily having to pass metadata (at least I hope that's the case, as I wanted to implement it that way).
But I can easily remove the default value from the column :) Or even go further and make metadata non-optional if you think this is preferable.
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.
I don’t really have a preference for default value versus nullable. But I don’t much like having both. Please pick what’s easier implementation-wise :) My guess is that removing the default value (and so have the default be NULL/None) is easier.
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.
Not is should be non nullable and with DEFAULT '[]
for dataset and folders :).
The update route for datasets parses the metadata field as optional and used the already existing metadata in the backend query in case the user / client did not provide any metadata (untested) 🙈
Ok what to do with tags :) I just talked with @normanrz and the conclusion is to keep tags as some are using them as well as the pretty important filtering feature. If we later want to merge the tags into the metadata, we can still do that. In case dataset/folder already has a metadata field named "tags" already exists, we just come up with something :). |
@@ -172,15 +177,15 @@ function DatasetDetails({ selectedDataset }: { selectedDataset: APIDatasetCompac | |||
</Tag> | |||
)} | |||
</div> | |||
{fullDataset && ( |
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 question? Is it ok to move the tags into the "spinner area"? Previously they weren'T but, the metadata should be within the "spinner area" as it depends on fullDataset
and IMO the newly added UI element should appear after the already known tags.
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.
sure, why not 👍
testing went well. Tags should be restored in the frontend :) |
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
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.
just tested the dev instance again and everything looks good 👍 I didn't test the migration. ifi that is tested well, feel free to merge and deploy
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.
@fm3 I just pushed a commit to fix the reversion. Could you please give me a final thumbs up?
I just tested the migration and reversion. both should work
|
||
-- Add existing info on species of metadata to details | ||
UPDATE webknossos.datasets | ||
SET details = jsonb_set(COALESCE(details, '{}'), '{species}', ( |
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.
The newest commit changed
SET details = jsonb_set(details, '{species}', (
to
SET details = jsonb_set(COALESCE(details, '{}'), '{species}', (
This was necessary, as the default value of the newly re-added details column was null and thus the jsonb_set
would also return null. This should work now :)
And @fm3 no need to test the migration again. I tested it and it should work ™️ 🙈 |
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.
🎉
(Could you please link the follow-up issue for search in the PR description?)
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Open Question
Issues:
(Please delete unneeded items, merge only when none are left open)