Feature/hermes sharing #810
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Yeah, I've deployed it to hermes-dev but havent updated the nginx yet to accept much larger URIs... its on my list |
There was a problem hiding this comment.
I think these changes are great!
I've made some comments, questions and suggestions. Let me know if you'd like to take care of these or if you'd rather I worked on them myself.
Finally, I think it makes a lot of sense to add some tests to tom_dataproducts and tom_targets to make sure we are handling the conversions correctly and handling feedback as expected. Obviously we should also have some canary tests to make sure we properly catch any Hermes changes, but that should maybe wait until our canary test overhall.
tom_dataproducts/templates/tom_dataproducts/partials/dataproduct_list_for_target.html
Show resolved
Hide resolved
tom_dataproducts/templates/tom_dataproducts/partials/photometry_datalist_for_target.html
Outdated
Show resolved
Hide resolved
I've updated hermes-dev so it hopefully will work for larger datasets opening in hermes. I have it like 5Mb for header size, so not sure exactly how large but assume hundreds of datapoints would be fine now. |
|
Just wanted to update that I've made most of the changes you requested, but in testing I couldn't get our deploy of hermes to work with any reasonable sized preloading from the URI, so I've been working on another solution for preloading that doesn't depend on large URIs and should work with our deployment infrastructure. I hope to have that deployed along with the updates to this branch in the next day or so. |
|
I've addressed your original comments, and also refactored a bit to work with the different method of preloading data into the hermes form. This new method is deployed to hermes-dev.lco.global right now, and it doesn't depend on storing things in the URL so it should be safe for any size of data being shared. The only caveat is the data being preloaded is required to pass hermes validation, i.e. your datums must have an instrument or telescope set on them... But this is also a requirement for the data to be shared through hermes through the form anyway. |
| hermes_alert.save() | ||
| for tomtoolkit_photometry in datums: | ||
| tomtoolkit_photometry.message.add(hermes_alert) | ||
| except Exception: |
There was a problem hiding this comment.
Should this be a more specific exception or list of exceptions?
This seems like it could hide other errors.
There was a problem hiding this comment.
Hmm I think we probably want to catch any exception so the page doesn't crash. The response is returned so if it's an error response or doesn't contain the message we expect, then we will report the error back to the user.
|
Okay, I've addressed all the issues again. Thank you for your very in-depth testing! Let me know if there are any more issues. |
There was a problem hiding this comment.
I think this is mostly good. I did run into two last bugs but we can spin these off into different issues if you like.
Specifically, if I don't have a Hermes API key, I get a KeyError when trying to open in Hermes. This probably shouldn't happen. If we need the API key, we should handle it gracefully and give the "no API key" error message like in hermes.py:44.
The second issue, which I'm guessing is more on the Hermes side is that if I'm not currently logged into Hermes-dev, trying to open a message in Hermes tells me I need to log in, but then doesn't redirect me back to the pre-loaded message.
Yeah, it needs an API token since I don't want to allow non-authenticated users to preload (store) data in hermes. I'll see about gateing the open in hermes button around having at API token set at least.
Yes, this is unfortunate but I don't understand the login flow well enough to route it back to where you left from - I'm not even sure if that is possible with the scimma auth login flow... You can go back in your browser once you login since the preload is cached for 15 mintues, or you can click the open in hermes link again to open the page which will work once you've logged in since the login persists. |
… with sharing only target in hermes
…se into feature/hermes_sharing
|
Okay, I've hidden the open in hermes button if |
So this is a rather large PR - I'm happy to go through it with you after the AAS.
Major Changes: