JSON Export: Not exporting pinned posts

[sandrockcstm]

Describe the bug

Currently, when a user exports their data via JSON, posts in a collection which are pinned are not included in the exported JSON file. This behavior is absent from the txt and csv exports (but IS present for the Prettified export).

Steps to reproduce (if necessary)

Steps to reproduce the behavior:

1. Create a post.
2. Click on "pin" from that collection's page (i.e. domain-name/my-cool-blog)
3. Go to domain-name/me/export.
4. Click "JSON" or "Prettified" and download the file.
5. Open the JSON file. The pinned post will be missing from that collection's posts value (the array will be empty if there are no unpinned posts in that collection).

Expected behavior

All posts should be included in the posts array, regardless of its pinned_position value.

Application configuration

• Multi-user mode
• Mysql
• Open registration? Yes
• Federation enabled? Yes

Version or last commit: 0.9.1

[joyeusenoelle]

Exporting the JSON file calls viewExportFull() in account.go; that function calls compileFullExport() to select data from the database.

compileFullExport() is defined in export.go. At line 114 it calls app.db.GetPosts().

GetPosts() is defined (as a method on db) in database.go, and explicitly doesn't retrieve pinned posts. There's an additional GetPinnedPosts() at line 1527.

Which is the better solution:

• Edit compileFullExport() in export.go to also add GetPinnedPosts() and append it to collObjs
• Add a GetAllPosts() method to database.go, which ignores pinned status, and replace GetPosts() with GetAllPosts() at export.go:114
• The previous option, but rename GetPosts() to GetStandardPosts(), make the new method GetPosts(), and refactor throughout to make sure everything that used to call GetPosts() now calls GetStandardPosts()

The first is probably the easiest, but leaves the door open for this bug to appear in other functions. The third satisfies my desire to make things more understandable for future developers (WordPress has similar issues with some of its functions and hooks), but it's more work and might introduce errors if it's not done carefully. The second, I think, splits the difference nicely; it's a non-breaking change even if the names of the functions are imperfect.

Thoughts?

[joyeusenoelle]

Addendum: It's worth noting that we can't simply duplicate what the CSV/TXT export options do. The CSV and TXT options both refer to viewUserPosts() in account.go, which itself refers to app.db.GetUserPosts() in database.go. GetUserPosts() retrieves all posts for the current user, regardless of pinned status. But GetUserPosts() only selects a specific user's posts - and the JSON export option wants all the posts, regardless of user.

I bring this up here but it's potentially fodder for discussion on the forums: should we consider refactoring the export process, so that JSON isn't separate from CSV and TXT? In other words, users could export either just posts, in CSV, TXT, or JSON format, or export user+blogs+posts, also in CSV, TXT, or JSON format. I'm not sure of the rationale behind the CSV+TXT/JSON split, but it seems like it might be useful to merge everything together and allow all three formats for both export options.

[sandrockcstm]

I think I agree with you that option 2 is probably the best compromise between solving both this bug and potential future bugs and also maintaining consistent behaviour so that a complete refactor isn't necessary.

Though, as you say, the reason for the split might make a big difference on how we handle this, and a refactor, as difficult as it would be, might be the best option depending on the design philosophy at play here.

[thebaer]

I think the path of least resistance will be best here, as this is one special case where we need to do things differently against several other places in the app where the current setup is correct (i.e. getting posts for displaying blogs via HTML, the API, ActivityPub outbox, etc.).

I agree the naming could probably be better, but refactoring won't be worth the time investment / potential stability issues right now. So I'd say we go with the first option, adding a GetPinnedPosts() call in that for loop during the full export. Maybe we call it before GetPosts(), so pinned posts come out at the top.

Since both of you have been in that code now, if either of you wants to quickly knock this out, just let us know and assign the task to yourself. Otherwise I'll take care of it when I have some free time.

[joyeusenoelle]

I'll pick this one up. @thebaer, on further review, I've noticed an issue, though: GetPinnedPosts() only gets summaries for each post (including only the first 80 characters of the content). Is there another function I'm missing that gets the full text of all the pinned posts, or should I write a GetFullPinnedPosts() function modeled on GetPosts()? (Or, since I'm going to have to be in database.go anyway, should I just implement GetAllPosts() like in option #2? It's worth noting that I think this is trivial - I just have to remove AND pinned_position IS NULL from the query string, and everything else is the same.)

[joyeusenoelle]

I've offered two branches off develop for review. One adds GetAllPosts() to database.go and changes export.go to call it; the other changes GetPosts() to have a new parameter, includePinned, modifies the SQL query based on its value, and changes each place where GetPosts() is called to include an argument for the new parameter.

I leave it to you to choose which (if either) to include (the second is easier to maintain but we'll have to remember the new parameter if any new calls are made).