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

Allow adding URL parameters in via content_for_template() #579

Open
yuriescl opened this issue Dec 29, 2021 · 10 comments
Open

Allow adding URL parameters in via content_for_template() #579

yuriescl opened this issue Dec 29, 2021 · 10 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@yuriescl
Copy link
Contributor

I couldn't find a way to add query params to the URL during the SEP-24 interactive flow.
form_for_transaction and content_for_template just allow returning the form and template content, but not set additional URL parameters.
It's difficult to add previous/next buttons to the form without setting some URL parameter to identify which part of the flow the user is at the moment.
I'd like to have something like:
sep24/transactions/withdraw/webapp?currentStep=1&transaction_id=...

currentStep would indicate which form to return.

As a workaround, Polaris could, for example, get those extra URL parameters from content_for_template, similar to how the template name is set using template_name.

@JakeUrban
Copy link
Contributor

JakeUrban commented Jan 4, 2022

Hi @yuriescl,

Why does currentStep need to be stored in the URL instead of in the users's session cookie or the database?

For example, I could add two forms to my base template, one for "Previous" and one for "Next" and they would POST to different endpoints:

<!-- form content for the current step here (rendered by Django) --!>

<form method="post" action="/transactions/deposit/webapp/next">
    <input type="submit" value="Previous">
</form>

<form method="post" action="/transactions/deposit/webapp/previous">
    <input type="submit" value="Next">
</form>

Then /next and /previous endpoints would increment or decrement request.session['current_step'] and redirect to /transactions/deposit/webapp so Polaris can call my implementation of form_for_transaction(), which would return a form dependent on the value of request.session['current_step'].

@yuriescl
Copy link
Contributor Author

yuriescl commented Jan 4, 2022

That's a great point.
One of the challenges I faced was to implement a multiple step interactive flow in SEP-24, in which the user could press the back button, change something, then go forward again, etc. One of my ideas to control the flow was to add a step variable in the URL, but your idea would also work. The only limitation I can imagine with that is if the user disabled cookies for some reason, but since the transaction id is in the URL, it can be used instead.
Thanks!

@yuriescl yuriescl closed this as completed Jan 4, 2022
@yuriescl
Copy link
Contributor Author

yuriescl commented Jan 7, 2022

@JakeUrban I think there might be an issue with the web browser back/forward buttons when using the session to store the step.
If the user presses the browser back button and it goes to the same URL, how would I know he went back a step?

@yuriescl yuriescl reopened this Jan 7, 2022
@JakeUrban
Copy link
Contributor

JakeUrban commented Jan 7, 2022

Hi @yuriescl, you're right, you'd need the GET request to contain an indication that the user went back in that case.

We can add a key, url_args, to the dictionary returned from content_for_template(). The value of the key should be a dictionary that can be passed to urllib.parse.urlencode(). Polaris will append the result of that call to the URL Polaris redirects to after processing each POST request.

        if next_form or next_content:
            args = {
                "transaction_id": transaction.id,
                "asset_code": asset.code,
                **content_from_anchor.get("url_args", {})
            }
            if amount:
                args["amount"] = amount
            if callback:
                args["callback"] = callback
            url = reverse("get_interactive_deposit")
            return redirect(f"{url}?{urlencode(args)}")

The limitation with this approach is that we won't be able to add URLs to the initial URL returned from the POST /transactions/deposit/interactive request, since Polaris does not call content_for_template() then. That shouldn't be a problem since the first form is always step 1 anyway right?

@JakeUrban
Copy link
Contributor

JakeUrban commented Jan 7, 2022

Actually, there would be a way for you to change the initial URL. Polaris provides a DepositIntegration.interactive_url() function that is called on every GET request to the interactive flow.

    try:
        url = rdi.interactive_url(
            request=request,
            transaction=transaction,
            asset=asset,
            amount=amount,
            callback=callback,
        )
    except NotImplementedError:
        pass
    else:
        # The anchor uses a standalone interactive flow
        return redirect(url)

You could redirect to request.build_absolute_uri() + urlencode({"currentStep": current_step - 1}).

@JakeUrban JakeUrban self-assigned this Jan 12, 2022
@JakeUrban JakeUrban added the enhancement New feature or request label Jan 12, 2022
@yuriescl
Copy link
Contributor Author

You could redirect to request.build_absolute_uri() + urlencode({"currentStep": current_step - 1}).

This creates an infinite redirect loop

@yuriescl
Copy link
Contributor Author

yuriescl commented Jan 13, 2022

This worked for me:

def interactive_url(
    self,
    request: Request,
    transaction: Transaction,
    asset: Asset,
    amount: Optional[Decimal],
    callback: Optional[str],
    *args: List,
    **kwargs: Dict,
) -> Optional[str]:
    if request.query_params.get("step"):
        raise NotImplementedError()

    step = session["step"]
    url = request.build_absolute_uri()
    parsed_url = urlparse(url)
    url += "&" if parsed_url.query else "?"
    return url + urlencode({"step": step})

@yuriescl
Copy link
Contributor Author

It should be easier to control the URL params, though.

@JakeUrban JakeUrban reopened this Jan 13, 2022
@JakeUrban
Copy link
Contributor

@yuriescl I'll keep this issue open for adding that functionality, glad to hear you have a temporary workaround though

@JakeUrban JakeUrban changed the title How to add URL parameters in SEP-24 Allow adding URL parameters in via content_for_template() Jan 13, 2022
@yuriescl
Copy link
Contributor Author

After a few more tests, I see there's an issue when the form is invalid. Polaris won't add my step parameter to the post URL, and what ends up happening is the browser goes back to the same page before going back to the previous page (because the URL is different - missing the step parameter). So I have to press the Back button twice to go back.

Allowing to add params via content_for_template seems to be the only viable workaround atm.

@JakeUrban JakeUrban added this to the 3.0 milestone Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants