Add integration tests for edit status and edit poll

[andregasser]

No description provided.

[codecov]

Codecov Report

Merging #166 (efade7d) into master (beea849) will decrease coverage by 0.03%.
The diff coverage is n/a.

Additional details and impacted files

@@             Coverage Diff              @@
##             master     #166      +/-   ##
============================================
- Coverage     45.00%   44.97%   -0.03%     
+ Complexity      235      233       -2     
============================================
  Files            75       75              
  Lines          2171     2161      -10     
  Branches        115      115              
============================================
- Hits            977      972       -5     
+ Misses         1148     1143       -5     
  Partials         46       46              

Impacted Files	Coverage Δ
.../src/main/kotlin/social/bigbone/rx/RxAppMethods.kt	0.00% <ø> (ø)
...rc/main/kotlin/social/bigbone/rx/RxOAuthMethods.kt	0.00% <ø> (ø)
...ain/kotlin/social/bigbone/api/method/AppMethods.kt	78.94% <ø> (-1.06%)	⬇️
...n/kotlin/social/bigbone/api/method/OAuthMethods.kt	91.83% <ø> (+1.09%)	⬆️

[bocops]

Looks good. One question I had while writing the methods was whether editing a poll status using editStatus() really didn't reset the poll. A test along those lines could be added eventually, but shouldn't stop this PR to go through as is.

[andregasser]

Looks good. One question I had while writing the methods was whether editing a poll status using editStatus() really didn't reset the poll. A test along those lines could be added eventually, but shouldn't stop this PR to go through as is.

That's a good point. I'll add another test to this PR. After that, we can merge it.

[andregasser]

Good morning @bocops , I have written a test which checks that the poll data is still there after a poll's status text has been edited (test is added to this branch). Unfortunately, the poll data is reset after the edit. Do you eventually have time to look at this? If not, please let me know. Thanks! 🙂

[bocops]

@andregasser I'll have a look at it today. :)

[bocops]

@andregasser Editing the status not only resets the poll options, but removes the poll completely. I checked this by replacing the failing line in the test with assertNotNull(editedPoll2.poll).

Mastodon documentation is really sparse in that regard, but if that is the case I assume that we would need to add the same poll details (pollOptions and pollExpiresIn) already contained in the status when editing it. This assumptions seems to be supported by actual Mastodon instances, where editing a poll without changing its options still resets its expiry value.

I further assume that sending the same pollOptions with an edit means that already registered votes will not be reset - but we can't currently test this unless we add methods to vote on polls (which I could do later this week). If this is the case, the correct action would be to add all necessary parameters to editPoll() so that no poll status ever needs to be edited using editStatus(). Then, we should add documentation to both methods warning users that using editStatus() on a status with poll will remove that poll.

[andregasser]

Hm, the use case "edit status" starts to become a bit omplicated from a library user perspective.

I wonder if we could hide this complexity from the user and do something like this:

• provide just a single editStatus method that handles status updates
• If the status to be edited contains a poll, we store the existing poll data on the status before we edit it and re-apply it when we send the update.

Would be interesting to see if this works. Mastodon API docs simply mention "Note that editing a poll’s options will reset the votes." But setting the same poll values as part of the edit request is probably not considered a change, right? So hopefully, the poll is still there after editing the status....

Regarding the first bullet item above (provide just a single editStatus method):

By providing a number of overloads for the editStatus method we could make life easier for the user:

- editStatus(statusId, spoilerText, sensitive, language, mediaIds)
- editStatus(statusId, status, spoilerText, sensitive, language, mediaIds) 
- editStatus(statusId, status, spoilerText, sensitive, language, poll) 

I have created these signatures based on the restrictions / constraints documented in the Mastodon API docs.

The whole point actually is: If there's a poll present on the status to be edited, we just re-apply the values in the edit request, to make sure the poll is still there after the edit.

Alternatively, we could also check other wrapper libraries and see how they handle the edit scenario. Eventually, this gives some valuable input as well....

What do you think?

[bocops]

Yeah, I wondered about this myself, editing a poll is somewhat complicated. There are some alternatives, each with their own pros and cons, but I really don't know what would be the best way to proceed. Some ideas:

