Skip to content

[MM-56792] Clarify large plugin upload error#26271

Merged
hanzei merged 13 commits intomattermost:masterfrom
daveseo901:master
Sep 9, 2024
Merged

[MM-56792] Clarify large plugin upload error#26271
hanzei merged 13 commits intomattermost:masterfrom
daveseo901:master

Conversation

@daveseo901
Copy link
Contributor

Summary

Changes uploadPlugin() in plugin.go to more descriptively handle the error case where an upload plugin exceeds the maximum file upload size prescribed by the server (by default, this maximum is 100 MB). Now, the server will respond with a 413 HTTP status code and an api.plugin.upload.file_too_large.app_error error ID.

Also changes en.json to include an error message, "Uploaded plugin size exceeds limit." with the error ID mentioned above.

Adds tests for this error handling in plugin_test.go. Changes resources.go to symlink plugin_api_tests to facilitate plugin tests. Maybe in the future we should move plugin_api_tests to the standard tests directory?

Ticket Link

Fixes #26245
Jira https://mattermost.atlassian.net/browse/MM-56792

Release Note

Changed server-side logic to return a 413: Request Entity Too Large HTTP status code for a plugin upload that is too large.

Added an api.plugin.upload.file_too_large.app_error error ID.

Added a more descriptive error message, "Uploaded plugin size exceeds limit." for plugin uploads that are too large.

Added plugin_api_tests to the list of symlinked test resources.

@mm-cloud-bot mm-cloud-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Feb 20, 2024
@mattermost-build
Copy link
Contributor

Hello @daveseo901,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@larkox larkox added 2: Dev Review Requires review by a developer 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review labels Feb 22, 2024
@mattermost-build
Copy link
Contributor

E2E tests not automatically triggered, because the PR is not in a mergeable state. Please update the branch with the base branch and resolve outstanding conflicts.

Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

Thanks @daveseo901! LGTM 👍

@mickmister mickmister removed the 2: Dev Review Requires review by a developer label Feb 28, 2024
@hanzei
Copy link
Contributor

hanzei commented Feb 28, 2024

/update-branch

@hanzei hanzei added the 2: Dev Review Requires review by a developer label Feb 28, 2024
@hanzei hanzei removed the request for review from DHaussermann February 29, 2024 13:18
@mattermost-build
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

@hanzei
Copy link
Contributor

hanzei commented Mar 11, 2024

Hey @daveseo901,

What do you think about my comments above? #26271 (review)

@daveseo901
Copy link
Contributor Author

Hi @hanzei ,

I added some comments and a question above!

@mattermost-build
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

@hanzei
Copy link
Contributor

hanzei commented Apr 8, 2024

@daveseo901 Are you still interested in wrapping up the final comments on this PR? I would love to merge it soon.

@daveseo901
Copy link
Contributor Author

@daveseo901 Are you still interested in wrapping up the final comments on this PR? I would love to merge it soon.

@hanzei Yes, I will submit the changes later today.

@hanzei hanzei self-assigned this Aug 15, 2024
@hanzei hanzei removed the Awaiting Submitter Action Blocked on the author label Aug 29, 2024
@hanzei
Copy link
Contributor

hanzei commented Aug 29, 2024

I really want to merge this PR, so I updated the error message. It no longer include the SiteURL, but instead explains where to find the settings:

Screenshot from 2024-08-29 14-06-57

@hanzei hanzei requested a review from larkox August 29, 2024 12:11
@hanzei
Copy link
Contributor

hanzei commented Aug 29, 2024

@larkox Please take another look

@hanzei hanzei added the 2: Dev Review Requires review by a developer label Aug 29, 2024
@hanzei hanzei removed the 2: Dev Review Requires review by a developer label Aug 30, 2024
@hanzei hanzei requested a review from DHaussermann August 30, 2024 13:07
@hanzei
Copy link
Contributor

hanzei commented Aug 30, 2024

/update-branch

Copy link
Contributor

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested and passed

  • Confirmed 2 new UI changes
    image
    image
  • Cryptic URL has been removed
    LGTM!

Huge thanks for your help on this @hanzei

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review labels Sep 6, 2024
@hanzei
Copy link
Contributor

hanzei commented Sep 9, 2024

/update-branch

@hanzei hanzei added this to the v10.1.0 milestone Sep 9, 2024
@hanzei hanzei merged commit 388dfdf into mattermost:master Sep 9, 2024
@hanzei hanzei removed their assignment Sep 9, 2024
@amyblais amyblais added Docs/Needed Requires documentation Changelog/Done Required changelog entry has been written labels Sep 9, 2024
NadavTasher pushed a commit to NadavTasher/mattermost that referenced this pull request Sep 25, 2024
Co-authored-by: Ben Schumacher <ben.schumacher@mattermost.com>
@cwarnermm cwarnermm added Docs/Not Needed Does not require documentation and removed Docs/Needed Requires documentation labels Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4: Reviews Complete All reviewers have approved the pull request Changelog/Done Required changelog entry has been written Contributor Docs/Not Needed Does not require documentation release-note Denotes a PR that will be considered when it comes time to generate release notes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clarify error message in System Console when uploading too big plugin bundle

10 participants