Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error messages when a pending Trusted Publisher's project name already exists #17405

Merged

Conversation

facutuesca
Copy link
Contributor

@facutuesca facutuesca commented Jan 13, 2025

The problem

There are some cases where we validate the name of a new project, where the error messages could be better:

The implementation

This PR removes the enum used for describing the invalid project name errors, replacing it with simple classes that allow including some metadata in each error object. This metadata allows us to tell if the current user is the owner of the existing project, which we then use to display better error messages.

# Before
class ProjectNameUnavailableReason(enum.Enum):
    Invalid = "invalid"
    Stdlib = "stdlib"
    AlreadyExists = "already-exists"
    Prohibited = "prohibited"
    TooSimilar = "too-similar"

# After
class ProjectNameUnavailableInvalid:
    pass
class ProjectNameUnavailableStdlib:
    pass
class ProjectNameUnavailableProhibited:
    pass

class ProjectNameUnavailableExisting:
    def __init__(self, existing_project: Project):
        self.existing_project: Project = existing_project
class ProjectNameUnavailableSimilar:
    def __init__(self, similar_project: Project):
        self.similar_project: Project = similar_project

ProjectNameUnavailableError = (
    ProjectNameUnavailableInvalid
    | ProjectNameUnavailableStdlib
    | ProjectNameUnavailableExisting
    | ProjectNameUnavailableProhibited
    | ProjectNameUnavailableSimilar
)

Screenshots

Creating a pending Trusted Publisher

Project already exists and user is owner

Screenshot 2025-01-13 at 22 39 42

Project already exists and user is not owner

Screenshot 2025-01-13 at 22 42 15

This fixes #17392

cc @di @woodruffw

@samuelcolvin
Copy link

Thanks so much for fixing so quickly ❤️ .

warehouse/oidc/forms/_core.py Outdated Show resolved Hide resolved
@facutuesca facutuesca force-pushed the ft/improve-upload-pkg-error-msgs branch from 1e208d0 to 882ebe3 Compare January 14, 2025 19:15
@facutuesca facutuesca changed the title Improve error messages when a new project's name is too similar to an existing one Improve error messages when a pending Trusted Publisher's project name already exists Jan 14, 2025
warehouse/packaging/models.py Outdated Show resolved Hide resolved
tests/unit/oidc/forms/test_activestate.py Outdated Show resolved Hide resolved
warehouse/packaging/models.py Outdated Show resolved Hide resolved
warehouse/packaging/services.py Outdated Show resolved Hide resolved
@facutuesca facutuesca force-pushed the ft/improve-upload-pkg-error-msgs branch from 882ebe3 to 025ea4a Compare January 31, 2025 21:05
@di di requested a review from miketheman February 3, 2025 16:27
Copy link
Member

@miketheman miketheman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coming along nicely, I think we're almost there! A few last bits to consider, and then I think we're ready to go. Let me know when it's ready for re-review by clicking the 🔄 button on my username.

warehouse/packaging/interfaces.py Outdated Show resolved Hide resolved
warehouse/packaging/interfaces.py Outdated Show resolved Hide resolved
warehouse/packaging/services.py Outdated Show resolved Hide resolved
This change improves the error messages when uploading a new project
or creating a pending Trusted Publisher, when the new project's name
already exists or is too similar to an existing project.

Signed-off-by: Facundo Tuesca <[email protected]>
Signed-off-by: Facundo Tuesca <[email protected]>
Fix the check for projects with too-similar names when
there is more than one existing project with the same
ultranormalized name.

Signed-off-by: Facundo Tuesca <[email protected]>
Signed-off-by: Facundo Tuesca <[email protected]>
Also change the TooSimilar error to store the conflicting
project name instead of a reference to the entire project.

Signed-off-by: Facundo Tuesca <[email protected]>
Signed-off-by: Facundo Tuesca <[email protected]>
@facutuesca facutuesca force-pushed the ft/improve-upload-pkg-error-msgs branch from 025ea4a to 88ffa21 Compare February 6, 2025 13:28
@facutuesca facutuesca requested a review from miketheman February 6, 2025 13:29
Copy link
Member

@miketheman miketheman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done! :shipit:

@miketheman miketheman merged commit 4775901 into pypi:main Feb 6, 2025
20 checks passed
@miketheman miketheman deleted the ft/improve-upload-pkg-error-msgs branch February 6, 2025 13:41
@ewdurbin
Copy link
Member

ewdurbin commented Feb 6, 2025

ewdurbin added a commit that referenced this pull request Feb 6, 2025
changing from return value to exception was not caught due to stubs.

it's unclear to me how the changes in #17405 were expected to work, or if they were intended to be surfaced in the UI.
ewdurbin added a commit that referenced this pull request Feb 6, 2025
* resolve new errors introduced in #17405

changing from return value to exception was not caught due to stubs.

it's unclear to me how the changes in #17405 were expected to work, or if they were intended to be surfaced in the UI.

* translations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trusted Publisher error message is confusing when project exists but user doesn't have ownership
6 participants