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

Change routes from packages/<action> to packages/manage/<action> to avoid conflicts with package ids #3131

Merged
merged 1 commit into from
Jul 27, 2016

Conversation

scottbommarito
Copy link
Contributor

@scottbommarito scottbommarito commented Jul 14, 2016

Currently, if someone creates a package with an id of "upload", "upload-progress", "verify-upload", or "cancel-upload", they can potentially run into conflicts with our routing scheme. In other words, a package with an id of "upload" would be impossible to view through packages/<id>, because MVC would route it to our upload package path route, packages/upload.

To avoid conflicts with our system, this PR changes those routes to packages/manage/upload and so on. Since packages/<id> and packages/manage/<action> are now completely distinct routes, users can upload a package with an id equal to any of our actions and view it without any issues. Note that a package id of "manage" is also still supported.

Will be making some additional PRs in different repos to update the references to the old route URLs.

@maartenba @skofman1 @xavierdecoster

(Fixes #3130)

@dnfclas
Copy link

dnfclas commented Jul 14, 2016

Hi @scottbommarito, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@maartenba
Copy link
Contributor

Quick Q: what if my package is called "manage"?

@scottbommarito
Copy link
Contributor Author

scottbommarito commented Jul 14, 2016

@maartenba
Still works! packages/manage does not conflict with packages/manage/<action> so MVC routes it to the package display page. We do have a packages/<id>/<action> route, but all of the actions work as they do for every package, even with "manage".

@maartenba
Copy link
Contributor

Change looks good, did you verify all views as well? (I saw a couple but not 100% sure those are all, perhaps good to make another pass for the paranoid)

@scottbommarito
Copy link
Contributor Author

Still learning MVC so not entirely sure what you mean by "all views" in this context, but I do know everything in the Views/Packages folder is still reachable and functional.

@scottbommarito
Copy link
Contributor Author

@maartenba @xavierdecoster @skofman1 @shishirx34 @ryuyu
Looking for some more sign-offs so we can merge this and it doesn't become stale.

@shishirx34
Copy link
Contributor

Looks Good!

@scottbommarito
Copy link
Contributor Author

@maartenba Can I merge this? You never responded to my response to your question so I wasn't sure whether or not to count it as a "ship-it" haha.

@maartenba
Copy link
Contributor

Good from my side.

@scottbommarito scottbommarito merged commit 935d746 into dev Jul 27, 2016
@scottbommarito scottbommarito deleted the scottbom/packagemanagerouting branch July 27, 2016 17:23
@maartenba maartenba mentioned this pull request Jul 29, 2016
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants