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

Model Copy: prior to implementing a model copy, allow curators to update the title and then save #490

Closed
vanaukenk opened this issue Jun 9, 2022 · 19 comments

Comments

@vanaukenk
Copy link

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.

@kltm
Copy link
Member

kltm commented Jun 9, 2022

Just a side note as we're going through a security audit right now: remember to sanitize your inputs.

@vanaukenk
Copy link
Author

remember to sanitize your inputs

I'll need some more info on this one @kltm :-)

@kltm
Copy link
Member

kltm commented Jun 10, 2022

@vanaukenk Just a note for developers to remember to make sure that the inputs are safe; standard software development practices.

@tmushayahama
Copy link
Contributor

@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

@balhoff
Copy link
Member

balhoff commented Jun 13, 2022

This feels like a client-side issue. Is there anything for Minerva to do here?

@vanaukenk
Copy link
Author

@tmushayahama - can you elaborate on what the minerva work would be? Thx.

@tmushayahama
Copy link
Contributor

@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.

@kltm
Copy link
Member

kltm commented Jun 14, 2022

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.

@balhoff
Copy link
Member

balhoff commented Jun 14, 2022

@tmushayahama @kltm thanks, that sounds good and should be doable.

@tmushayahama
Copy link
Contributor

Thanks @kltm yes you described it better. @balhoff the latter would be ideal to have one API call

@vanaukenk
Copy link
Author

@tmushayahama @balhoff
Where does this ticket stand from your POVs?

@balhoff
Copy link
Member

balhoff commented Jun 21, 2022

@vanaukenk I should be able to add the parameter to accept a title. I need to work on this this week.

@balhoff
Copy link
Member

balhoff commented Jun 21, 2022

@tmushayahama this is now implemented as part of #482. (2ea2c87).

It's also merged into the dev branch for testing.

@balhoff
Copy link
Member

balhoff commented Jun 21, 2022

@tmushayahama I forgot to say: the arg is called title.

@kltm
Copy link
Member

kltm commented Jun 21, 2022

noctua-dev now updated with the latest dev minerva code.

@balhoff
Copy link
Member

balhoff commented Jun 28, 2022

@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.

@kltm
Copy link
Member

kltm commented Jun 28, 2022

From @tmushayahama (on slack), will hold off on minerva update on noctua-dev until given the word.

@vanaukenk
Copy link
Author

@tmushayahama
This feature is really nice to have; thanks for adding it.
A have two, hopefully minor, requests for updating:

  1. 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.
  2. 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.

@vanaukenk
Copy link
Author

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.

balhoff added a commit that referenced this issue Jul 11, 2022
* Initial implementation of model copy operation.

* Save model on initial copy. For #487.

* Support title argument for model copy (#490).

* Accept a title for model copy using the same mechanism as for normal title changes.

* Add test assertions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants