WikiEducationFoundation / WikiEduDashboard

Wiki Education Foundation's Wikipedia course dashboard system
https://dashboard.wikiedu.org
MIT License
392 stars 630 forks source link

Cannot directly load student view for username that include a period #4873

Closed ragesoss closed 2 years ago

ragesoss commented 2 years ago

The server returns a 404 for this URL: https://dashboard.wikiedu.org/courses/University_of_Ottawa/CMN2160C_(Winter)/students/articles/Heba.Aweiwi

Other usernames in the same course work fine: https://dashboard.wikiedu.org/courses/University_of_Ottawa/CMN2160C_(Winter)/students/articles/Nabila%20Bashamuher

Navigating to that user from an already-loaded course page (such as the second link) also work fine, so the problem is likely with the Rails route.

enelesmai commented 2 years ago

Hello @ragesoss I would like to work on this issue, I will start the research

ragesoss commented 2 years ago

@enelesmai how is it going? Give me a ping if you need any help.

buildwithallan commented 2 years ago

Hi @ragesoss I will like to work on this if it's still available.

buildwithallan commented 2 years ago

Hi @ragesoss I am having a tough time figuring this out. Any clue?

ragesoss commented 2 years ago

@buildwithallan sure. can you summarize what you've researched and what ideas you have so far?

buildwithallan commented 2 years ago

@ragesoss Okay. I read that if you want to support a url parameter more complex than an id number, you may run into trouble with the parser if the value contains a period. Anything following a period will be assumed to be a format(i.e. json)

If you want to support a url parameter more complex than an id number, you may run into trouble with the parser if the value contains a period. Anything following a period will be assumed to be a format (i.e. json, xml). For it to work, you will have to add a constraint to broaden the accepted input.

Example: resources :users, constraints: { id: /.*/ }

ragesoss commented 2 years ago

Good, it sounds like you are on the right track. A good next step would be to write some tests for the examples in this issue. Doing that may help you pin down which route lacks the constraints it needs to work in this situation.

ragesoss commented 2 years ago

You can see that we already use this constraints: { id: /.*/ } approach for many routes in routes.rb, so it might be easy to fix once you find the correct route.

buildwithallan commented 2 years ago

@ragesoss Sure I will do that.

zishugshan commented 2 years ago

initially :

get 'courses/:school/:titleterm(/:_subpage(/:_subsubpage(/:_subsubsubpage)))' => 'courses#show',
        :as => 'show',
        constraints: {
          school: /[^\/]*/,
          titleterm: /[^\/]*/
}

mysuggestion :

get 'courses/:school/:titleterm(/:_subpage(/:_subsubpage(/:_subsubsubpage)))' => 'courses#show',
        :as => 'show',
        constraints: {
          school: /[^\/]*/,
          titleterm: /[^\/]*/,
          _subsubsubpage: /.*/
        }

update :

+ _subsubsubpage: /.*/

➡️ @ragesoss is it make sense?

ragesoss commented 2 years ago

Something like that is what I was thinking, yes. Did you test it out?

buildwithallan commented 2 years ago

I am currently working on this issue @zishugshan please

zishugshan commented 2 years ago

👍 sure, you do this. @buildwithallan I am sorry, I thought you are not active for someday