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

Do not automatically subscribe admin OR users to new federated magazine lookups #237

Merged
3 commits merged into from
Nov 10, 2023

Conversation

ghost
Copy link

@ghost ghost commented Nov 10, 2023

Disables the default /kbin behavior of automatically subscribing the admin to federated magazine lookups. The searching user must explicitly hit the subscribe button in order for the Mbin instance to pull in federated updates.

This change would also seem to remove the need for the admin panel option to explicitly allow only logged in users to search for federated content since you have to be logged in to subscribe anyway, but I left it intact incase we want to modify this behavior in the future.

@ghost ghost added enhancement New feature or request backend Backend related issues and pull requests labels Nov 10, 2023
@ghost ghost changed the title Do not automatically subscribe admin OR users to new federated magazines lookups Do not automatically subscribe admin OR users to new federated magazine lookups Nov 10, 2023
@e-five256
Copy link
Member

e-five256 commented Nov 10, 2023

Super curious how this flow looks now (especially with the described async annoyance in that:) user searches for new remote mag, gets no results because the process is async, searches again, and... does it show up and load that time? but doesn't subscribe them, so they have to click into that mag and subscribe

At the point before any subscribers, the magazine exists as a new entry in magazine, but without any subscribers it won't get activity events?

Edit: we walked through the flow together in matrix and all I can say is the UX continues to remain confusing as heck, which it always has been even before this PR 😆 but everything seemed fine, I think the risk is low, as debounced has already lived with the flow of manually unsubscribing so we know how that acts, there are more improvements to come in benti's remote mod rework

@BentiGorlich
Copy link
Member

Does this work when you are creating a new magazine? I did try it in my PR, but reverted it, because newly created local magazines would always throw a 500 because the owner was missing

@ghost
Copy link
Author

ghost commented Nov 10, 2023

I added an condition where local mag creations still get auto subscribed to the user, now this will only impact federated magazines. :-)

@BentiGorlich BentiGorlich self-requested a review November 10, 2023 16:12
@ghost ghost merged commit 024e502 into main Nov 10, 2023
7 checks passed
@ghost ghost deleted the mag_no_auto_subscribe branch November 10, 2023 16:14
@melroy89
Copy link
Member

Thank god.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Backend related issues and pull requests enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants