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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add State alongside City (and other customer info). #2463

Merged
merged 9 commits into from Jun 28, 2019

Conversation

@KZeni
Copy link
Contributor

commented Jun 19, 2019

Adds an oddly missing State data point when it currently has Country, City, and Postal Code. I have a client with State-based campaigns, currently, so them not having that one data point is something that could/should be addressed for them and others.

Why would State not be included as a possible data point in the reporting if it's not shown by default (similar to City)?

Accessibility

I simply added a new data point based off of the existing City field so its accessibility qualities should carry over appropriately.

Changelog Note:

Enhancement: add state/region to Customers Report. 馃憦馃徎 @KZeni

KZeni added some commits Jun 19, 2019

Added state alongside city & other info, and made location info sorta鈥
鈥le for convenient grouping of data at a glance.

I figured there's no reason you couldn't sort by the location data just so you can group them together (ex. quickly see how many are from a specific state at a glance rather than having it spread out & not having a means to group them together from this view.)
@joshuatf
Copy link
Contributor

left a comment

Awesome- thanks for this addition, @KZeni!

There's one more update that needs to be made for tests to pass; changing 14 to 15 here:

$this->assertCount( 14, $properties );

Could you make that change if you get a chance?

Also, I think the decision to leave out state may have been intentional since many countries don't have "states." This is probably okay since we can toggle off this column easily though. /cc @LevinMedia @jameskoster for your thoughts.

@KZeni

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

@joshuatf Done!

I can see that, but WooCommerce itself has State as a data point with accompanying functions for processing it, etc. accordingly. So I really think it should be able to be surfaced if it's wanted/needed. For those that sell mostly/exclusively in the US; having State included is a must-have for location information.

Just because it isn't always present doesn't mean it shouldn't be available. For example, "Why have a Coupon column for the order report if some orders don't have coupons?" would apply to the reasoning for not including state/province.

As an aside, I could certainly see the "State" column's name changed to "State/Province" to be more widely applicable if that's a hangup.

@joshuatf
Copy link
Contributor

left a comment

Thanks for those changes @KZeni! I'll go ahead and approve this and we can make any changes in a follow-up.

As an aside, I could certainly see the "State" column's name changed to "State/Province" to be more widely applicable if that's a hangup.

This is a good idea and will probably resolve any lingering issues between stores/orders. @jameskoster Do you have an opinion on this?

@jameskoster

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

@joshuatf forgive me, where is this being added? :>

@KZeni

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

@jameskoster The goal is to get State (or rather "State/Province") shown alongside City & other info which is currently being shown as an optional column on Analytics => Customers for WooCommerce stats.

@joshuatf

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

@jameskoster As @KZeni noted (thanks!), this would be a column in the Customers report. Stores that don't need this can toggle the column off as needed. A label of State / Province might be more broadly matching across different addresses though- but you can weigh in there as well if you have an opinion.

Screen Shot 2019-06-27 at 11 33 28 PM

@LevinMedia

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2019

@KZeni @joshuatf Sorry I'm late to the party. This is fantastic, and something I've been hoping we could add for a while now. Excited to see this in the mix.

I did a little bit of digging, turns out there's quite the variety in what countries call their various administrative divisions. I think using "Region" would be a little more inclusive than even "State / Province", it's also less letters. (Even thought this is what some countries call their version of a state.)

This is the pattern Google Analytics follows as well: Country > Region > City

Screen Shot 2019-06-27 at 9 15 25 AM

@KZeni

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2019

@LevinMedia Oh yeah, that sounds like the best label to go with so far. More inclusive & concise. 馃憤

@jeffstieler

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

Just a heads up - I'm removing the [ready to merge] label since we're making a change to the field name. @KZeni please ping after you've updated state to region, or let us know if you'd like us to make the change instead. Thank you!

KZeni added some commits Jun 28, 2019

@KZeni

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

@jeffstieler Done.

I renamed the label in the table and API to be "Region" instead of "State". I kept it as "state" behind the scenes since WooCommerce itself considers this as state in its functions, keys, filters, etc. That way, it's consistent with WooCommerce's conventions while having the label be more appropriate for when it's shown.

label: __( 'Region', 'woocommerce-admin' ),
key: 'state',
hiddenByDefault: true,
isSortable: true,
},
{
label: __( 'Postal Code', 'woocommerce-admin' ),
key: 'postal_code',

This comment has been minimized.

Copy link
@jeffstieler

jeffstieler Jun 28, 2019

Contributor

Adding sorting highlights a bug we have - this should be postcode. Can you please fix @KZeni?

@jeffstieler

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2019

One more small tweak and this is ready to go @KZeni! Thank you for your contribution.

@KZeni

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

@jeffstieler Okay! That should be taken care of if I understood things correctly.

@jeffstieler jeffstieler merged commit 2582c29 into woocommerce:master Jun 28, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Isotope automation moved this from wc-admin Backlog 馃敊 to Done June 25 鈥 July 8 Jun 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can鈥檛 perform that action at this time.