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 post login action - follow #10365

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

ShilpThapak
Copy link

@ShilpThapak ShilpThapak commented Jan 19, 2025

Closes #10338

This PR is a feature

Technical

Currently, the website does not allow users to follow a publisher if they are not logged in. After logging in, the user must manually return to the publisher's profile to follow them.

With this PR, the follow operation will be handled by the backend.

Testing

To test this, go to any user profile and try to follow them while logged out. You will be redirected to the login page, and after logging in, the backend will automatically complete the follow operation for you and display a flash message.

Screenshot

Before:

before_ol.mp4

After:

after_no_follow.mp4

Already following:

after_ol.mp4

Stakeholders

@mekarpeles

@github-actions github-actions bot added the Priority: 2 Important, as time permits. [managed] label Jan 19, 2025
if not redirect_path or "/" not in redirect_path:
return None

publisher = redirect_path.split("/")[-1]
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we can reliably use the redirect_url to get the publisher and the state as the redirect page may be something like the book page (e.g. /books/OL14587328M -- i.e. the page to send the patron to after the follow is successful).

Copy link
Author

@ShilpThapak ShilpThapak Jan 19, 2025

Choose a reason for hiding this comment

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

I don't think we can reliably use the redirect_url to get the publisher and the state as the redirect page may be something like the book page (e.g. /books/OL14587328M -- i.e. the page to send the patron to after the follow is successful).

Thanks for your input, Mek. I agree with your design choice.
I'll update the PR to retrieve the publisher username from the parameters instead of the redirect URL.

But, could you please clarify what you mean by "state"? In this scenario, the state will always be "follow" since the user is logged out, and the button will always display "follow."

Copy link
Member

@mekarpeles mekarpeles Jan 24, 2025

Choose a reason for hiding this comment

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

But, could you please clarify what you mean by "state"? In this scenario, the state will always be "follow" since the user is logged out, and the button will always display "follow."

🤔 yes, good point @ShilpThapak! Maybe we can get away with not requiring an additional state parameter for exactly the reason you describe.

I wonder if maybe to keep things simple we want to try something like

action=follow:<username>? e.g. action=follow:mekBot

We wouldn't have to add any new fields and can easily split the action string on :.

Comment on lines 447 to 450
if PubSub.is_subscribed(subscriber=ol_account.username, publisher=publisher):
flash_message = "You are already following this user!"
else:
PubSub.subscribe(subscriber=ol_account.username, publisher=publisher)
Copy link
Member

@mekarpeles mekarpeles Jan 19, 2025

Choose a reason for hiding this comment

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

It doesn't matter so much if the patron is already following the patron, we can simplify this to

PubSub.subscribe(subscriber=ol_account.username, publisher=publisher) and then f"You are following {publisher}"

Copy link
Author

@ShilpThapak ShilpThapak Jan 19, 2025

Choose a reason for hiding this comment

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

It doesn't matter so much if the patron is already following the patron, we can simplify this to

PubSub.subscribe(subscriber=ol_account.username, publisher=publisher) and then f"You are following {publisher}"

Makes sense, Mek. Your approach will be better for performance since it will reduce the lookup operation. I'll change this to your code.

I just wanted to point out that the message will say "You are following username" and not "You are following display_name" because we are capturing the username in the parameters.

Copy link
Member

Choose a reason for hiding this comment

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

That's true -- we could fetch the display_name should we choose to from the web.ctx.site query engine (as is done throughout upstream/accounts and upstream/mybooks, I believe)

Copy link
Member

@mekarpeles mekarpeles left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution @ShilpThapak! The form and overall structure look good! Take a look at the original example in the issue as there may be some opportunities to simplify the code.

I think beyond (or with) action we need to somehow also capture the publisher and op for the follow case.

@mekarpeles mekarpeles added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Jan 19, 2025
@github-actions github-actions bot removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Jan 26, 2025
@ShilpThapak ShilpThapak force-pushed the 10338/feature/post_login_action branch from 694da07 to ed1a2e3 Compare January 26, 2025 16:56
@ShilpThapak
Copy link
Author

ShilpThapak commented Jan 26, 2025

@mekarpeles,
As per your suggestion, I have made some improvements:

  1. Added follow:publisher in the parameters to simplify the code and remove the dependency on the redirect URL.
  2. Included publisher's display name in the flash message.

Please take a look at the attached video.

pr_update.mp4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 2 Important, as time permits. [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Visitor actions that redirect to Login should remember & execute action
2 participants