1. Have one editStatus method with all potential parameters, then let the user beware.
2. Have two methods editStatus and editPoll and restrict parameters to what's useful in each case, because those are the two major use cases for statuses (this is what we currently do)
3. Overload the editStatus method so that it accepts either poll or media data on top of the always useful parameters (this is your suggestion)
4. Considering that editing a status nearly always means getting the previous status data, we could
   4.a either make the necessary calls to get that status data within editStatus to get unchanged data
   4.b or add optional parameters previousStatus and previousStatusSource to editStatus to get data from there
5. Especially in combination with 1. or 2., we could provide a builder for library users who'd like to be safe about their edits, while still allowing a more direct access to Mastodon's API for everyone else.

The builder could look something like this:

val previousStatusId = "foobar" // ID of the status we want to edit
val previousStatus = getStatus(previousStatusId)
val previousStatusSource = getStatusSource(previousStatusId)

// allow reuse of data that the library user has downloaded already
StatusMethods.EditBuilder(previousStatus, previousStatusSource)
    .content("new content text") // editing content means that library user will have loaded and displayed to their user the previous content themselves, so this would be used in combination with this EditBuilder call
    .spoiler("spoiler text") // changes spoiler text and adds sensitivity flag, or removes if parameter is null
    .addMedia("mediaId") // add another image, keeping existing ones
    .removeMedia("mediaId2") // remove a specific image, but not any others
    .performEdit() // send all changes

// alternatively, call builder with an ID to have it load the data itself
StatusMethods.EditBuilder(previousStatusId)
    .resetPollOptions(listOf("Foo", "Bar", "Baz")) // make library user aware that this will reset the poll
    .changePollExpiry(300) // this might allow changing some of the poll but not its options, so not resetting it
    .performEdit() // send all changes

[andregasser]

Hello! I will need to dig in a bit more over the weekend to understand how the REST API works.

Some more feedback / questions from my side:

1. Status Source
Do you know what the purpose of the GET /api/v1/statuses/:id/source call is? It doesn't matter on what sort of status I run it (text post, post with poll, post with media), it always returns me these three attributes:

{
  "id":"<status id>",
  "text":"<status text>",
  "spoiler_text": "<spoiler text>"
}

I have observed that the Mastodon Web Client uses it every time when initiating a status edit.

2. Polls and Status Edits
I can confirm that the poll is not lost after a status edit, if we send the same poll value that was there before the status edit. This is also what the Mastodon Web Client does.

3. Method Structure
In general, I think it would be nice to have methods in our API that allow "low-level" access to the REST API by providing access to all the parameters (in case the user wants to fiddle arround and to stuff). But we should also provide helper methods, which allow the user to easily create statuses and polls, etc....

I will add more thoughts to this 3rd item later, need to leave now :-)

[bocops]

Those three attributes may come with HTML tags when loading a Status, while StatusSource contains just the plain text. Instead of having to strip HTML from status elements, a client can just present the respective StatusSource elements for editing.

[andregasser]

Hello @bocops ,

I would once more quickly come back to our discussion, on how many methods / which methods to offer for creating / editing statuses and polls. I have looked at the methods we currently have and I think, we should not change too much now. But I might have found some spots where things could be simplified even a bit more.

These are the methods we currently have in our code for creating / editing statuses and polls:

fun postStatus(
    status: String,
    visibility: Status.Visibility = Status.Visibility.Public,
    inReplyToId: String? = null,
    mediaIds: List<String>? = null,
    sensitive: Boolean = false,
    spoilerText: String? = null,
    language: String? = null
): MastodonRequest<Status>

fun editStatus(
    statusId: String,
    status: String,
    mediaIds: List<String>? = null,
    sensitive: Boolean = false,
    spoilerText: String? = null,
    language: String? = null
): MastodonRequest<Status>

fun scheduleStatus(
    status: String,
    scheduledAt: String,
    visibility: Status.Visibility = Status.Visibility.Public,
    inReplyToId: String? = null,
    mediaIds: List<String>? = null,
    sensitive: Boolean = false,
    spoilerText: String? = null,
    language: String? = null
): MastodonRequest<ScheduledStatus>

fun postPoll(
    status: String, 
    pollOptions: List<String>, 
    pollExpiresIn: Int, 
    visibility: Status.Visibility = Status.Visibility.Public,        
    pollMultiple: Boolean = false,
    pollHideTotals: Boolean = false,
    inReplyToId: String? = null,
    sensitive: Boolean = false,
    spoilerText: String? = null,
    language: String? = null
): MastodonRequest<Status>

fun editPoll(
    statusId: String,
    status: String,
    pollOptions: List<String>,
    pollExpiresIn: Int,
    pollMultiple: Boolean = false,
    pollHideTotals: Boolean = false,
    sensitive: Boolean = false,
    spoilerText: String? = null,
    language: String? = null
): MastodonRequest<Status>

fun schedulePoll(
    status: String,
    scheduledAt: String,
    pollOptions: List<String>,
    pollExpiresIn: Int,
    visibility: Status.Visibility = Status.Visibility.Public,
    pollMultiple: Boolean = false,
    pollHideTotals: Boolean = false,
    inReplyToId: String? = null,
    sensitive: Boolean = false,
    spoilerText: String? = null,
    language: String? = null
)

I see two things, which I think we could simplify here:

• put all the poll parameters into a separate data class (e.g. Poll or similar) to shorten / simplify the parameter list and make it more obvious, which settings belong to a poll
• merge the 'postStatus' / 'scheduleStatus' methods and the 'postPoll' / 'schedulePoll' methods into one each, as they only differ in the 'scheduledAt' parameter.

This would result in the following new method signatures:

fun postStatus(
    status: String,
    visibility: Status.Visibility = Status.Visibility.Public,
    inReplyToId: String? = null,
    mediaIds: List<String>? = null,
    sensitive: Boolean = false,
    spoilerText: String? = null,
    language: String? = null,
    scheduledAt: String? = null
): MastodonRequest<Status>

fun editStatus(
    statusId: String,
    status: String,
    mediaIds: List<String>? = null,
    sensitive: Boolean = false,
    spoilerText: String? = null,
    language: String? = null
): MastodonRequest<Status>

fun postPoll(
    status: String, 
    poll: Poll,
    visibility: Status.Visibility = Status.Visibility.Public,        
    inReplyToId: String? = null,
    sensitive: Boolean = false,
    spoilerText: String? = null,
    language: String? = null,
    scheduleAt: String? = null
): MastodonRequest<Status>

fun editPoll(
    statusId: String,
    status: String,
    poll: Poll,
    sensitive: Boolean = false,
    spoilerText: String? = null,
    language: String? = null
): MastodonRequest<Status>

Then, let's just describe in KDoc when which method is to be used. For the editPoll method, we add a note, that the poll must be sent exactly as it was before, otherwise it will be lost.

One issue that still would need to be solved is the thing with the different data returned when a status or a scheduled status is posted (Status / ScheduledStatus). Did not think in detail about this yet.

I would hold back creating builders for a next major release in order to save some time.

What do you think?

[bocops]

Regarding poll data, we already have ScheduledStatus.Poll, which is basically the data class you describe because ScheduledStatus just contains the data that is used to build a status in its params. We could move that class to somewhere else - although I'm not completely sure where that would be, as it is not a "first-class" Mastodon entity itself. Perhaps something like social.bigbone.api.entity.data or .internal would make the most sense? Generally a good idea, though. :)

Merging postX and scheduleX methods would indeed have the problem of different return values. Even if we could solve that, I wonder if we wouldn't just move complexity from one point (duplicate-looking methods) to another (single method that somehow switches different functionality based on its parameters) without actually reducing it.

I think this is another thing that might better be solved using a Builder that ends in either .post() (posting immediately, returning a Status) or .schedule(time) (scheduling and returning a ScheduledStatus).

[andregasser]

We actually have two poll classes at the moment:

• *.entity.Poll (used by Mastodon's polls endpoints)
• *.entity.ScheduledStatus.Poll (currently used to return poll data of a scheduled poll)

As proposed by you, we could move *.entity.ScheduledStatus.Poll into *.entity.data. I think this is a good place for helper data classes created by us. Then, we could use it also in the *Poll methods to simplify the parameter list.

So let's keep the rest as it is and just point out the important points in the KDoc.

Would that be fine for you as well?

[bocops]

Sounds good! :)