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

Visitor actions that redirect to Login should remember & execute action #10338

Open
mekarpeles opened this issue Jan 15, 2025 · 2 comments · May be fixed by #10365
Open

Visitor actions that redirect to Login should remember & execute action #10338

mekarpeles opened this issue Jan 15, 2025 · 2 comments · May be fixed by #10365
Assignees
Labels
Fellowship Opportunity Good First Issue Easy issue. Good for newcomers. [managed] Lead: @mekarpeles Issues overseen by Mek (Staff: Program Lead) [managed] Priority: 2 Important, as time permits. [managed] Type: Feature Request Issue describes a feature or enhancement we'd like to implement. [managed]

Comments

@mekarpeles
Copy link
Member

mekarpeles commented Jan 15, 2025

Proposal

Similar to:

Currently, clicking on Follow, Borrow, or Want to Read as a not logged in visitor will send patron to login without remembering or performing the action they performed.

Whereas #9409 attempts to remember a patron's originating request (Follow, Borrow, or Want to Read) throughout a complicated registration process using cookies, This issue proposes we more modestly extend the existing login flow capabilities to remember and complete a visitor's requested op (operation) after successful login and before we redirect them back to where they came from. We will also use our existing add_flash_message to add a temporary flash message to the screen indicating whether the action was completed or failed. The redirect flow is already complete.

Proposed Solution

Let's implement a generalized flow to extend the capabilities of the login flow to remember and complete a visitor's requested op (operation) in the cases of Follow, Borrow, or Want to Read.

Let's start with Follow as the requirement for completing this specific issue.

Justification

Problem:

Today when a visitor clicks a button like Follow, Borrow, or Want to Read, they are sensibly redirected to the login page /account/login?redirect=$(ctx.path) in a way that uses a redirect url GET query params to preserve the $ctx.path (i.e. the original page they came from) so the visitor can be redirected back after successful login...

However the action itself (i.e. Follow, Borrow, or Want to Read) the patron was trying to complete is not executed upon successful login.

Example

  • Update Trending Now cards to display patron (if reading log public) #10241
    By using the Follow macro to add Follow buttons to the /trending page results, a visitor that is not logged in might click the Follow button, be brought to login, and then brought back to the /trending page after being successfully logged in, but the actual Follow operation they requested is never actually performed! The visitor would have to click Follow, successfully perform login, be brought back to the /trending page and then know they have to click Follow again. This is both non-intuitive and either difficult or impossible because the /trending page by nature changes from moment to moment and the reader our patron was trying to follow may no longer be featured on the trending page when they return from login! 🤦

Impact

This likely impacts thousands of visitors who are either new and have a poor first experience, or who have accounts and thought they were logged in.

Breakdown

If/when a visitor clicks Follow and goes to the the login flow, the parameters necessary to complete the Follow request would somehow be captured and passed along within the login url/flow (e.g. the Follow macro url here https://github.com/internetarchive/openlibrary/pull/8607/files#diff-f8a08ee2ccd19673120d20e7bb9a9bb0522f49eed20aa98edd0007787058c302R17 might be updated to include additional parameters like /account/login?redir_url=$(ctx.path)&op=follow,publisher:$publisher,&state:$following) so after login is successful, the follow command would get executed on the backend before the logged in patron is redirected to the redir_url (i.e. the page they came from).

Here is the login handler (the GET and POST) where the login flow happens:
https://github.com/internetarchive/openlibrary/blob/master/openlibrary/plugins/upstream/account.py#L399-L476

We might update https://github.com/internetarchive/openlibrary/blob/master/openlibrary/plugins/upstream/account.py#L427 onward within the GET code to add:

# this extracts or sets defaults for GET query `?params=` in the url
i = web.input(redirect=referer, op=None)  # add `op`
op = {}
f = forms.Login()
f['redirect'].value = i.redirect
return render.login(f, op=i.op)

We can then update the login template to get passed access to this op object (to be passed along with the POST request)

https://github.com/internetarchive/openlibrary/blob/master/openlibrary/templates/login.html#L1

$def with (form, op=None)

And we add another hidden parameter like redirect
https://github.com/internetarchive/openlibrary/blob/master/openlibrary/templates/login.html#L72 called "op" to the login template that will pass along the values in our op to the POST controller in the backend:

<input type="hidden" id="op" value="$op" name="redirect" />

Finally, we extend our login POST handler https://github.com/internetarchive/openlibrary/blob/master/openlibrary/plugins/upstream/account.py#L476 to safely perform the op.

stats.increment('ol.account.xauth.login') # after this line...
if i.op:
    """
    assuming url is: "?op=follow,publisher:xxx,state=yyy"
    i.op should be: "follow,publisher:xxx,state=yyy"
    """
    # Split and extract/unpack op, such that:
    # op="follow", its args=["publisher:xxx", "state:yyy"]
    op, *args = i.op.split(",")
    # Converts args to dict {'publisher': 'xxx', 'state': 'yyy'}
    params = {k: v for kv in args for k, v in [kv.split(":")]}
    if op == "follow":        
        new_perform_function(op, **params) # exercise left to reader! :)
raise web.seeother(i.redirect)

At which point you'll have to think about the best way to implement this new perform function which, based on the value of op may need to do different things, check and safely sanitize the parameters, and add a toast message with success or failure of the action

The remaining unknowns here for contributors are:

  1. How do I perform the follow action?
  1. How do I do the flash message?

Instructions for Contributors

Please run these commands to ensure your repository is up to date before creating a new branch to work on this issue and each time after pushing code to Github, because the pre-commit bot may add commits to your PRs upstream.

@mekarpeles mekarpeles added Type: Feature Request Issue describes a feature or enhancement we'd like to implement. [managed] Good First Issue Easy issue. Good for newcomers. [managed] Priority: 2 Important, as time permits. [managed] Lead: @mekarpeles Issues overseen by Mek (Staff: Program Lead) [managed] Fellowship Opportunity labels Jan 15, 2025
@ShilpThapak
Copy link

Hi Mek,
I’d like to work on this issue. Could you please assign it to me?

@mekarpeles
Copy link
Member Author

Hi @ShilpThapak, sure we'd love your help. Please note that this issue is an important companion to:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fellowship Opportunity Good First Issue Easy issue. Good for newcomers. [managed] Lead: @mekarpeles Issues overseen by Mek (Staff: Program Lead) [managed] Priority: 2 Important, as time permits. [managed] Type: Feature Request Issue describes a feature or enhancement we'd like to implement. [managed]
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants