Model Copy: prior to implementing a model copy, allow curators to update the title and then save
vanaukenk opened this issue · 19 comments
Curators would like to be able to provide a title to the new model when copying.
UI work will be implemented separately.
From 2022-06-09 workbenches call.
Just a side note as we're going through a security audit right now: remember to sanitize your inputs.
@vanaukenk Just a note for developers to remember to make sure that the inputs are safe; standard software development practices.
@kltm @vanaukenk I don't understand why on this ticket though and why now? We are using forms and input everywhere, any specific problem? The sanitization issue should be handled by server Barista or Minerva rather than relying on frontend. I can just paste the evil code in the address bar. All it is doing is send url model copy with new title. But to answer the question, the modern frameworks takes care of sanitization automatically with good coding by interpolation, so no worries.
You can check by adding <script>alert("noctua attacked")</script> to any of the new workbenches input
This feels like a client-side issue. Is there anything for Minerva to do here?
@tmushayahama - can you elaborate on what the minerva work would be? Thx.
@vanaukenk @balhoff so ideally it should be minerva. So instead of saving the title as title = "Copy of " + title the title should be a variable of "newTitle" coming from the client via API. So maybe copyModel (id=id, title=newTitle). However, if it cannot be done easily on server code, then clients can call edit annotation API (to edit title) after copy model finishes (2 calls). It depends with @kltm if you want heavy clients because you have to do the same in every client like Graph Editor using.
I think it would depend on the exact workflow for users, but having this built into the API call for the server would give us more flexibility to try different things and make client programming easier, especially as we're still using different frameworks.
Currently, as I understand it, if I were to do use the current minerva API in the graph editor copy a model, the steps would be:
- call API
- get response, pull out new model ID
- call to add name to new model
- get affirmative response
- call to save model
- get affirmative response
- forward to new model
As opposed to a proposed:
- call API w/title
- get response, pull out new model ID
- forward to new model
As a client developer given how annoying multiple API shots in a safe series in JS can be, the latter looks a lot nicer. This is predicated, naturally, on some assumptions I have on the desired workflow that may not be true.
@tmushayahama @kltm thanks, that sounds good and should be doable.
@tmushayahama @balhoff
Where does this ticket stand from your POVs?
@vanaukenk I should be able to add the parameter to accept a title. I need to work on this this week.
@tmushayahama this is now implemented as part of #482. (2ea2c87).
It's also merged into the dev branch for testing.
@tmushayahama I forgot to say: the arg is called title.
noctua-dev now updated with the latest dev minerva code.
@tmushayahama @kltm I've updated the service to accept the title for the copied model using the same format Noctua has been using when a title is added to an existing model. Basically:
requests=[{"entity":"model","operation":"copy","arguments":{"model-id":"gomodel:MGI_MGI_88276", "values":[{"key":"title","value":"my new title for the copied model"}]}}]
This change is in the PR and has been merged into dev.
From @tmushayahama (on slack), will hold off on minerva update on noctua-dev until given the word.
@tmushayahama
This feature is really nice to have; thanks for adding it.
A have two, hopefully minor, requests for updating:
- In the pop-up window, let's change the text to 'Edit Title and Confirm Copy' to be really clear on what functionality is there.
- Once the curator has clicked on Save in the pop-up, we should grey out/dismantle the Copy Model button at the bottom of the copy model window. Otherwise, it's not clear whether the model actually copied and you need to click on Copy Model again.
We'll do the work for 1 above, adding more text to the title of the pop-up window.
For 2, we don't have a great all-around solution, so we won't do anything right now and wait for more feedback from curators.