Skip to content

Sync: Avoid synching personal data in WooCommerce orders #9058

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.

Sign up for GitHub

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

Jump to bottom
Merged
dereksmart merged 2 commits into from
Mar 23, 2018
Merged

Sync: Avoid synching personal data in WooCommerce orders #9058

dereksmart merged 2 commits into from
Mar 23, 2018

Conversation

@allendav
Copy link
Contributor

@allendav allendav commented Mar 15, 2018 •
edited
Loading

Changes proposed in this Pull Request:

  • Removes some of the fields added in 5fbe2f5
  • We will have to sync this code change to the server side too

Testing instructions:

  • N/A - Although D9552 had them, the stats work in D9461 did not rely on these fields

@allendav allendav requested a review from westi March 15, 2018 20:39
@allendav allendav requested a review from a team as a code owner March 15, 2018 20:39
westi
westi requested changes Mar 15, 2018
View reviewed changes
Copy link
Contributor

@westi westi left a comment

Choose a reason for hiding this comment

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

Can we change this to comment out, and explain, the fields that are not being synced.

This will make it easier to review future updates and help these not get re-instated.

Bonus: Can we add a test to ensure they are not re-added?

@allendav
Copy link
Contributor Author

allendav commented Mar 15, 2018

@westi wrote:

Can we change this to comment out, and explain, the fields that are not being synced.

This will make it easier to review future updates and help these not get re-instated.

Bonus: Can we add a test to ensure they are not re-added?

Ooh - good ideas - will do

@jeherve jeherve added [Type] Bug When a feature is broken and / or not performing as intended [Package] Sync [Pri] High [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Mar 16, 2018
@jeherve jeherve added this to the 6.0 milestone Mar 16, 2018
@allendav allendav self-assigned this Mar 16, 2018
@westi westi added [Status] Ready to Merge Go ahead, you can push that green button! 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 Mar 16, 2018
@allendav
Copy link
Contributor Author

allendav commented Mar 16, 2018 •
edited
Loading

@westi wrote:

Can we change this to comment out, and explain, the fields that are not being synced.

Fixed in 4283b67

Bonus: Can we add a test to ensure they are not re-added?

@westi - I've created a unit test which I believe is correctly written, but it is failing (so I have not added it to this PR yet - can you take a look - my Jetpack foo is limited:

https://gist.github.com/allendav/47658545cedfb8f05eb1effe918d5f08

Also noteworthy: #9065

edit: I see you've added the ready-to-merge label - we can always add the unit test in a subsequent PR instead, especially once 9065 is fixed

@westi
Copy link
Contributor

westi commented Mar 16, 2018

I think @enejb or @lezama probably know more about the Jetpack Sync tests than I do.

@oskosk
Copy link
Contributor

oskosk commented Mar 16, 2018

@allendav can these be removed instead of commented out ?

@allendav
Copy link
Contributor Author

allendav commented Mar 20, 2018

@oskosk wrote

@allendav can these be removed instead of commented out ?

Let's keep them as comments - @westi had the good point that it should help keep someone from putting them back again as array members :)

@allendav allendav mentioned this pull request Mar 20, 2018
Closed
@oskosk
Copy link
Contributor

oskosk commented Mar 20, 2018

WOW, I totally missed westi's comment when I dropped mine. Thanks @allendav !

@dereksmart dereksmart merged commit 162b4a3 into master Mar 23, 2018
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 23, 2018
@dereksmart dereksmart deleted the update/woocommerce-orders-synched-data branch March 23, 2018 18:52
oskosk added a commit that referenced this pull request Mar 26, 2018
@oskosk
Add #9058 to changelog
3254474
dereksmart pushed a commit that referenced this pull request Mar 27, 2018
@oskosk @dereksmart
Changelog 6.0: running changelog (#9034)
f54fc7b 
* Changelog 6.0: create base for changelog.

* Add #8938 to changelog

* Add #8962 to changelog

* Add #8974 to changelog

* Add #8975 to changelog

* Add #8978 to changelog

* Add #8867 to changelog

* Add #8937 to changelog

* Add #8961 to changelog

* Add #8855 to changelog

* Add #8944 to changelog

* Add #8973 to changelog

* Add #8977 to changelog

* Add #8979 to changelog

* Add #8980 to changelog

* Add #8982 to changelog

* Add #8983 to changelog

* Add #8984 to changelog

* Add #8986 to changelog

* Add #9005 to changelog

* Add #9010 to changelog

* Add #9012 to changelog

* Add #9021 to changelog

* Add #9022 to changelog

* Add #9056 to changelog

* Add #9061 to changelog

* Add #9079 to changelog

* Add #9080 to changelog

* Add #9088 to changelog

* Add #9096 to changelog

* Add #9097 to changelog

* Add #9100 to changelog

* Add #9107 to changelog

* Add #8969 to changelog

* Add #8993 to changelog

* Add #9003 to changelog

* Add #9031 to changelog

* Add #8945 to changelog

* Add #9052 to changelog

* Add #9058 to changelog

* Add #9066 to changelog

* Add #9076 to changelog

* Add #9053 to changelog

* Add #9108 to changelog

* Add #9135 to changelog

* Add #9148 to changelog

* Add #9125 to changelog

* Add #9137 to changelog

* Added testing instructions for 6.0.

* Added IS testing instructions, huge props to @tiagonoronha.

* Added #8498 to changelog.

* Added #8954 to changelog.

* Added #8985 to changelog.

* add #9027

* add #9112 to changelog

* add #9136 to changelog

* add #9102 to changelog

* add #9093 to changelog

* add #9062 to changelog

* add #9172 to changelog
@emdashcodes emdashcodes mentioned this pull request Apr 27, 2018
Merged
@brezocordero brezocordero mentioned this pull request Aug 1, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@westi westi westi approved these changes

Assignees

@allendav allendav

Labels

[Package] Sync [Pri] High [Type] Bug When a feature is broken and / or not performing as intended

Projects

None yet

Milestone

6.0

Development

Successfully merging this pull request may close these issues.

7 participants

@allendav @westi @oskosk @jeherve @dereksmart @matticbot