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

Storing Bookmarks ... or not #883

Closed
awead opened this issue Sep 14, 2021 · 4 comments · Fixed by #1189
Closed

Storing Bookmarks ... or not #883

awead opened this issue Sep 14, 2021 · 4 comments · Fixed by #1189
Assignees

Comments

@awead
Copy link
Contributor

awead commented Sep 14, 2021

Presently, bookmarks are stored in the database, linked to a user accounts. User accounts are only created per session. So as long as the user's session remains active, there is a user account in the database with that session ID, and the associated bookmarks are linked to that specific session user. Once the session is lost, when the catalog page is reload or visited again, the application cannot retrieve the original user session record, and the bookmarks are lost.

Technically, neither user nor bookmarks are "lost" because they're still in the database, we just can't retrieve them anymore because we don't have the original session ID that was used to create and link them.

Current Issues

  • the user and bookmark records are never cleaned out, so these records accumulate and are never removed
  • bookmarks are only active as long as the user's session is
    • presently, the user's session is stored as a client-side cookie
  • it's not clear to the user that bookmarks are ephemeral

Proposals

Here are some solutions in ascending order of complexity.

Option 1: Remove Bookmarks

One idea is to remove bookmarks completely. Because the urls for searches and individual records are their own permalinks, the bookmarking feature could be replaced with the bookmarks that your browser keeps.

  • helpful UI messaging
  • JS integration? i.e., clicking "bookmark" triggers the bookmarking action in your browser

Pros: super easy, simplifies things a lot (no more user records!)
Cons: no bookmarking feature

Option 2: Alternate bookmarking storage

We can store the bookmarks in Redis, like we do with searches, and give them an appropriate TTL value, ex. 30 days. User records can probably go away as well, since this will all be driven by the session. It's still up to the user to maintain their session in the browser. If that session outlives the Redis TTL, then their bookmarks would disappear. Otherwise, if the user looses their session or starts a new one, the bookmarks will not follow and be "unmoored" in the Redis store to be culled when the TTL expires.

Pros: duplicates searches solution, gets rids of users (?)
Cons: bookmarks still disappear, moderate complexity

Option 3: OAuth integration

We integrate with OAuth and create user accounts for those that want to login. The user's account is permanent, and bookmarks can be retrieved regardless of session status. Guest accounts would need to be periodically truncated.

Pros: it's got it all
Cons: complicated. We need to manage users (not hard, but just more things to do)

Related Issues

Implement Blacklight Bookmarks - My Account Permanence #331
User Management #878

@banukutlu banukutlu added this to the 1.2.x milestone Sep 15, 2021
@whereismyjetpack
Copy link
Contributor

whereismyjetpack commented Sep 17, 2021

I'm leaning towards option 2.5. we don't need to hook in oauth to get database backed user records. we could use the work in the userlogin branch and use information from the headers to fill in the user record. The changes we'd need to make for bookmarks would be to hide the bookmark button if you're not logged in, or direct it to the /login route if you're not logged in, and yank the devise-guests plugin.

As attractive as ripping out the models seems, We might be causing more problems than it's worth, for example in the future we want to use some-cool-gem and it makes assumptions about activerecord..

@awead
Copy link
Contributor Author

awead commented Sep 17, 2021

So this still gives us user records? I don't see the need for them until we've successfully answered the reason for bookmarks, which I don't think we have yet. Option 2 essentially preserves the way it's working now. If that's good enough, then I don't think we need to do anymore.

If they need to be persisted beyond the session, then hook into OAuth. I'd rather do it like we do it elsewhere, i.e. Scholarsphere, because there's less cognitive overload. If we implement the same integration slightly differently in each application, it confusing quickly. But, we can get rid of guest accounts and only support bookmark retention for PSU users.

I also think we need an explicit "Login" otherwise we're creating account-like objects for them without their knowing, which seems duplicitous to me.

@whereismyjetpack
Copy link
Contributor

IMO oauth in the app is way more complicated, and was needed in scholarsphere because of the groups integration. If i had to do it again, i'd probably do some combination of oauth-proxy, and a smaller service in the app that just pulls out groups, but I also don't want to change something that's working.. which is why I used catalog as a way to try something different.

@awead
Copy link
Contributor Author

awead commented Sep 17, 2021

Ok, then I would use this a means to refactor the scholarsphere implementation.

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 a pull request may close this issue.

3 participants