Skip to content
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

Fix for issue #462 #589

Merged
merged 4 commits into from
Feb 26, 2017
Merged

Fix for issue #462 #589

merged 4 commits into from
Feb 26, 2017

Conversation

rmhasan
Copy link
Contributor

@rmhasan rmhasan commented Feb 2, 2017

Modified uploadCompose action to send media ids of attached
media when sending a request. Modified create method in MediaController
to check if when posting a video, there are no other media attached
to the status by looking at the media ids sent from the uploadCompose
action. This is a fix for #462.

@Gargron
Copy link
Member

Gargron commented Feb 2, 2017

Hey! Thank you for taking the time to investigate this issue. I am not sure if this approach is ultimately correct - client apps could simply choose not to send other media IDs along. If it can be faked then it might as well be a client-side check (in so far as it would be possible to check the mime type of the selected file).

The final validation happens here: https://github.com/tootsuite/mastodon/blob/master/app/services/post_status_service.rb#L39 This is where it could really be fixed, as currently the only thing it does is ensure at most 4 media attachments.

You also don't need to worry about instantly deleting uploaded attachments. There is a cronjob that peridocally purges unattached media.

@rmhasan
Copy link
Contributor Author

rmhasan commented Feb 3, 2017

Hi @Gargron, good point on client apps choosing not to send media ids. I moved the validation check to post_status_service.rb. Please take a look at my latest commit.

@Gargron
Copy link
Member

Gargron commented Feb 4, 2017

Okay, final suggestion: attach_media happens after the status is created, so instead of raising an error I suggest silently discarding invalid media, i.e. if a video is present, discard the other media items.

@rmhasan
Copy link
Contributor Author

rmhasan commented Feb 5, 2017

Hi @Gargron, In my last commit I moved the validation check before creating the status. What do you think about that? Or do you want me to implement what you suggested?

@rmhasan
Copy link
Contributor Author

rmhasan commented Feb 17, 2017

Hi @Gargon, I added a new commit to this PR, to use the exception handlers in api_controller.rb

Modified uploadCompose action to send media ids of attached
media when sending a request. Modified create method in MediaController
to check if when posting a video, there are no other media attached
to the status by looking at the media ids sent from the uploadCompose
action.
Moved validation to services/post_status_service.rb
of mix of video and images in status, just wasn't rendering
the show action. I moved the validation before the status creation
Using catch statement in api_controller.rb to catch NotPermitted
Exception, and render error message
@Gargron Gargron merged commit 9433d03 into mastodon:master Feb 26, 2017
abcang added a commit to pixiv/mastodon that referenced this pull request Feb 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants