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

First cut of showing federation staleness #436

Merged
merged 36 commits into from
Mar 18, 2024

Conversation

andrewmoise
Copy link
Contributor

First cut of a fix for #362: Add warning panels analogous to the federated magazine panel.

In addition to checking whether the way I did the added internal functionality was right, it seems worth reviewing whether we should be making the "This magazine is from a federated server" panel more subtle, now that we have this? Having the bright yellow panels only show up if there's an actual problem seems better UX wise.

@andrewmoise
Copy link
Contributor Author

Discussion in Matrix is that this should be redone a bit (in addition to fixing the CI); I'll update and repush.

@melroy89

This comment was marked as outdated.

@andrewmoise
Copy link
Contributor Author

Okay, I've adjusted it as discussed. We stop showing the existing "this is a federated magazine" warning, unless we're on the last page of results (i.e. there probably is more on the original server). And, we show a warning on non-subscribed magazines that they're not getting updates, including a display of how long it's been since they got any updates.

translations/messages.en.yaml Outdated Show resolved Hide resolved
templates/magazine/_federated_info.html.twig Outdated Show resolved Hide resolved
Show an alert__info panel for magazines which don't have any local
subscribers or for which we haven't attempted federation in over 24 hours.

Add magazine_has_local_subscribers() to MagazineExtensionRuntime to
support it.
Copy link
Member

@BentiGorlich BentiGorlich left a comment

Choose a reason for hiding this comment

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

Just the 2 little things I mentioned in the comments.

About the lastActive thing: the field is getting updated whenever a new Entry, EntryComment, Post or PostComment is received. So this field is probably getting updated when you post something to a remote magazine. I suggest that you add a field lastRemoteUpdate to Magazine and set it the same way and on the same place as lastActive is set, but with the condition, that the magazine has an apId, the post or whatever does as well and that the domain of the apDomain of the post is the same as the magazine. That way we know we receive updates from the host.

This involves writing a migration, so a bit more fiddly for you, sorry 😅

templates/magazine/_federated_info.html.twig Outdated Show resolved Hide resolved
translations/messages.en.yaml Outdated Show resolved Hide resolved
melroy89
melroy89 previously approved these changes Feb 12, 2024
@melroy89 melroy89 added the frontend Visual issues, improvements, bugs or other aspects relating mostly to the front end label Feb 15, 2024
BentiGorlich added a commit that referenced this pull request Feb 17, 2024
- magazines now have an additional property `last_origin_update` for when the last update came directly from the origin

this is acting as the base for #436
@melroy89
Copy link
Member

After PR #503 is merged, we will update this PR I suppose?

@andrewmoise
Copy link
Contributor Author

Yeah, that sounds good to me, I can take a look, it should be pretty much trivial if the new column is done for me, so I can do the update then if that sounds good.

@BentiGorlich BentiGorlich added this to the v1.4.0 milestone Feb 19, 2024
melroy89 and others added 6 commits February 20, 2024 19:21
Co-authored-by: BentiGorlich <benjamin_g@posteo.de>
moved service worker registration into main app bundle, and also
deferred the service worker registration a bit as recommended by
https://web.dev/articles/service-workers-registration#improving_the_boilerplate,
though our service worker doesn't seems to be doing anything much.

this removes 1 inline script block from base template
changed entries summary in AP representation to draw from title instead
from body, this way the title will be shown along with 'show more'
button in most of other fediverse software, which should feel more
intuitive and avoids repeating the body twice
@andrewmoise
Copy link
Contributor Author

Hm, how about this?:

We could only update last_origin_update on posts or entries, not on comments, and only when we have local subscribers to the magazine. That way, it'd get a continuous flow of updates for as long as we were in good communication with the origin server, and with local subscribers, but not otherwise.

It's a little not-ideal maybe because it wouldn't show anything if we aren't in communication with an origin server (and that's not an imaginary situation - IDK how mbin works but on Lemmy you can be "subscribed" to a community on a server that's defederated from you, and people on your server can interact with you, but it doesn't show any content from the origin. It'd be nice to show a warning for that situation). IDK, maybe keeping track of good communication per-remote-instance would be a good thing to track as an alternate way of triggering the same type of warning?

But in any case for this change, if changing last_origin_update semantics sounds ok I'm happy to make the change.

@BentiGorlich
Copy link
Member

Sorry didn't get a chance to answer, yet. I'd like to merge this after I've looked at it again and deal with the last_origin_update in another PR.

I think the biggest problem is, that we cannot know whether the update is received because someone is subscribed to the magazine or to a user posting to the magazine if both, the user and the magazine have the same home server. That problem is there, regardless of how we deal with it. We could of course only update last_origin_update when we get an announce activity from the magazine actor, that would definitely guarantee that we have contact with the magazine. Maybe thats the way to go...

@andrewmoise
Copy link
Contributor Author

Ooh. Yeah, that makes perfect sense to me. I'm not super familiar with the federation details, but from what you say, updating last_origin_update when we get announces from the magazine actor sounds perfect.

Okay, just LMK if there's anything you want me to do with it.

Copy link
Member

@BentiGorlich BentiGorlich left a comment

Choose a reason for hiding this comment

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

When that is done, we can finally merge this PR. Sorry for taking so long to review it

templates/magazine/_federated_info.html.twig Outdated Show resolved Hide resolved
@andrewmoise
Copy link
Contributor Author

All good. I think the requested changes are in now -- did you want me to modify the last_origin_update semantics though? To me it still seems like when it gets updated still isn't the perfect setup last I looked.

@BentiGorlich
Copy link
Member

Nono, I'll do it, but in a different PR. I'd appreciate it if you would create an issue though 😁

@andrewmoise
Copy link
Contributor Author

Done 👍🏻

@BentiGorlich BentiGorlich added this to the v1.5.0 milestone Mar 15, 2024
@BentiGorlich
Copy link
Member

The Info for

Subscribe to start receiving updates.

Is not getting added to the always_disconnected_magazine_info

@andrewmoise
Copy link
Contributor Author

?

What do you mean by this?

@BentiGorlich
Copy link
Member

I meant that if the magazine never received any updates the info that ypu can subscribe to get updates is not being displayed

@andrewmoise
Copy link
Contributor Author

What? Last week you asked me to split it in two and only display the second sentence if the user is logged in. It's dependent on the existence of app.user, nothing to do with subscription status; you can look at the diff.

@BentiGorlich
Copy link
Member

Bildschirmfoto vom 2024-03-18 13-50-17
This is the part I mean and for this message the "subscribe to receive updates" is not getting displayed...

Yes this message should only be displayed to logged in users, but that part is missing here

@andrewmoise
Copy link
Contributor Author

Got it, I see what you meant. Fixed.

@BentiGorlich BentiGorlich enabled auto-merge (squash) March 18, 2024 13:24
@ghost ghost self-requested a review March 18, 2024 13:24
@BentiGorlich BentiGorlich merged commit 11559f2 into MbinOrg:main Mar 18, 2024
7 checks passed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Visual issues, improvements, bugs or other aspects relating mostly to the front end
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants