Prevent ad slots between posts from jumping around

[matteocontrini]

Currently, ad slots between posts are calculated by considering only the currently rendered posts (PostStream children):

https://github.com/FriendsOfFlarum/ads/blob/44f401a207ef2ca39684367a4339a7b6ea7b9bb2/js/src/forum/addAdBetweenPosts.js#L13-L26

This has the side effect that when new posts are loaded "above", ad slots are recalculated and might be positioned differently, since the code assumes that the first post of the discussion is the first vnode in the stream.

One solution to this would be to use the actual post number (post.attrs['data-number']) instead of the index of the vnode (i), and the total posts count (this.count()) instead of the total number of loaded comments (commentPosts.length). The main difference would be that all kinds of posts would be taken into account for the "between" count, instead of only the comment posts. I'm not even sure if this is better or worse in terms of where you would expect ads to be placed... But it is a solution.

[clarkwinkelmann]

Thanks for the report.

Post numbers alone probably won't solve the issue since we can't rely on numbers to be subsequent or even be in order.

The best solution would probably to read the full list of post IDs for the discussion, which is already loaded by the scrubber, calculate the positions based on that list, then identify the posts by their IDs in the stream and use that to identify where to insert ads.

[matteocontrini]

Hi Clark!

we can't rely on numbers to be subsequent

You're right, especially with hidden/deleted posts, I didn't consider that.

or even be in order

Could this actually happen? I know that the post stream is sorted by post creation date (not sure why honestly), but the post number is still incremented at each post and recomputed when you split/merge. It doesn't seem to behave very differently from the autoincrementing post ID, unless I'm missing something.

Anyway, you're right that the post ID solution is better, at least for the first reason above.

[clarkwinkelmann]

Could this actually happen?

At this moment, with the existing community extensions, and from a forum that started fresh with Flarum, probably very unlikely.

My own Author Change extension can be used to create such anomalies. Other extensions might as well.

This can also happen with faulty imports. I'm actually dealing with this problem right now and creating https://github.com/migratetoflarum/renumber-posts to rectify Flarum databases.

Not sure when we'll be able to implement this into Ads however 😬

[matteocontrini]

Ok! I'll probably send a PR at some point, it doesn't seem hard to implement