pypi / warehouse

The Python Package Index
https://pypi.org
Apache License 2.0
3.6k stars 963 forks source link

Improve PendingPublisher UX when project already exists #14232

Closed songololo closed 2 months ago

songololo commented 1 year ago

Currently, when an owner attempts to create a "Pending Publisher" for a project that already exists and that they already own, they get the same error as when they don't own the project:

https://github.com/pypi/warehouse/blob/97ff3692081c621c8ccc4906c6d0e228bd5225ae/warehouse/oidc/forms.py#L190

Ideally, we would just create a regular publisher for them instead, or at the very least provide a more meaningful error message in this case.


Original issue below:

What's the problem this feature will solve? It currently isn't possible to have a separate Trusted Publisher workflow for development and official workflows because more than one workflow is not permitted from the same project repository.

Describe the solution you'd like I have a Github Actions workflow publish_package_dev which publishes alpha and beta releases to test.pypi.org, and another workflow publish_package which publishes non development tagged releases to pypi.org.

However, when setting up a Trusted Publisher workflow for the test repository then it is no longer possible to add a Trusted Publisher for the non-test version of PyPi because it complains that "This project name is already in use".

di commented 1 year ago

Hi, @songololo, sorry you're having issues. If I understand correctly, it's unlikely that this is the actual issue, because pypi.org and test.pypi.org are completely separate.

What's the project name that you're trying to set up a trusted publisher for?

songololo commented 1 year ago

@di thanks for your response, the project repo / name is: https://github.com/benchmark-urbanism/cityseer-api

The PyPi project / package name is cityseer.

songololo commented 1 year ago

Apologies, on closer inspection I was trying to add the trusted publisher as a "pending publisher" instead of directly from he project's settings. Closing issue. Thanks for your help.

di commented 1 year ago

Reopening this w/ an updated title & description because I think there's an improvement we can make here.

woodruffw commented 10 months ago

I've asked @tetsuo-cpp to look at this!

tetsuo-cpp commented 9 months ago

I had a look at this today. I think if we just want to make a more informative error message, that should be reasonably straightforward.

But if we want to create a regular publisher in that case, there's some details to get right. At the moment, the page to add pending publishers lists projects with active publishers and not the active publishers themselves. So if we made the "add pending publisher" action add a regular publisher behind the scenes when the user owns the project, it wouldn't be visible from that page (but the project would be made visible in the "projects with publishers" list if it isn't already). Do we think that's acceptable? Or should we redesign this page to show all active publishers instead of just projects that have them?

My vote is for me to make an more informative error message and make an issue to add a regular publisher in this case (and answer the questions above) since I imagine it'll involve some discussion. Does anyone else have thoughts about this?

woodruffw commented 9 months ago

My vote is for me to make an more informative error message and make an issue to add a regular publisher in this case (and answer the questions above) since I imagine it'll involve some discussion. Does anyone else have thoughts about this?

That sounds good to me! I agree with your reasoning -- ideally the distinction between "pending" and "normal" publishers would be handled transparently, but I think we'll need more discussion about the UX there (and ensuring that we don't try to be too clever/guess the user's intent too much).

woodruffw commented 9 months ago

15366 does the first part of this, improving the error message.

facutuesca commented 3 months ago

15366 does the first part of this, improving the error message.

@woodruffw I've been looking into this. For the second part, I see that the suggestion was to silently create a normal Trusted Publisher if the user tries to incorrectly create a Pending Trusted Publisher for an existing project. But, as mentioned above, this assumes that the user's intention was to create a TP for that existing project. A scenario where this might be incorrect is:

  1. User has an existing package foo
  2. User wants to create a Pending Publisher for a new package, foo-bin
  3. User correctly navigates to Pending TP form, but due to confusion incorrectly fills in foo rather than foo-bin
  4. PyPI assumes the intention was to create a normal TP for foo and silently creates it
  5. User doesn't see any error, so assumes a Pending TP was created
  6. Even when the user discovers the error, they might not realise they should also delete the new foo TP.

Maybe the scenario is a bit contrived, but I have a suggestion that still allows the user to create a normal TP (during the error flow), while at the same time making the error explicit and giving the user a choice:

When the user submits the Pending TP form for a project that already exists (and is owned by the user), we can display the validation error message, but also include in it a link to the specific project's publishing settings, where the user can click and fill the correct form to create a normal TP.

More importantly, now that we're adding magic links to pre-fill the forms, we can use the data that the user already filled in the Pending TP form and pre-fill the normal TP form. It could look something like this:

image

And when the user clicks "here", the magic link redirects to a pre-filled TP form for existing-pkg:

image

WDYT?

woodruffw commented 3 months ago

Thanks for the detailed summary @facutuesca! I think that approach makes a lot of sense -- giving a user a link to do the correct thing rather than doing what we hope is the correct thing minimizes the amount of magic involved 🙂