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

Add a referer header when new issues are created and redirected to issue page #1276

Closed
miketaylr opened this issue Jan 11, 2017 · 8 comments
Closed

Comments

@miketaylr
Copy link
Member

See #1273 (comment)

We can use that as a signal to show the "Thanks!" flash message, rather than the current (hacky, imo) solution of using session.

See https://github.com/webcompat/webcompat.com/blob/master/webcompat/views.py#L214 and all other instances of session['show_thanks'] = True

@nj4710
Copy link
Contributor

nj4710 commented Mar 27, 2017

Hi @miketaylr I would like to work on this

@miketaylr
Copy link
Member Author

Cool! We might want to get @karlcow to review these patches when they're ready.

nj4710 added a commit to nj4710/webcompat.com that referenced this issue Mar 28, 2017
@karlcow
Copy link
Member

karlcow commented Mar 29, 2017

I will remove the status: good next bug for now. As I think it requires a bit more knowledge. Let's rewind a bit and give a longer description of the issue.

Some background.

An HTTP POST is not idempotent and not safe. It means that two subsequent POST /issues/new do not create the same results. In our case each of them will create a new issue and create a new URI. So when we do a POST the server is replying to us and provide a number. We want to use this number to redirect the browser to the URL of the issue and at the same time show a flash message.

In #1273, @miketaylr implemented a way to

  1. redirect the browser to the issue which has just been created aka through POST /issues/new.
  2. And show the Thanks message.

He is doing the Thanks message by storing in session a keyword with the value True.

session['show_thanks'] = True
return redirect(url_for('show_issue', number=response.get('number')))

This is then used in @app.route('/issues/<int:number>') with def show_issue(number): where it is checked if session contains the right keyword to send the show_thanks and then removes it from the session. This function/route is used for showing issues. So we do not want to send the thanks message all the time. That's why there is the current session mechanism.

@app.route('/issues/<int:number>')
@cache_policy(private=True, uri_max_age=0, must_revalidate=True)
def show_issue(number):
'''Route to display a single issue.'''
if g.user:
get_user_info()
if session.get('show_thanks'):
flash(number, 'thanks')
session.pop('show_thanks')
return render_template('issue.html', number=number)

What I suggested in my comment is that instead of storing a session for this flash message. We just use the fact that we show the flask message only when we come from POST /issues/new. It means when accessing /issues/<int:number> we need to check the HTTP Referer on the request. If it comes from /issues/new then we send a show thanks message. So instead of a if session.get()… we would use a if request.referer

Things to do:

  • Create tests to make sure that the referer is here when we transition from POST /issues/new to GET /issues/issue_number Probably in test_urls.py
  • Change the show_issue to use the referer
  • Clean the code elsewhere that we are sure we don't use the two ways of doing things
  • Make sure that we don't break the functional tests
  • Run nosetests

@nj4710
Copy link
Contributor

nj4710 commented Mar 29, 2017

That was a great explanation. Thank you so much.
Sorry for my last commit.
Can I at-least try and complete one of these tasks?
So, what I understand from this is we do not need to use set_referer or get_referer method in def create_issue(). We will need to remove the session variables from this function since we are not using them anymore.

Now, inside show_issue(number) we need to change the if condition, since we are checking the referer now. It will be if request.referrer == ('/issues/new') and then show the flask message.

Please correct me if I am wrong.
Also, am I eligible to work on this? Since my last PR is in review.
Thanks

@karlcow
Copy link
Member

karlcow commented Mar 29, 2017

@nj4710 You are always welcome to work on things. And if you can't finish for any reasons it is ok too. And we will help along the way or find a solution together. 🆒

nj4710 added a commit to nj4710/webcompat.com that referenced this issue Mar 30, 2017
@nj4710
Copy link
Contributor

nj4710 commented Mar 30, 2017

I tried to complete the show_issue task. r? @karlcow

@miketaylr
Copy link
Member Author

I tried to complete the show_issue task. r? @karlcow

(Our process is to create a PR and ask for review in that, rather than a comment in a bug. :)) It's also OK to open up a PR and ask for feedback as you're developing things, before things are 100% ready for review.

nj4710 added a commit to nj4710/webcompat.com that referenced this issue Apr 5, 2017
nj4710 added a commit to nj4710/webcompat.com that referenced this issue Apr 6, 2017
@karlcow
Copy link
Member

karlcow commented Apr 12, 2017

See my comments in @nj4710 Pull Request
#1466 (comment)

My initial idea was bad and that was discovered by exploring the pull request of @nj4710
For the record, @nj4710 did a good work.

I'm closing the issue as wontfix. Thanks.

@karlcow karlcow closed this as completed Apr 12, 2017
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

No branches or pull requests

3 participants