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

WIP: Perform authentication on the store level #464

Closed

Conversation

jcgurango
Copy link
Collaborator

@jcgurango jcgurango commented Jul 31, 2023

Creating this PR to track progress on a system for addressing #117, I thought I could put it out in small chunks but it's difficult to do so because it's kinda weird if some parts of the app use accounts one way and different parts use it a different way.

Goal here is to authenticate on the post store/comment store level so that when you open further screens, it doesn't just grab the default account (i.e. you select an account at the home tab, then when you open posts that'll be the account used for commenting, viewing, etc.). Default account would still be used for other things for e.x. if an external link opens the app.

@mykdavies
Copy link
Contributor

mykdavies commented Aug 1, 2023

That is quite a chunk of work, and more than I can take in easily tbh, so I will leave it to sharper minds to review in depth :-). But this is really important work, and will really help us address some of the opaqueness of our current handling of multiple accounts.

Can I just ask a couple of questions?

  • is it possible to push some of the logic into the accounts store (or similar) by creating a "this is the user's current chosen account/instance" record and related actions and computed values, so we're not creating additional ties between widgets, and so that the conditional code in the widgets becomes simpler?

  • it may be that you considered that approach (and others) but rejected them for good reasons. Could you document some of that thinking in a suitable place (in the code I guess to avoid it getting overlooked) so that future us will all remain aware of the issues with re-writing or working around this approach?

  • (edit, forgot my third point) -- I love the use of the initicon to generate a default icon for accounts without an avatar. That's something that we can certainly make use of elsewhere.

Thanks!

@jcgurango jcgurango changed the title WIP: Pass accounts up and down the widget tree/stack WIP: Perform authentication on the store level Aug 2, 2023
@jcgurango jcgurango closed this Aug 2, 2023
@jcgurango jcgurango reopened this Aug 2, 2023
@jcgurango
Copy link
Collaborator Author

jcgurango commented Aug 2, 2023

@mykdavies I've updated the title, I was inaccurate in the way I was describing the system but hey that's why it's WIP 😂

What it actually will do is perform the authentication on the stores (i.e. poststore, commentstore, etc.) which made sense to me because these all accept credentials on the function level (i.e. you pass the credentials to, say, the refresh() function but if you're using the store for a single widget it's always going to be the same credentials anyway). So the post store becomes a store of a post viewed by a specific account, or no account, rather than ambiguously just a post but you can pass different credentials to the functions to get different results.

The reason I didn't want the account selection at the top level AccountStore is because it'd get somewhat confusing when you do something like comment on a post and then decide you want to switch account while commenting (like say you realized it's the wrong account). That account switch would propagate to all screens, so the next time you refresh the post it'd be a different set of credentials passed. I'm also trying to think of ways to make it more obvious what account you're using to view a post/community if anyone's got suggestions for that.

@mykdavies
Copy link
Contributor

Thanks for clearing up my confusion there :-)

That account switch would propagate to all screens, so the next time you refresh the post it'd be a different set of credentials passed.

Oh, that's interesting, I was thinking that a user would want the change to stick, but I can see why this approach also makes sense. Perhaps when we support mod actions that will be important. I guess we'll find out more about how people actually use this once we get some actual usage feedback.

I'm also trying to think of ways to make it more obvious what account you're using to view a post/community if anyone's got suggestions for that.

I guess the important thing is that it's clear when they take an obvious action such as posting or replying, where having it on those screen would help. But then perhaps there are other actions such as blocking, reporting and even voting where someone might want to quickly check which account is active. One way to give reasonably easy confirmation would be adding the current account/instance to the top of the "..." menu with an option to change it from there, but I think a full re-think of the UI is needed at some point...

@mykdavies
Copy link
Contributor

I just added a pull request with an outline of something prompted by this discussion: it adds a user icon to the AppBar that shows a user their chosen account and allows them to change this. It's only a prototype at the moment as I'm still working out how it would actually work, but I thought it worth putting out there for people to have a think about, so comments are welcome: #471

@jcgurango
Copy link
Collaborator Author

So I've been chipping away at this, but damn if there isn't a ton of interconnected parts. I made a map which shows progress and might be handy in the future:

image

@jcgurango
Copy link
Collaborator Author

Okay, this one really got away from me and I kept going in the same direction when the more I added to this system the more it became clear it's not the right way to go. Too much to change, too much added complexity. I'm closing this one in favor of #510

@jcgurango jcgurango closed this Sep 19, 2023
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 this pull request may close these issues.

2 participants