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

Add editing for published statuses #17320

Merged
merged 3 commits into from
Feb 9, 2022
Merged

Conversation

Gargron
Copy link
Member

@Gargron Gargron commented Jan 18, 2022

Follow-up to #16697

Through the REST API, a status can be updated using status, spoiler_text, sensitive, media_ids and poll params. If a poll is provided with different options, the votes are reset.

Does not remove the "Delete & redraft" option from the UI.


Funded through the BMBF (BMBF Förderkennzeichen: 01IS21S29)

@Gargron Gargron added api REST API, Streaming API, Web Push API ui Front-end, design labels Jan 18, 2022
Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments from the old PR

app/services/process_hashtags_service.rb Outdated Show resolved Hide resolved
app/presenters/activitypub/activity_presenter.rb Outdated Show resolved Hide resolved
app/controllers/api/v1/media_controller.rb Outdated Show resolved Hide resolved
app/controllers/api/v1/media_controller.rb Show resolved Hide resolved
@Gargron Gargron force-pushed the feature-status-edit branch 2 times, most recently from 3127e64 to c415357 Compare January 18, 2022 23:16
Base automatically changed from feature-status-edit to main January 19, 2022 21:37
@Gargron Gargron force-pushed the feature-status-edit-authoring branch 6 times, most recently from 0c47d59 to 7d37447 Compare January 27, 2022 16:23
app/models/status.rb Outdated Show resolved Hide resolved
@Gargron Gargron force-pushed the feature-status-edit-authoring branch 5 times, most recently from 70bb16d to 0cf16d1 Compare January 27, 2022 17:06
@Gargron
Copy link
Member Author

Gargron commented Jan 27, 2022

Updated.

@Gargron Gargron force-pushed the feature-status-edit-authoring branch 3 times, most recently from 8236095 to 55bef46 Compare January 29, 2022 20:10
@Gargron Gargron force-pushed the feature-status-edit-authoring branch from 55bef46 to 47299c1 Compare January 29, 2022 20:39
@Gargron Gargron force-pushed the feature-status-edit-authoring branch from 47299c1 to 7b9c5ac Compare January 29, 2022 20:41
@Gargron Gargron requested a review from ClearlyClaire February 6, 2022 06:19
Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with most of the changes, except with the media edit flow. I don't think it's good, neither for authors (who risk accidentally publishing changes without committing to save them) or consumers (which may lack the information something was updated). The more I think about it, the more I think it should be atomic somehow.

Otherwise the changes seem pretty solid to me. But I would not want the UI or API to be actually accessible yet, as support for receiving edited toots is not completely ironed out yet and hasn't been pushed to any release. It makes perfect sense for most of the backend changes (with their tests) to be merged in, though, but I'm less sure about UI changes (which do not have tests). In any case, I think the routes, MediaController, and status menu item changes should not be merged yet.

# votes, so we need to remove them
if @options[:poll][:options] != poll.options || ActiveModel::Type::Boolean.new.cast(@options[:poll][:multiple]) != poll.multiple
@poll_changed = true
poll.votes.delete_all unless poll.new_record?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed this, but I think the cached tallies are not reset, which they definitely need to be, here.

# This media attachment could have been detached, or the user might
# have updated the status already, in which case we don't need to
# do anything
return if @status.nil? || @status.edited_at > updated_at.to_datetime
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the text gets edited within the 5 minutes timeframe (without changing media ids), will the media change be recorded? I'm under the impression it will not.

Comment on lines 25 to 31
# If the media attachment being updated is attached to a published
# status, then the changes need to be recorded and distributed along
# with the status, but it may be that the status is going to be updated
# along with the media attachment, so we need to debounce
if @media_attachment.status_id.present? && @media_attachment.significantly_changed?
PublishMediaAttachmentUpdateWorker.perform_in(5.minutes, @media_attachment.id, @media_attachment.updated_at)
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an improvement over not recording changes, but I'm still uneasy with this. Even from an UX standpoint, this is weird: the changes are committed as soon as you edit the media, and even if you don't commit the overall change. Maybe this should be queued on the client-side and submitted at the same time as the status update.

}));
}
});
case COMPOSE_SET_STATUS:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could/should be factored in with the REDRAFT case but it's ok.

Comment on lines 126 to 134
it 'does not queue an update worker' do
expect(PublishMediaAttachmentUpdateWorker).to_not have_received(:perform_in)
end

it 'returns http not found' do
put :update, params: { id: media.id, description: 'Lorem ipsum!!!' }
expect(response).to have_http_status(:not_found)
context 'when already attached to a status' do
let(:status) { Fabricate(:status, account: user.account) }

it 'queues an update worker' do
expect(PublishMediaAttachmentUpdateWorker).to have_received(:perform_in)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to be reverted since the corresponding changes have been reverted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Gargron Gargron force-pushed the feature-status-edit-authoring branch from a5e3191 to 929b67c Compare February 9, 2022 22:35
Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think it should be disabled for now, in this PR, rather than merging the feature to immediately disable it (or worse, leaving it here, despite the lack of backward compatibility), but otherwise it looks good to me.

@Gargron Gargron merged commit 63002cd into main Feb 9, 2022
@Gargron Gargron deleted the feature-status-edit-authoring branch February 9, 2022 23:15
stsecurity pushed a commit to stsecurity/mastodon that referenced this pull request May 2, 2022
* Add editing for published statuses

* Fix change of multiple-choice boolean in poll not resetting votes

* Remove the ability to update existing media attachments for now
stsecurity pushed a commit to stsecurity/mastodon that referenced this pull request Jul 10, 2023
* Add editing for published statuses

* Fix change of multiple-choice boolean in poll not resetting votes

* Remove the ability to update existing media attachments for now
stsecurity pushed a commit to stsecurity/mastodon that referenced this pull request Oct 17, 2023
* Add editing for published statuses

* Fix change of multiple-choice boolean in poll not resetting votes

* Remove the ability to update existing media attachments for now
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api REST API, Streaming API, Web Push API ui Front-end, design
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants