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

Contact Form: Add IP & date to CSV Export #14304

Merged
merged 6 commits into from
Jan 10, 2020
Merged

Contact Form: Add IP & date to CSV Export #14304

merged 6 commits into from
Jan 10, 2020

Conversation

airdrummer
Copy link
Contributor

@airdrummer airdrummer commented Jan 7, 2020

Fixes #
Changes proposed in this Pull Request:

add feedback ip & date to csv export

Testing instructions:

Start with a connected Jetpack site.
Add a Contact Form in the usual manner - block or classic editor.
Add 4-5 contact form responses.
Export via CSV by going to wp-admin, Feedback-> Export
Open the CSV and confirm the date/IP are included.

Proposed changelog entry for your changes:

add feedback ip & date to csv export

add feedback ip & date to csv export
@airdrummer airdrummer requested a review from a team January 7, 2020 20:15
@jetpackbot
Copy link

jetpackbot commented Jan 7, 2020

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: January 14, 2020.
Scheduled code freeze: January 7, 2020

Generated by 🚫 dangerJS against fae73a3

@jeherve jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Contact Form [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jan 8, 2020
@jeherve
Copy link
Member

jeherve commented Jan 8, 2020

Thanks for the contribution. Unit Tests are currently failing with your change:

1) WP_Test_Grunion_Contact_Form::test_personal_data_exporter
has total expected data keys
Failed asserting that 8 is identical to 6.
/tmp/wordpress-latest/src/wp-content/plugins/jetpack/tests/php/modules/contact-form/test-class.grunion-contact-form.php:1405

You'll consequently need to update the test to take your changes into account.

Could you also add testing instructions to your Pull Request's description?

Thank you!

total expected data keys now = 8
@airdrummer
Copy link
Contributor Author

airdrummer commented Jan 9, 2020

ok, fixed test...not sure about instructions

@kraftbj
Copy link
Contributor

kraftbj commented Jan 9, 2020

Thanks @airdrummer. With instructions, they don't need to be complex, but something like:

  1. Start with a connected Jetpack site.
  2. Add a Contact Form in the usual manner - block or classic editor.
  3. Add 4-5 contact form responses.
  4. Export via CSV by going to wp-admin, Feedback-> XYZ
  5. Open the CSV and confirm the date/IP are included.

Enough so a reviewer can know the expected path on exposing the new feature.

@airdrummer
Copy link
Contributor Author

airdrummer commented Jan 9, 2020

ok, thanx, kraftbj...not sure if my hacks r the most efficient way to add ip & datestamp, and my definition of metadata seems 2 b different from wordpress's;-} but it fixes a severe deficiency in export.

curiously, if you have 2 different contact forms & export separately, the field order is reversed but idk as long as the metadata is complete;-)

@kraftbj
Copy link
Contributor

kraftbj commented Jan 9, 2020

Pushed up a commit to fix the PHPCS violations.

@kraftbj kraftbj added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jan 9, 2020
modules/contact-form/grunion-contact-form.php Show resolved Hide resolved
modules/contact-form/grunion-contact-form.php Outdated Show resolved Hide resolved
modules/contact-form/grunion-contact-form.php Outdated Show resolved Hide resolved
@kraftbj kraftbj added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 9, 2020
@kraftbj kraftbj added this to the 8.2 milestone Jan 9, 2020
@kraftbj
Copy link
Contributor

kraftbj commented Jan 9, 2020

A couple more minor things that I added suggestions for, but I think we can aim to land this in 8.2.

@kraftbj kraftbj self-assigned this Jan 9, 2020
airdrummer and others added 2 commits January 9, 2020 21:37
protect against uset IP

Co-Authored-By: Brandon Kraft <public@brandonkraft.com>
use unambiguous date format

Co-Authored-By: Brandon Kraft <public@brandonkraft.com>
@jeherve jeherve changed the title add feedback ip & date to csv export Contact Form: add feedback ip & date to csv export Jan 10, 2020
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This seems to test well for me. 👍

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jan 10, 2020
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello airdrummer! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D37523-code before merging this PR. Thank you!

@kraftbj
Copy link
Contributor

kraftbj commented Jan 10, 2020

201495-wpcom

@kraftbj kraftbj changed the title Contact Form: add feedback ip & date to csv export Contact Form: Add IP & date to CSV Export Jan 10, 2020
@kraftbj kraftbj merged commit 6fec780 into Automattic:master Jan 10, 2020
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jan 10, 2020
jeherve added a commit that referenced this pull request Jan 17, 2020
jeherve added a commit that referenced this pull request Jan 28, 2020
* [not verified] Remove empty readme section

* Initial changelog for 8.2

* Changelog: add #14220

* Changelog: add #14252

* Changelog: add #14291

* Changelog: add #14309

* Changelog: add #14304

* Changelog: add general connection log.

* Changelog: add #14275

* Changelog: add #14313

* Changelog: add #14213

* Changelog: add #14357

* Add sync testing instructions

* Add 8.1.1 changelog back

See eeaafab and 61757eb

* Changelog: add #14371

* Changelog: add #14386

* Changelog: add #14471

* Changelog: add #14325

* Changelog: add #14194

* Changelog: add #14340

* Changelog: add #14418

* Changelog: add #14417

* Changelog: add #14075

* Changelog: add #14467

* Changelog: add #14307

* Changelog: add #14326
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Contact Form [Status] Tested on WP.com Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants