unitaryfund / metriq-app

Web app for Metriq
https://metriq.info
Apache License 2.0
27 stars 19 forks source link

Submission success message needed #99

Closed willzeng closed 3 years ago

willzeng commented 3 years ago

After making a successful? submission I get this output:

(metriq) will@Wills-MacBook-Pro metriq % python submission_add.py
id='611a9c4f4f5e51bdf13b9e8e' userId=None submissionName='Google AI MAXCUT' submissionNameNormal='google ai maxcut' description='Maxcut with 3-regular graphs from a recent Google Quantum Team paper.' submittedDate='2021-08-16T17:11:43.690Z' submissionContentUrl='https://arxiv.org/abs/2004.04197' submissionThumbnailUrl=None approvedDate=None deletedDate=None upvotes=[]

A human readable message here rather that just a dictionary would help confirm what has happened.

vprusso commented 3 years ago

I think that definitely makes sense, it's a bit cryptic if you just see a response with no success/failure indication.

Not that this is the "gold standard", but the API for PWC seems to follow the above convention. Just because they don't have a message to indicate failure/success, does not necessarily indicate that we need to follow the same pattern of course.

I suppose part of this comes down to how we would like to display the message and to what part has the onus of relaying this message to the end user. For instance, would it be preferable to:

Any thoughts or preferences on the above, @WrathfulSpatula / @willzeng ?

WrathfulSpatula commented 3 years ago

@vprusso We actually do return a success message, with our API result. However, the client we've co-opted apparently chops the data field out of the API response and returns that, without the message field.

I'd like to adapt the client to also print the message field. However, if that proves thornier, would could alternatively print a generic human-readable success message to the user for any HTTP 200 OK response.

willzeng commented 3 years ago

Modifying the submission_add.py file to print more info would probably do the trick and still leave the actual API route clean

On Mon, Aug 16, 2021, 2:18 PM Daniel Strano @.***> wrote:

@vprusso https://github.com/vprusso We actually do return a success message, with our API result. However, the client we've co-opted apparently chops the data field out of the API response and returns that, without the message field.

I'd like to adapt the client to also print the message field. However, if that proves thornier, would could alternatively print a generic human-readable success message to the user for any HTTP 200 OK response.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/unitaryfund/metriq-app/issues/99#issuecomment-899719406, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABHZDAXILN7M335O6626IXLT5FI5ZANCNFSM5CIEQHAA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .

WrathfulSpatula commented 3 years ago

It's likely not the client, but just what we're printing out of the response, now that I think about it. The message is already in the API response, without needing to modify it.

WrathfulSpatula commented 3 years ago

@vprusso I see an easy way to do this, (without modifying the API). I'll take this.

WrathfulSpatula commented 3 years ago

I think this is addressed here: https://github.com/unitaryfund/metriq-client/pull/14

We just print the API response message field that we already return. However, we might want to change the example API call scripts as well, so that the user doesn't miss the message printing before the data result. I wouldn't remove the data printing, but I might just insert a blank line between the two.

vprusso commented 3 years ago

Think we can close this one, right, @WrathfulSpatula ?

WrathfulSpatula commented 3 years ago

We're now printing the API response messages, so I think so, @vprusso.