Skip to content

Sync: Adda a way to distinguish between an added and an updated attachment#6884

Merged
enejb merged 2 commits intomasterfrom
add/sync-attachment-action
Apr 6, 2017
Merged

Sync: Adda a way to distinguish between an added and an updated attachment#6884
enejb merged 2 commits intomasterfrom
add/sync-attachment-action

Conversation

@enejb
Copy link
Copy Markdown
Member

@enejb enejb commented Apr 3, 2017

Changes proposed in this Pull Request:

Current we can't tell if an user added or updated an attachment.
This PR tries so solve this by adding the current filter to the action being synced.

Testing instructions:

  • Do the tests pass?
  • Notice when you add an attachment that the add_attachment action is being send to .com and that the edit_attachment action is being send to .com when you update the attachment info.

@enejb enejb added [Package] Sync [Status] Needs Review This PR is ready for review. labels Apr 3, 2017
@enejb enejb self-assigned this Apr 3, 2017
@enejb enejb requested review from gititon and lezama April 3, 2017 22:04
*/
do_action( 'jetpack_sync_save_add_attachment', $attachment_id, $attachment );
$filter = current_filter();
do_action( 'jetpack_sync_save_add_attachment', $attachment_id, $attachment, $filter );
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it would be more semantic and easier to search if we sync two different actions, what do you think?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes that makes sense I have split up the actions and now we sync both of them.

jeherve
jeherve previously requested changes Apr 4, 2017
*/
do_action( 'jetpack_sync_save_add_attachment', $attachment_id, $attachment );
$filter = current_filter();
do_action( 'jetpack_sync_save_add_attachment', $attachment_id, $attachment, $filter );
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you move the action one line up? The parser won't find the docblock if there is another line of code above it.

@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Apr 4, 2017
@enejb enejb changed the title Sync: Adda a way to distinguish between a added and updated attacment Sync: Adda a way to distinguish between a added and updated attachment Apr 4, 2017
@enejb enejb changed the title Sync: Adda a way to distinguish between a added and updated attachment Sync: Adda a way to distinguish between an added and an updated attachment Apr 4, 2017
@enejb
Copy link
Copy Markdown
Member Author

enejb commented Apr 4, 2017

I split the event. This will also require a change on the .com side. Will do this after a consensus on this approach. Also will need to update the tests of the activity log.

@enejb enejb added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Apr 4, 2017
@enejb enejb dismissed jeherve’s stale review April 4, 2017 22:03

addressed

@lezama
Copy link
Copy Markdown
Contributor

lezama commented Apr 5, 2017

I vote for this solution by a lot, let's fix the tests :)

@jeherve jeherve added [Status] In Progress [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Apr 6, 2017
Copy link
Copy Markdown
Contributor

@gititon gititon left a comment

Choose a reason for hiding this comment

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

Looks good to me, Enej!

@enejb enejb removed [Status] In Progress [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Apr 6, 2017
@enejb enejb merged commit f3d1b5f into master Apr 6, 2017
@enejb enejb deleted the add/sync-attachment-action branch April 6, 2017 21:35
jeherve added a commit that referenced this pull request Apr 24, 2017
eliorivero pushed a commit that referenced this pull request Apr 25, 2017
* Changelog: initial commit for 4.9 release.

* Changelog: add #6929

* Changelog: move old changelogs to changelog.txt

* Readme: restore deleted release post link.

The post is now live.

* Changelog: add #6853

* Changelog: add #6856

* Changelog: add #6857

* Changelog: add #6884

* Changelog: add #6885

* Changelog: add #6892

* Changelog: add #6894

* Changelog: add #6898

* Changelog: add #6899

* Changelog: add #6900

* Changelog: add #6909

* Changelog: add #6927

* Changelog: add #6947

* Chagelog: add #6958

* Changelog: add #6961

* Changelog: add #6963

* Changelog: add #6965

* Changelog: add #6986

* Changelog: add #7000

* Changelog: add #7013

* Changelog: add #7015

* Changelog: add #7019

* Changelog: add #7028

* Changelog: add #6998

* Changelog: add #6999

* Changelog: add #7044

* Changelog: add #6881

* Changelog: add #6922

* Changelog: add #6940

* Changelog: add #6962

* Changelog: add #6942

* Changelog: add #6959

* Changelog: add #7018

* Changelog: add #6948

* Changelog: add #6657

* Changelog: add #7030

* Changelog: add #7048

* Changelog: add #7031

* Changelog: add #6990

* Changelog: add #6957

* Changelog: add #7027
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants