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

Strip # from referer url which which is malformed #2267

Closed
wants to merge 1 commit into from

Conversation

dan1d
Copy link
Contributor

@dan1d dan1d commented May 5, 2018

What? Why?

Closes #2251 as it correctly parses the url sent on the referer header, the error on #2251 comes from the url having 2 different "# #" in the url:
https://alpha.katuma.org/shops#/?query=grup%20de%20consum%20la%20panarra&show_closed=1#grup-de-consum-la-panarra

It also caches the parsed uri on an instance variable so it could be used by another method that is also parsing the url.

What should we test?

Request referer #2251
curl --request GET
--header 'X-Csrf-Token: OLie+WlEahxpXGW5i+yTRNgVR+ONVsZkgE/dzFHNNXQ='
--header 'Accept-Encoding: gzip, deflate'
--header 'Cookie: [FILTERED]'
--header 'Accept: application/json, text/javascript, /'
--header 'User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/601.7.8 (KHTML, like Gecko) Version/9.1.3 Safari/537.86.7'
--header 'Referer: https://alpha.katuma.org/shops#/?query=grup%20de%20consum%20la%20panarra&show_closed=1#grup-de-consum-la-panarra'
--header 'Accept-Language: ca-es'
--header 'X-Requested-With: XMLHttpRequest'
'https://alpha.katuma.org/enterprises/12/relatives.json'

@enricostano
Copy link
Contributor

thanks @dan1d ! Do we have any clue on why the URI is malformed?

cc/ @sauloperez


def parsed_referer_uri
return @parsed_referer_uri if @parsed_referer_uri
referer_url = request.referer.gsub("#", '')
Copy link
Contributor

Choose a reason for hiding this comment

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

watch out, Rubocop (in Code Climate) is asking you to use #delete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I should have run rubocop before commiting. Should I make a new commit with the changes required by rubocop ?
I will start doing that on others PR's.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's up to you but normally for these single-commit PRs I just amend the commit where I added the change and I force the git push.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I should have run rubocop before commiting.

The earlier you get feedback from Rubocop the better. That's why I strongly recommend to integrate it with your editor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll integrate it with Visual Code, I've switched to it recently 👍
I've force pushed the changes with the single commit.

@dan1d
Copy link
Contributor Author

dan1d commented May 6, 2018

Hi @enricostano, I don't have a clue where this URL comes from, I'm new to the project but I'm assuming that the first "#" is the angular one and the last "#" is used as fragment identifier maybe ?, I'll dig a bit more but I don't really know If I will find the answer.

@sauloperez
Copy link
Contributor

sauloperez commented May 6, 2018

Thanks a lot for your help @dan1d ! keep it up 💪 ! Just out of curosity, how did you find about OFN?

	- Cache url parsing call as it is used by other methods.
@dan1d
Copy link
Contributor Author

dan1d commented May 6, 2018

@sauloperez I was looking around on https://www.codetriage.com and this repository showed up on the third page I think, saw the project name, read the description and I liked the project, then I saw the long list of issues, so I'm here triying to help and contribute.

@myriamboure
Copy link
Contributor

Welcome @dan1d ! We really appreciate your contribution, as you see we still have a lot to do but we are clarifying processes an cleaning up old issues step by step :-) We just sent you an invitation to our Slack, if you want to get more involved in the OFN project there are opportunities! Let's talk there and meet with the team :-)

@daniellemoorhead
Copy link
Contributor

Hi @dan1d 😄
Wondering if you've had a chance to look at @sauloperez's comments and make some changes so we can keep moving this through the pipe to merged?

@dan1d
Copy link
Contributor Author

dan1d commented May 18, 2018

Hi @daniellemoorhead I didn't have a chance yet, I'll figure it out this weekend which I should be free 👍
I've added the changes requested by @sauloperez but I still need to figure out the first: https://alpha.katuma.org/shops# # which caused this issue.

@daniellemoorhead
Copy link
Contributor

daniellemoorhead commented Jun 1, 2018

Hiya @dan1d...just checking in...

nag

but I still need to figure out the first: https://alpha.katuma.org/shops# # which caused this issue.

How's it going solving this ☝️

#tidyingupthedeliverytrain

@daniellemoorhead
Copy link
Contributor

Closing this for now and moving the issue back into dev ready. @dan1d if you get a chance to come back and finish this then go right ahead and reopen the PR. Cheers 😄

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.

URI::InvalidURIError in enterprises#relatives
5 participants