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

Use explicit regex for accepted add-on types in AMO routes #10363

Closed
kumar303 opened this issue May 3, 2017 · 11 comments · Fixed by mozilla/addons-frontend#6761
Closed

Use explicit regex for accepted add-on types in AMO routes #10363

kumar303 opened this issue May 3, 2017 · 11 comments · Fixed by mozilla/addons-frontend#6761

Comments

@kumar303
Copy link
Contributor

kumar303 commented May 3, 2017

Once we get the new router (which will probably happen in #10295) then we can change all of these routes:

<Route path=":visibleAddonType/categories/" component={CategoryList} />

to a specific regex list of accepted types, like:

<Route path=":visibleAddonType(addon|theme|...)/categories/" component={CategoryList} />

This will prevent confusing 500 errors when you type in a URL like /en-US/firefox/users/register/.

@kumar303
Copy link
Contributor Author

kumar303 commented May 5, 2017

We can also remove error handling code in the component for 404s

@willdurand
Copy link
Member

This should be doable once mozilla/addons-frontend#5652 lands.

@seanprashad
Copy link

Hey guys 👋🏽I'd like to help with this if it's still available!

@willdurand
Copy link
Member

willdurand commented Oct 25, 2018

@seanprashad sure! This issue is about:

  • adding a regexp in the route's path of the category pages
  • removing the code that validates the visibleAddonType parameter (404, etc.)

@seanprashad
Copy link

Ok, I've made some progress and would like to make sure I'm not heading down the wrong 🐰hole @willdurand:

All of the <Route> components from React live in src/amo/components/Routes/index.js.

Three occurrences of visibleAddonType appear as path params:

  1. https://github.com/mozilla/addons-frontend/blob/43ee8c787128f14fabc87b58287f097bc6ecd405/src/amo/components/Routes/index.js#L92-L96

  2. https://github.com/mozilla/addons-frontend/blob/43ee8c787128f14fabc87b58287f097bc6ecd405/src/amo/components/Routes/index.js#L97-L101

  3. https://github.com/mozilla/addons-frontend/blob/43ee8c787128f14fabc87b58287f097bc6ecd405/src/amo/components/Routes/index.js#L144-L148

I figured that finding the list of acceptable addon types would be the next move. visibleAddonType turns out to be a function, which is found here:

https://github.com/mozilla/addons-frontend/blob/43ee8c787128f14fabc87b58287f097bc6ecd405/src/core/utils/index.js#L193-L205

Within the definition of visibleAddonType, I saw VISIBLE_ADDON_TYPES_MAPPING which tells me that it's being used as a lookup for a set of values:

https://github.com/mozilla/addons-frontend/blob/43ee8c787128f14fabc87b58287f097bc6ecd405/src/core/constants.js#L68-L75

Now we'll have to figure out the right regex that will conform to all 7 valid types. I'm not sure where to place the hardcoded expression and where we'll need to do something like .match().

Is there anything I missed along the way that might give me some clues?

@willdurand
Copy link
Member

I believe only "themes" or "extensions" are used:

export const API_ADDON_TYPES_MAPPING = {
  extensions: ADDON_TYPE_EXTENSION,
  themes: ADDON_TYPE_THEME,
};

@willdurand
Copy link
Member

You can add the values in the path prop of each route.

@seanprashad
Copy link

seanprashad commented Oct 27, 2018

Gotcha - for reference, the Route components path prop uses path-to-regexp.

@ioanarusiczki
Copy link

@willdurand Please add some STR for qa or mark it "qa not needed".
Thanks!

@willdurand
Copy link
Member

@ioanarusiczki sorry, please check that we can still navigate to the landing pages (extensions, themes) and category pages (including the android page with the list of all categories).

@ioanarusiczki
Copy link

Verified on AMO dev with FF63 - Win10 & Android 8.0
The detail pages for extensions/themes are displayed. I also checked that the list of categories is displayed and then verified each category on Windows and then on Android.

@KevinMind KevinMind transferred this issue from mozilla/addons-frontend May 5, 2024
@KevinMind KevinMind added repository:addons-frontend Issue relating to addons-frontend migration:2024 labels May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants