-
Notifications
You must be signed in to change notification settings - Fork 285
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
[MRG] Make metadata editable from the frontend #249
Conversation
…uychou/metadata-frontend
…uychou/metadata-frontend
Would there be controls around this to prevent accidental edits (and / or restrict when edits can or cannot be made)? |
@suhailshergill we can certainly add some checks wrt edits. Did you have particular kinds of checks in mind? We are currently not preserving a history of edits on the metadata, but we can look into logging that too, if this is an important use case. |
frontend/public/js/models.js
Outdated
@@ -1191,6 +1195,9 @@ $(function() { | |||
function dragStart(event, ui) { | |||
filterKey = $(ui.helper.context).data('key'); | |||
filterVal = $(ui.helper.context).data('val'); | |||
console.log('context', ui.helper.context); |
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.
Please remove before merging
@weienlee please take a look |
frontend/util/api.js
Outdated
if (value.indexOf('.') != -1) { | ||
valueType = 'double'; | ||
} else { | ||
valueType = parseInt(value) > 2**31 - 1 ? 'long': 'int'; |
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.
Math.pow(2, 31) -- throws error otherwise
Tested it out, looks good overall! Some things to fix:
|
@mpvartak my thought process (excuse the verbosity):
specifically, one concrete potential invariant might be that any metadata change (similar to a code change) must be reviewed. if so, how would we enforce that? |
@mpvartak Just addressed your comments, and added an edit button.
Could you give an example? Only values should be editable. |
Changes look good. Maybe have missed that before. Two small fixes:
OK for this PR to not address metadata versioning; that can be a separate PR. |
@suhailshergill Thanks for sharing your thoughts regarding metadata. Here's our thinking:
Let me know if you or Anton have other thoughts! |
@mpvartak looking forward to the gdoc with design options for metadata versioning |
@suhailshergill Yes, on my list! |
MODELDB_model_id