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

Improve random customers data #95

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

LuigiPulcini
Copy link

@LuigiPulcini LuigiPulcini commented Mar 24, 2022

All Submissions:

Changes proposed in this Pull Request:

This Pull Request contains two main enhancements:

  1. It will now be possible to pass the email domain name in the command line so that all the email addresses are generated with the same domain. For example, passing --email-domain=passeddomain.com, all the customers will have their email addresses in the format [email protected]. This is vital for email testing where, for example, an external server could be used to verify the delivery of test emails (e.g. email.ghostinspector.com). If no email domain name is passed, the customers' email addresses will have their domain name randomly generated by $faker->safeEmailDomain()
  2. With the approach in place at the moment, first and last name, username, and email address are randomly and independently generated, which ends up with the customer details being completely unrelated to one another (e.g. first name John, last name Doe, username karen.white, email address [email protected]). This leads to a certain degree of confusion when it comes to testing certain features. With the proposed change, all the user data will be unequivocally matching (e.g. first name John, last name Doe, username john.doe, email address [email protected] or [email protected] when an email domain is passed).

How to test the changes in this Pull Request:

  1. Input the following command: wp wc generate customers 10 --email-domain=passed-domain.com
  2. Verify that the 10 newly generated customers have their username and email addresses matching their first and last name and their email addresses have the email domain name passed in the --email-domain parameter.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?

Changelog entry

Enhancement: The domain name of the email addresses of customers can now be passed in the command line.
Enhancement: Usernames and email addresses of customers are now generated from the random first and last names.

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

Luc45
Luc45 previously approved these changes Jul 1, 2022
Copy link
Member

@Luc45 Luc45 left a comment

Choose a reason for hiding this comment

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

Works great! Thanks for the contribution!

wp wc generate customers 10

image

wp wc generate customers 10 --email-domain=foo-bar.com

image

I do expect some merge conflicts with #80, though.

I'll forward this internally to see if we can get it merged soon.

Comment on lines +124 to +128
$domain = '';

if ( ! empty( $assoc_args['email-domain'] ) ) {
$domain = $assoc_args['email-domain'];
}
Copy link

Choose a reason for hiding this comment

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

This can be consolidated to one line:

Suggested change
$domain = '';
if ( ! empty( $assoc_args['email-domain'] ) ) {
$domain = $assoc_args['email-domain'];
}
$domain = $assoc_args['email-domain'] ?? '';

do {
$username = self::$faker->userName();
} while ( username_exists( $username ) );
$safeEmailDomain = $emailDomain ? $emailDomain : self::$faker->safeEmailDomain();
Copy link

Choose a reason for hiding this comment

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

The second instance of the variable is redundant.

Suggested change
$safeEmailDomain = $emailDomain ? $emailDomain : self::$faker->safeEmailDomain();
$safeEmailDomain = $emailDomain ?: self::$faker->safeEmailDomain();

@jonathansadowski jonathansadowski dismissed Luc45’s stale review September 23, 2022 16:53

File conflicts from out of date files

@jonathansadowski
Copy link
Contributor

Hi @LuigiPulcini,

Apologies not getting to this sooner. It looks like there are some conflicts now. Would you mind resolving those so that we can give this a re-review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants