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

Ensure replied-to is a status not a boost #9129

Merged
merged 5 commits into from Nov 25, 2018

Conversation

Projects
None yet
5 participants
@valerauko
Copy link
Contributor

valerauko commented Oct 28, 2018

Issue

Fixes #8794

Outline

If the given in-reply-to status is a boost, use the ID of the original status for replying.

To do

  • tests for this specific case
@puckipedia

This comment has been minimized.

Copy link
Collaborator

puckipedia commented Oct 28, 2018

so, suggestion, do the same thing reblogs do, so this isn't just fixed on the API side but on the federation side too:

def set_reblog
self.reblog = reblog.reblog if reblog? && reblog.reblog?
end

and
before_validation :set_reblog

(it's also less code :P)

@valerauko

This comment has been minimized.

Copy link
Contributor Author

valerauko commented Oct 28, 2018

I thought it's the same thing, I just put it in the creation service instead of a pre-save macro.

You say this belongs in the model instead?

@puckipedia

This comment has been minimized.

Copy link
Collaborator

puckipedia commented Oct 29, 2018

it's not the same thing. I can still send a s2s Create of a Note that is inReplyTo a boost. I think it's better in the model, because it fixes it in all cases, not just this single edge case.

@valerauko

This comment has been minimized.

Copy link
Contributor Author

valerauko commented Oct 29, 2018

That's my lack of understanding then. Gonna adjust.

@valerauko

This comment has been minimized.

Copy link
Contributor Author

valerauko commented Oct 30, 2018

Moved to model.

@Gargron

This comment has been minimized.

Copy link
Member

Gargron commented Nov 23, 2018

@nightpool @ThibG Thoughts? I'm not sure replying to boosts is a bug, we certainly aren't taking advantage of it in the UI, but I'm not sure if we never will.

@nightpool

This comment has been minimized.

Copy link
Collaborator

nightpool commented Nov 23, 2018

hmm. I'm worried that by merging this, we'd be encouraging client developers to be loose about boost/original IDs in other places as well.

@nightpool

This comment has been minimized.

Copy link
Collaborator

nightpool commented Nov 23, 2018

however, since we already do this for boosting posts, maybe there's an argument that we should do it for replies for symmetry

@valerauko

This comment has been minimized.

Copy link
Contributor Author

valerauko commented Nov 24, 2018

I don't have any strong feelings about this, but I'd like to make some points:

  • validating user input doesn't mean encouraging invalid input
  • the current behaviour results in unexpected UX on the front
  • it's literally a one-line change so if you ever want to implement separate conversation threads for boosts it's no effort to revert
@ThibG

This comment has been minimized.

Copy link
Collaborator

ThibG commented Nov 24, 2018

I'm fine with this. We aren't storing AP objects anyway, it makes sense to change this in the model for our purposes.

@@ -434,6 +434,9 @@ def set_visibility
end

def set_conversation
# clients might send the ID of the boost itself instead of the boosted status

This comment has been minimized.

@Gargron

Gargron Nov 24, 2018

Member

I think this comment is unnecessary

@Gargron Gargron merged commit db9aea3 into tootsuite:master Nov 25, 2018

11 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: check-i18n Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.3 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.3 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-webui Your tests passed on CircleCI!
Details
codeclimate All good!
Details

@valerauko valerauko deleted the valerauko:prevent-boost-reply branch Nov 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment