Scrolling up discussion shows [deleted] for first post user

[dsevillamartin]

Bug Report

1. Click on post notification (seems to only bug out on your own discussions)
2. Scroll all the way up
3. See error

Happens to me @ https://discuss.flarum.org/d/20949-friendsofflarum-ban-ips.

Request isn't loading any user relationships. Might be that the first post data is loaded, excluding user, when clicking notification / navigating to post.

[Ralkage]

Just happened to me a few minutes ago on Discuss just mindlessly viewing the discussion I created (clicking on the notification from a new reply that is):

[katosdev]

Confirmed just now on Discuss:

Discussion URL : https://discuss.flarum.org/d/23121-covid-19-hows-it-affecting-you

[askvortsov1]

I can also confirm that this happens, good to see there's already an issue for it

[matteocontrini]

Excuse me for bumping this, but my users are complaining.

Does anyone have an idea of what might be causing this? I don't remember seeing this problem before 2019, and the issue creation date makes me think that it's a regression introduced with beta9 (which was developed in the period September 2018 - July 2019).

I've taken a quick look at commits and changes of that release but I couldn't immediately find anything that could be related to this, though...

[dsevillamartin]

@matteocontrini Essentially, I think it has to do with the first post of the discussion being loaded but not with its author relationship.

The way Flarum handles relationships is that they must be returned in the API request for them to be seen.

In other words, even if both a post and its author are in the JS store, unless the post has the relationship linking it with the author, post.user() will return false/null. As including relationships in API calls makes them take longer, we usually only request the info we need. Sadly, right now it is not possible to tell the API to only return the relationship (and not the actual referenced model, as for every relationship the API includes - { type: 'users', id: 1 } - it includes the actual model as well { type: 'users', id: 1, data: { displayName: '...' } }.

I also may have just gone on a tangent that has nothing to do with this issue. But I think if there was some way for Flarum to know from the API response who the author ID is, the issue would be resolved as the author is already most likely in the app.store (discussion author).

🤔 Feel free to yell at me if nothing I just wrote made sense.

[askvortsov1]

Don't we request post authors for all other posts in the discussion? Why wouldn't we just do the same for the first post?

[dsevillamartin]

@askvortsov1 I think the problem here is that the first post may already be loaded through the firstPost relationship? Not sure, but it's the only thing I can think of right now. Might be wrong.

What could help is seeing the HTTP response of the request when this bug occurs. That way we can see the requested data & returned payload.

[askvortsov1]

Hmm yeah that'd do it, and since we've already loaded the first post from the API, the post stream doesn't reload it... For this instance, feels like adding first post author to the includes should be fine? Although that'd affect discussion list performance...

[dsevillamartin]

I'm not sure if that's actually what's causing this. It's the only thing I can think of - but based on my comment before, I don't think discussion.posts() includes discussion.firstPost() (for the reasons mentioned above) so I don't know.

[matteocontrini]

What could help is seeing the HTTP response of the request when this bug occurs. That way we can see the requested data & returned payload.

I've been trying to reproduce the issue for the last 15 minutes... And I couldn't 😕

[askvortsov1]

When you try to reproduce, are you doing it on a refreshed page (nothing in store) by clicking on a notification and scrolling up?

[matteocontrini]

Ok reproduced now.

The discussion's author is myself, but I see [deleted]. The XHR request that was made is:

https://forum.fibra.click/api/posts?filter[id]=146716,146719,146721,146722,146726,146727,146728,146729,146730,146732,146734,146735,146736,146755,146756,146758,146762,146766,146768

And the discussion is this one.

Should the first post be loaded through this request? Because it's not... (the ID is 146708)

[dsevillamartin]

@matteocontrini Based on your findings, it looks like my suspicions are correct. The first post has already been loaded (w/o user relationship because discussion.firstPost()) so it isn't loaded again.

[matteocontrini]

I'm now seeing this issue not only on the first post but also with other posts in the "middle" of a discussion. I've seen it myself and another user reported this to me. I thought this issue was limited to the first post of a discussion?

[matteocontrini]

Just for the record, this happens also in the middle of the discussion, even when the post author is not the author of the discussion.

In the following screenshot, the author of the "second" post is still me (matteocontrini) but weirdly it's not shown:

[dsevillamartin]

More information on this. There's many issues that result in this behavior.

1) The JS assumes that if a post has a canEdit property that isn't undefined, all the information necessary from that post is present https://github.com/flarum/core/blob/3f8432a58994a37f42c9891949afacc8bf2524eb/js/src/forum/states/PostStreamState.js#L262-L264

• A better thing to check for would be the user relationship, but there's some issues there too 2) The API doesn't return a null relationship if a request relationship doesn't exist
• e.g. deleted user's post will not have a user relationship, so you cannot differentiate between user having been loaded or user not existing

The fact that doing ?include=user on a deleted user's post doesn't return any sign of the user being deleted is concerning and most likely not JSON-API compliant. We should most likely be setting the relationship to null in the response to indicate that it was loaded. Otherwise we cannot differentiate between needing to load a relation vs the relation not existing.

[matteocontrini]

Ah, it seems that this issue was present in the early days (#295) and it was (apparently not) fixed with f3612261ec02e6ea3d15db28451429165657a996 and previously with 6b601ae2d66cea485385d37f5a8c2cfb419c467f.

I'm wondering why the canEdit property was chosen for determining whether the post is fully loaded. 🤔

It seems there are still cases when that property is defined but some information is missing. This could also be the cause for the cases when the post likes are not loaded, as reported on Discuss here.

[dsevillamartin]

@matteocontrini Yep! When reproducing the issue on discuss, likes relation also wasn't loaded. I think this logic in the PostStreamState should be extendable so extensions can specify which information they need & mark a post as needing to be re-fetched from the API (apart from fixing the JSON-API issue)

[matteocontrini]

Ok, so to summarize there are two problems here:

• sometimes the user relationship isn't available, and this can actually happen on any post and not only on the first one. This could be fixed by adding a user check to the condition in the PostStreamState:

if (post && post.discussion() && typeof post.canEdit() !== 'undefined' && post.user()) { 

(or maybe directly as a replacement of the canEdit check)

This would fix the issue but it would have two disadvantages:

1) sometimes the user information is already available in cache but the relationship is not available, so the user data would be fetched again (negligible, IMO, seeing how rare the issue is) 2) posts whose author doesn't exist anymore would be unnecessarily fetched over and over

• sometimes extension data is missing from the cached post, for example likes data. This could be fixed by allowing extensions to participate in the decision on whether to load a post.

I'm guessing a way to solve this would be to move the if condition to a function:

isPostLoaded(post) {
  return post && post.discussion() && typeof post.canEdit() !== 'undefined' && post.user();
}

Which extensions would then be able to extend:

override(PostStreamState.prototype, 'isPostLoaded', function(super, post) {
  return super(post) && post.likes();
}

(I'm not entirely sure this is the best way to do it, but extend wouldn't work with a boolean, so...)

Did I miss something else?

[dsevillamartin]

The two problems I listed are not the ones you summarized - you extracted two problems from one of mine.

Problem 1

The extension data missing from the post is the same issue as the avatar not being present - the loaded post relationship from Notifications (in my test case) doesn't load the post author or likes relationships (for good reason). But then the forum doesn't re-fetch the information when that data isn't present.

It could be fixed by something like the isPostLoaded method that you said (you could use extend, not just override, btw).

Problem 2

There is no way to check if a user relationship has been loaded when the user doesn't exist. In both cases the user relation is missing from data.relationships. In other words, the isPostLoaded method (for example) would not be able to use post.user() as a check (which you mentioned), as it would return null in both cases.

The solution to this is solve what most likely is an incorrect implementation of the JSON-API and somehow return null for the relationship PHP-side when the relation does not exist BUT it is included in the response (as opposed to not loaded at all).

[askvortsov1]

The above linked issue, https://github.com/flarum/core/issues/2404, goes into more detail on some aspects of this.

[luceos]

I am no longer able to reproduce this, can someone confirm?

[dsevillamartin]

I don't think this would've been fixed with any recent changes. Tried reproducing on discuss, and cannot, but I don't see how this would've been fixed 🤔

[matteocontrini]

If it helps, it was definitely present in beta.16 because I'm still running it and the issue happens there.

[matteocontrini]

Still happening on v1.0 as well.

[iPurpl3x]

Has this been fixed ? Can someone reproduce it ? I could not, but I don't know if I did the right steps to try to reproduce.

[matteocontrini]

It still happens, although not necessarily always on the first post as in the original report.

Also, I'm not sure why bug issues weren't kept in the flarum/framework repository. Lots of bugs (even reproducible ones) are not being taken into account for this reason...

[luceos]

Also, I'm not sure why bug issues weren't kept in the flarum/framewory repository

https://discuss.flarum.org/d/29518-the-clean-slate-method-on-issues-and-pull-requests

Thanks for your understanding.

[matteocontrini]

@luceos I actually forgot the part "...after we've pulled issues we feel are important", oops. Apologies for the OT.