Marketplace upgrade URL 404s for non-admins


#1

Hi team,

I’ve just had a user report from a user that the plan upgrade link my app was asking him to visit was 404-ing. It turns out that’s because he wasn’t an admin for the organisation.

Would it be possible to show a friendlier screen to non-admins who click through to upgrade a marketplace plan? The person who contacted me assumed that the 404 was Dependabot’s fault, but without asking for additional permissions for me app (i.e., read permission on members) I don’t think there’s any way I could have know this user wasn’t an admin.

The URL in question was https://github.com/marketplace/dependabot/upgrade/3/16642985, which is constructed according to the guide here.


#2

Hey @greysteil,

thanks for the feedback, I’ve forwarded this to your engineering team, it sounds reasonable!

Could you please indicate how that user, which was no org-admin, ended up hitting the upgrade plan url?

Cheers,
Víctor


#3

Hey @vroldanbet,

Thanks for the response.

The user got the URL from within the Dependabot dashboard - when we went through the Marketplace approval process we were told that there should be a link for users to manage their Dependabot plan from within the dashboard. Since we don’t have any permission on members I don’t think there’s any way that we can tell whether a user is an org-admin or not, so we have to show that link to everyone.

Best,

Grey


#4

This has been fixed. Page now redirects and shows a message.

Thanks @greysteil!


#5

Awesome, thanks @mscoutermarsh!


#6

@mscoutermarsh @vroldanbet I’ve been having this same issue but for the GitHub App installation settings screen, i.e. https://github.com/organizations/good-software/settings/installations/97441. In my app I display a direct link to this page for users to be able to change the repos that they’ve granted access to Pull Reminders.

Users contact me confused about 404’s and I tell them they have to be an Owner to view it. Would it be possible to also have this lead to a more informative error page?


#7

@abinoda :wave:,

Thanks for reporting this, I just opened an issue to track this. I guess an even better UX would be to not show this at all for users that have no permission, but that may be tricky to implement depending on your apps permissions and I’m not even sure we provide the API to achieve such a thing :man_shrugging:


#8

I guess an even better UX would be to not show this at all for users that have no permission

I could show or hide the link depending on whether the user is an admin or not, but then inevitably I would have non-owner users in my app asking me “how do I add more repos?”. So I would probably still end up showing the link but routing it through my own page that would check the user’s permission and if not an owner, display a message like “You must be an owner”.

This would ultimately be quite a bit of work (not just for me, but for other integrators) compared to that GitHub page displaying a user-friendly message, and I can think of other scenarios (i.e. one user sending the link to another user) where it’d be helpful as well.


#9

Sorry, for got to ping you on my last reply @vroldanbet


#10

@abinoda yeah I understand that puts a lot of work on the integrators side and that’s not desirable, but I’d also add users need to be aware of what they are allowed to do wrt GitHub permission model.

I’m not sure if you are aware, but another nifty why to handle this is using installation requests, which we introduced in Q2 2018. You just need to redirect your user to:

http://github.localhost/apps/pull-reminders/installations/new/permissions?target_id=<installation_target>

And they will be given the opportunity to request new repositories, or even request installing in all repos. This triggers an email notification to target admins and can also be seen in the organization settings. An installation is not necessary, so users could also request installing an integration for the first time.

EDIT: you can also redirect admins there, and they will get automatically redirected to app settings page if they have the permissions.

HTH,
Víctor.


#11

@vroldanbet - installation requests are rad, but not perfect when an organisation needs to be on a paid plan (or at least have the option of selecting that). Any plans to build similar functionality for Marketplace apps?

Using the installation request flow at the moment would mean two cycles:

  • User requests access and admin completes
  • User tries to use app and realises they need a paid plan, for which they again have to ask admin to complete

#12

@greysteil - know what you mean because I opened a PR some time ago to solve exactly that: users would be redirected to marketplace first if the app is listed in marketplace. Unfortunately, it’s a bit more complicated than anticipated because some apps do billing on their own and that does not work for every app, so we are going to need more time before releasing this new feature. But yeah, there are plans and it’s in our roadmap!


#13

Awesome - thanks for all your work on this @vroldanbet.

One thin that would basically fix this from my app’s perspective is to move the organisation selection bit of the marketplace flow onto the first page (i.e., onto the github.com/marketplace/dependabot page).

I pinged the following mockups to marketplace@github.com but I appreciate there might be some complexities I’m not aware of:

