feat: better first-run experience in Usage Metrics Dashboard #128
Conversation
Do we know what caused this? |
Let's not block shipping this on writing our first tests for this work. But we should do that separately from this PR. |
dotNomad
left a comment
dotNomad
left a comment
There was a problem hiding this comment.
What happens if it's published by a Publisher? The API that we use to set the integrations does not say whether it requires Publisher or Admin credentials! https://docs.posit.co/connect/api/#put-/v1/content/-guid-/oauth/integrations/associations
Looking at the codebase it looks like this API requires a Publisher or higher auth role so should be good there. We should make an internal ticket to clarify in the docs though.
I think we can improve the language. "Add Required Integration" might be confusing — especially because we use the term "add integration" both to mean "associate it with this content" and "create a new one". Maybe it's fine because it mirrors the "Add Integration" button in the sidebar, but we could also just call it "Complete Setup", or "Add Integration and Complete Setup".
I think the wording simplification is much nicer now that we don't have to explain as much.
I'd argue for using the verbiage "Add Integration", but I'm open to other options.
Are you thinking we will release the update in another PR once this merges?
| "This content requires a <strong>Visitor API Key</strong> ", | ||
| "integration to show users the content they have access to." |
There was a problem hiding this comment.
| "This content requires a <strong>Visitor API Key</strong> ", | |
| "integration to show users the content they have access to." | |
| "This content requires a Publisher or Admin <strong>Visitor API Key</strong> ", | |
| "integration to show users the content they have access to." |
Minor suggestion to add the "Publisher" / "Admin" prior to the bolded text in the hopes it gets read.
There was a problem hiding this comment.
After today's discussion, do you think we need to mention this detail to users? This text shows all the time, and I think that if an appropriate integration exists, we don't need to show those things to the user.
I do hear what you're saying though, good to have these two words more noticeable. Other options:
- We could have the entire message be conditional, and this detail could go only in the message we show if no integration exists. I think that could be better, I'm having second thoughts about the whole wording today.
- We could bold the words where they appear on line 324, the instructions on how to create a new integration.
There was a problem hiding this comment.
With the fully automated add flow I do think the distinction is a lot less important. On the no-integration-exists case I do think we need to be clear about what is needed.
There was a problem hiding this comment.
I know we discussed a fully automated flow in the demo session, but I thought afterwards we landed on still liking to have an interstitial with a button press. Happy to try fully automating it tomorrow though.
@dotNomad I don't know, it's pretty mystifying to me. I feel like I need to investigate tomorrow. Going to post in
@dotNomad I checked and it does work. I had an old PR that adds permissions language to ALL endpoints in Connect's API docs. I forget why it got stalled; it's still there. I was just thinking earlier today that I should try to find time to revive and update it.
@dotNomad I think I want to hold release until I make another change or two, but I could also just update the manifest.json for this version — now that you mention it there is no reason not to! |
jonkeane
left a comment
jonkeane
left a comment
There was a problem hiding this comment.
I'm approving, but we should make the text cleaner like I suggest, but if you make that change, this can merge without me re-reviewing
| "Click the button below to allow this content to use the integration <strong>'", | ||
| selected_integration$name, | ||
| "'</strong>.", | ||
| "<br><br>", | ||
| "For more information, see ", | ||
| "<a href='https://docs.posit.co/connect/user/oauth-integrations/#obtaining-a-visitor-api-key' ", | ||
| "target='_blank'>Visitor API Key section of the User Guide</a>." | ||
| ) |
There was a problem hiding this comment.
Lets change this to something like this, where the name of the integration is in the button and we mention that it's compatible rather than details about it.
This content requires a Visitor API Key integration to show users the content they have access to. A compatible integration is listed below.
For more information, see documentation on Visitor API Key integrations.
[+ Add the "Connect API - Publisher role" integration]
| @@ -0,0 +1,53 @@ | |||
| library(connectapi) | |||
There was a problem hiding this comment.
Do we have (an) issue(s) for adding this to connectapi?
There was a problem hiding this comment.
I'm going to make sure we do when I close this
There was a problem hiding this comment.
If you make the issues now you could comment here saying "when this other thing is merged delete these" whcih I have always found very very helpful
There was a problem hiding this comment.
Yep — planning to reference this in the issue(s) 🫡 . Making them is on my burndown of things to do for today (next, in fact!)
[edit] OH you meant add a comment in the code here referencing the other issues. Sorry, took me a second!
There was a problem hiding this comment.
There is an epic from last year referencing them. I'm going to add sub-issues for the individual recipes to make things more bite-size (so they better fit into guild work), as the epic lists six thirteen different endpoints to support.
dotNomad
left a comment
dotNomad
left a comment
There was a problem hiding this comment.
Thanks for all of those changes, this is such a huge improvement.
The Usage Metrics Dashboard requires a Connect API Key integration to load data. Previously, it popped up a panel with instructions on how to add an integration.
Now, there are two variants of the panel:
Play with this:
Possibly required features:
Feedback please on: