Allow factory state to be set #10

kielabokkie · 2018-10-04T04:32:02Z

This PR allows you to pass factory states via the config like below:

'mailables' => [
    UserVerified::class => [
        ['class' => User::class, 'states' => ['verified']]
    ]
]

kielabokkie · 2018-10-04T05:15:19Z

I have a few Mailables that need my model to be in a specific state. It makes the mailables configuration array a bit less clean but more flexible. Let me know what you think @introwit

introwit · 2018-10-04T17:54:23Z

database/factories/TestFactory.php

+
+/** @var \Illuminate\Database\Eloquent\Factory $factory */
+
+$factory->define(Test::class, function () {


I think we can move this file to the tests directory 😄

introwit · 2018-10-04T17:57:29Z

src/MailViewer.php

+                    $factoryStates = [];
+
+                    if (is_array($dependency)) {
+                        // Check if the dependency array contains factory states


I feel we can remove this comment since the code is readable enough 😄

introwit · 2018-10-04T17:58:23Z

src/MailViewer.php

+
+                    if (is_array($dependency)) {
+                        // Check if the dependency array contains factory states
+                        if (in_array('states', array_keys($dependency)) === true) {


We can remove that === true since in_array returns a boolean iirc 🙂

introwit · 2018-10-04T18:01:02Z

src/MailViewer.php

+                    if (is_array($dependency)) {
+                        // Check if the dependency array contains factory states
+                        if (in_array('states', array_keys($dependency)) === true) {
+                            // Set the states and actual dependency class


I guess we can remove this comment too 😄

introwit · 2018-10-04T18:03:24Z

src/MailViewer.php

                    if (is_string($dependency) && class_exists($dependency)) {
                        if (isset($eloquentFactory[$dependency])) {
-                            $args[] = factory($dependency)->create();
+                            $args[] = factory($dependency)->states($factoryStates)->make();


Curious to know why make() over create() 😄

The first reason was the tests, the create function will try to save the Test model in the database as it's a Illuminate model. I can make it some general model that doesn't save to the database to make the test pass too. The other reason is that you probably don't want to save the records to the database when you are just viewing the email via the /mails page, especially on a production environment ;) I actually didn't realise this was happening until I was editing this code.

introwit · 2018-10-04T18:05:01Z

src/MailViewer.php

@@ -70,6 +81,14 @@ public static function prepareMails(array $mailables): array
            $givenParameters = [];

            foreach ($dependencies as $dependency) {
+                if (is_array($dependency)) {
+                    // Check if the dependency array contains factory states
+                    if (in_array('states', array_keys($dependency)) === true) {


Same as above for the comment and === true 🙂

introwit · 2018-10-04T18:06:42Z

src/MailViewer.php

@@ -83,8 +102,7 @@ public static function prepareMails(array $mailables): array

            if ($constructorParameters !== $givenParameters) {
                throw new Exception(
-                    "The arguments passed for {$mailable} in the config/mailviewer.php file do not match with the constructor 
-                    params of the {$mailable} class or the constructor params of the {$mailable} class aren't typehinted"
+                    "The arguments passed for {$mailable} in the config/mailviewer.php file do not match with the constructor params of the {$mailable} class or the constructor params of the {$mailable} class aren't typehinted"


Can you revert this change so that we adhere to PSR2's guideline for length of the line being 120 chars max. Also, we won't need to horizontally scroll when interacting with this file on GitHub if done 😄

introwit · 2018-10-04T18:09:12Z

src/MailViewerServiceProvider.php

@@ -15,6 +16,15 @@ public function boot()
        $this->loadRoutesFrom(__DIR__ . '/../routes/web.php');

        $this->loadViewsFrom(__DIR__ . '/../views', 'mailviewer');
+
+        if (app()->environment() === 'testing') {


Curious to know what is the benefit of this container binding since it might effect user's own testing env

I had to do this to make sure the TestFactory.php class would be loaded when running the unit tests. Laravel sets the default environment to testing when running unit tests so shouldn't cause a problem for anyone's testing env. See the code on Laravel's repo.

introwit · 2018-10-04T18:12:51Z

@kielabokkie Thank you so much for the contribution Wouter ❤️ 🙂 Just some minor stuff and few questions out of curiosity, rest looks great 👌 Also, a small request can you update the comment in the config file to document the new behaviour & feature if possible? Thank you again 😄

kielabokkie · 2018-10-04T19:43:45Z

@introwit I've updated the PR based on your feedback. Let me know if there is anything else I can do.

introwit · 2018-10-04T20:18:37Z

Thank you so much @kielabokkie 🙂 Is it okay if I tag you in future conversations related to this feature, if any comes up? I will merge this and tag a new minor release v2.0.3 😄 Great work Wouter ❤️

kielabokkie · 2018-10-04T20:22:28Z

@introwit Yes of course, that's not a problem at all. Thank you for the awesome package 👍

introwit · 2018-10-04T20:29:16Z

@kielabokkie thank you for the contribution Wouter 🙂 Have tagged the new release v2.1.0 and added credits to you in the changelog & release. Great work 👌

kielabokkie · 2018-10-04T20:51:37Z

@introwit I just realised an issue with the environment check in the service provider. All the unit tests that use factories on my project are now failing because it's only loading the factory that is part of this package. I'll have a quick look to see how I can fix this.

introwit · 2018-10-04T20:54:29Z

@kielabokkie if it takes too long just send a PR that removes that test factory & that binding, since it's just needed for the test anyways 🙂

kielabokkie added 3 commits October 4, 2018 16:11

Allow states to be passed via the config

9ba8bdb

Added test to verify state is applied

f46993a

Fixed comment

c196249

introwit requested changes Oct 4, 2018

View reviewed changes

kielabokkie added 2 commits October 5, 2018 07:43

Code review fixes

46ec27d

Add description and example to config and readme

14bd8c0

introwit approved these changes Oct 4, 2018

View reviewed changes

introwit merged commit 6ff5c5b into JoggApp:master Oct 4, 2018

introwit mentioned this pull request Oct 4, 2018

Move loading of factories to the base test class #11

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow factory state to be set #10

Allow factory state to be set #10

kielabokkie commented Oct 4, 2018

kielabokkie commented Oct 4, 2018

introwit Oct 4, 2018

introwit Oct 4, 2018

introwit Oct 4, 2018

introwit Oct 4, 2018

introwit Oct 4, 2018

kielabokkie Oct 4, 2018

introwit Oct 4, 2018

introwit Oct 4, 2018

introwit Oct 4, 2018

kielabokkie Oct 4, 2018

introwit commented Oct 4, 2018

kielabokkie commented Oct 4, 2018

introwit commented Oct 4, 2018

kielabokkie commented Oct 4, 2018

introwit commented Oct 4, 2018

kielabokkie commented Oct 4, 2018

introwit commented Oct 4, 2018 •

edited

Loading


		/** @var \Illuminate\Database\Eloquent\Factory $factory */

		$factory->define(Test::class, function () {

Allow factory state to be set #10

Allow factory state to be set #10

Conversation

kielabokkie commented Oct 4, 2018

kielabokkie commented Oct 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

introwit commented Oct 4, 2018

kielabokkie commented Oct 4, 2018

introwit commented Oct 4, 2018

kielabokkie commented Oct 4, 2018

introwit commented Oct 4, 2018

kielabokkie commented Oct 4, 2018

introwit commented Oct 4, 2018 • edited Loading

introwit commented Oct 4, 2018 •

edited

Loading