If I had that then I’d feel happy about sending users who want to add Dependabot to a new account to github.com/marketplace/dependabot, and users that want to change Dependabot’s permissions on an existing account to http://github.com/apps/dependabot/installations/new/permissions?target_id=<installation_target>, and I don’t think anyone would get confused.

(Have pinged this to several GitHub places now because I’m not sure where the right home for it. Just confused, rather than trying to push a personal crusade!)


#14

@vroldanbet I had a chance to take another look at the installation request screen. I tested it with @greysteil’s Dependabot as a non-owner user in my org.

I can see how this could work but I feel like there are some significant UX issues, given the scenario where I am sending a user to this screen from a link in my app that says “grant access to more repos”.

  1. The “Install Dependabot” headline feels confusing, since the app is already installed
  2. Related to (1), but the repo selection UI feels confusing too. Dependabot is already installed and has access to at least one repo, yet there’s nothing that shows that on this screen. Upon closer inspection I could see that they were omitted under the “Only select repositories” dropdown.
  3. I have mixed feelings about the permissions being displayed in the way they are. For the initial app installation its important for the permissions to be shown upfront since it’s part of the decision making process, but seeing them again when trying to adjust repo access again makes me feel like I’m doing a fresh install here vs tweaking a setting for an existing install.

Right now I’m not sure I’d feel comfortable sending users to this screen instead of trying to send them to the app settings screen (especially if the app settings screen provided a clear “You must be an owner” error message).


#15

Hey Abi,

thanks for providing that feedback and sad to hear it does not meet your UX expectations. This is valuable feedback and I’m gonna open an internal issue to keep track of it.

  1. I think this can be easily solved! But please be aware users may request installation of apps that hasn’t been installed before. I think we can make more clear it’s an installation request.
  2. What would be your expectations here? Would you like to see which repo’s is the app installed to? Do you feel the user needs to know which repos is it installed to, or simply that there is an installation already and that the user would be proposing to change it?
  3. I see where you are coming from and how this could scare/discourage users from installing the app. I also think we should be transparent with users w.r.t permissions being granted. A user requesting installation may not be even aware the app is already installed and maybe has never had interaction with it: it’s only fair to show them what their are requesting so they don’t get any surprises here. Ultimately, it’s the admin approving this request. Do you feel/known if there would be more conversions if the permissions were hidden? Do you think that it would be a fair thing to do (to hide that information)? Maybe there are other ways to make it more obvious this is an installation request and that it does not change the current settings and that it’s the admin who has the last word?

I admit we haven’t gathered numbers on whether the current UX encourages people to perform requests, we can do a better job here. I’m happy to forward ideas / mocks wrt installation requests.


#16

Here are my UX thoughts and suggestions:

  • Just to make sure we are talking about the same thing: we are discussing the installation request shown to Members when an app is already installed, for the scenario where a user is sent directly to this screen via a link in my app (i.e. “Grant Pull Reminders access to more repos”).
  • “I think this can be easily solved! But please be aware users may request installation of apps that hasn’t been installed before.” <- I understand that the page is used for two different flows right now. I think it’d be nice if the page could have logic to check whether the app is or isn’t already installed, and then be designed accordingly. I think initial installation and changing repo access settings for an installed apps are pretty different purposes that should warrant specifically designed user experiences.
  • Regarding your point #2 – I think it’d be more user friendly if it displayed something like “Pull Reminders is installed and has access to __ repos”, and then a “Grant access to more repos” link. That could expand or dropdown a UI for choosing repos to selecting all.
  • Regarding your point #3 – I think it’s fine to display the permissions, but I think the manner in which they are presented could be changed a little bit. I think I would suggest changing the header to “Permissions granted to Pull Reminders” or something, instead of “…with these permissions”. I think its just the overall design of the page that I have concerns with because its designed for the initial installation versus updating an existing one.

Having said all of this, any thoughts on having the app settings page display a better error message (ie. “you must be an owner to view this page”) like I originally requested? I still think that would work well because it’d be crystal clear to the user what they need to do (ping an owner) to change repo access settings. It also minimizes the amount of “jumping around” between my app and GitHub’s UI, which in general leads to a better user experience. And finally, perhaps this would be less work for your team compared to design changes to the installation request screen?


#17

Abi,

thanks for giving us concrete ideas on how this could be improved. I’ve opened internal issues to improve the installation request experience, and also an issue to better report when accessing installation settings :bowing_man: