1
\$\begingroup\$

After receiving some good feedback on the previous question, I've refactored my database and have begun to refactor the associated code. As a result of implementing models for all tables in the database, it has become smoother to use Eloquence Builder instead of a custom class using PDO.

Below find some of the changes made. For this review, I'm looking to see how effective my implementation of the Eloquence Builder is and how streamlined this email process seems. That being said, when I was testing the validateMailingList function, the $projects was coming back as an array(array(array()))... Not sure why, but I was able to get it working, but not sure about the best way to parse and clean up this data.

PhishingController

/**
 * retrieveProjects
 * Helper function to grab the 3 most recent projects for a user, then grab the project object of each project.
 * 
 * @param   int             $id         Mailing_list ID of the requested user.
 * @return  array
 */
private static function retrieveProjects($id) {
    $result = Sent_Mail::where('SML_UserId',$id)
        ->limit(3)
        ->get();
    $result = json_decode($result,true);
    $projects = array();
    for($i = 0; $i < sizeof($result); $i++) {
        $project = Project::where('PRJ_Id',$result[$i]['SML_ProjectId'])->get();
        $projects[] = json_decode($project,true);
    }
    return $projects;
}

/**
 * validateMailingList
 * Validates all the mailing_list recipients. Returns only those that will receive the email.
 *
 * @param   array           $currentProject         The current project being validated against.
 * @param   int             $periodInWeeks          Number of weeks back to check for most recent email.
 * @return  array
 */
private static function validateMailingList($currentProject, $periodInWeeks) {
    $users = Mailing_List_User::all();
    $users = json_decode($users,true);
    $mailingList = array();
    for($i = 0; $i < sizeof($users); $i++) {
        $date = date('Y-m-d h:i:s',strtotime("-$periodInWeeks weeks"));
        $projects = self::retrieveProjects($users[$i]['MGL_Id']);
        if($projects[0][0]['updated_at'] <= $date) {
            $mailingList[] = $users[$i];
        }
        $complexity = $currentProject[0]['PRJ_ComplexityType'];
        $target = $currentProject[0]['PRJ_TargetType'];
        if((!is_null($projects[0]) &&
                $complexity == $projects[0][0]['PRJ_ComplexityType'] &&
                $target == $projects[0][0]['PRJ_TargetType'])
            ||
                (!is_null($projects[0][0]) && !is_null($projects[1][0]) &&
                    $complexity == $projects[0][0]['PRJ_ComplexityType'] &&
                    $complexity == $projects[1][0]['PRJ_ComplexityType'])
            ||
                (!is_null($projects[0][0]) && !is_null($projects[1][0]) && !is_null($projects[2][0]) &&
                    $target == $projects[0][0]['PRJ_TargetType'] &&
                    $target == $projects[1][0]['PRJ_TargetType'] &&
                    $target == $projects[2][0]['PRJ_TargetType'])) {
            //invalid recipient - something may happen here
        }
        else {
            $mailingList[] = $users[$i];
        }
    }
    return $mailingList;
}

/**
 * sendEmail
 * Function mapped to Laravel route. Defines variable arrays and calls Email Class executeEmail.
 *
 * @param   Request         $request            Request object passed via AJAX from client.
 */
public function sendEmail(Request $request) {
    try {
        $templateConfig = new TemplateConfiguration(
            array(
                'templateName'=>$request->input('emailTemplate'),
                'companyName'=>$request->input('companyText'),
                'projectName'=>$request->input('projectData')['projectName'],
                'projectId'=>intval($request->input('projectData')['projectId'])
            )
        );
        $currentProject = json_decode(Project::where('PRJ_Id',$templateConfig->getProjectId())->get(),true);
        $periodInWeeks = 4;
        $emailConfig = new EmailConfiguration(
            array(
                'host'=>$request->input('mailServerText'),
                'port'=>$request->input('mailPortText'),
                'authUsername'=>$request->input('fromEmailText'),
                'authPassword'=>'password',
                'fromEmail'=>$request->input('fromEmailText'),
                'subject'=>$request->input('subject'),
                'users'=>self::validateMailingList($currentProject,$periodInWeeks)
            )
        );

        Email::executeEmail($emailConfig,$templateConfig);
    } catch(ConfigurationException $ce) {
        //will be doing something here - what still has yet to be defined (likely just log the exception)
    } catch(EmailException $ee) {
        //will be doing something here - what still has yet to be defined (likely just log the exception)
    }
}

Email

/**
 * executeEmail
 * Public-facing method to send an email to a database of users if they are a valid recipient.
 *
 * @param   EmailConfiguration          $emailConfig            Email Configuration object containing required information to send an email
 * @param   TemplateConfiguration       $templateConfig         Template Configuration object containing required information to build a template
 * @throws  EmailException                                      Custom Exception to embody any exceptions thrown in this class
 */
public static function executeEmail(
    EmailConfiguration $emailConfig,
    TemplateConfiguration $templateConfig)
{
    self::setTemplateConfig($templateConfig);
    self::setEmailConfig($emailConfig);

    try {
        foreach($emailConfig->getUsers() as $user) {
            self::sendEmail($user);
            self::logSentEmail($user);
        }
    } catch(Exception $e) {
        throw new EmailException(__CLASS__ . ' Exception',0,$e);
    }
}

/**
 * logSentEmail
 * Updates the user with the newest project and rotates the old projects down one.
 *
 * @param   array           $recipient           Mailing_List_User array
 */
private static function logSentEmail($recipient) {
    $sent_mail = Sent_Mail::create(
        ['SML_UserId'=>$recipient['MGL_Id'],
        'SML_ProjectId'=>self::$templateConfig->getProjectId(),
        'SML_Timestamp'=>Carbon::now()]
    );
}

/**
 * sendEmail
 * Sends them an email to the specified user.
 *
 * @param   User_test           $user           User object
 * @throws  FailureException
 */
private static function sendEmail($user) {
    $templateData = array(
        'companyName'=>self::$templateConfig->getCompanyName(),
        'projectName'=>self::$templateConfig->getProjectName(),
        'projectId'=>self::$templateConfig->getProjectId(),
        'lastName'=>$user['MGL_LastName'],
        'username'=>$user['MGL_Username'],
        'urlId'=>$user['MGL_UniqueURLId']
    );
    $subject = self::$emailConfig->getSubject();
    $from = self::$emailConfig->getFromEmail();
    $to = $user['MGL_Email'];
    $mailResult = Mail::send(
        ['html' => 'emails.phishing.' . self::$templateConfig->getTemplate()],
        $templateData,
        function($m) use ($from, $to, $subject) {
            $m->from($from);
            $m->to($to);
            $m->subject($subject);
        }
    );
    if(!$mailResult) {
        throw new FailureException('Email failed to send to ' . $to . ', from ' . $from);
    }
}

Currently, I just pass the json_decode result of the users to the sendEmail function in the Email class. I'm curious if it would be smarter to define a User class, not a model, that would take the associative array from the User model and create a User Object. It would look something like:

public function __construct($userArray) {
    //validate array
    //assign private vars
}

I could then create getters to retrieve using object notation instead of array notation - $user->getLastName instead of $user['MGL_LastName'].

Beyond this minor piece that I'm unsure about, it's pretty straight forward. I've got a few minor bugs on how the data is being passed to the server, but that doesn't affect the server implementation. How well have I implemented Eloquence Builder?

Thanks for the review!

\$\endgroup\$
2
\$\begingroup\$

I think you probably made a good decision in looking at a more full-featured ORM, as from earlier code review on your former DBManager class, I was of the opinion that the class was not really adding much value. Now you have shifted to a model-based means to define the objects your application is working. I think this is a good step forward.

I agree with your thoughts that you should have a user object and perhaps a user list object, user list factory or something similar to control the creation of a user list.

My comments have been added within your code below in multi-line quotes.

PhishingController

/*
You should use a join here in this method and retrieve all emails that
meet your project criteria in a single query.
Any time you see yourself executing a query within a loop, this should
be a red flag to you that there is probably a better way to do this.
*/
private static function retrieveProjects($id) {
/*
Validation for $id?
*/
    $result = Sent_Mail::where('SML_UserId',$id)
        ->limit(3)
        ->get();
/*
$result from above should be an array, why are you using json_decode here?
*/
    $result = json_decode($result,true);
    $projects = array();
    for($i = 0; $i < sizeof($result); $i++) {
        $project = Project::where('PRJ_Id',$result[$i]['SML_ProjectId'])->get();
/*
json_decode?
*/
        $projects[] = json_decode($project,true);
    }

    return $projects;
}

private static function validateMailingList($currentProject, $periodInWeeks) {
    $users = Mailing_List_User::all();
/*
Why is JSON decode needed?
*/
    $users = json_decode($users,true);
    $mailingList = array();
/*
Can you not apply filtering to your DB query and only return list of
applicable users from DB rather than filter in application code?
*/
    for($i = 0; $i < sizeof($users); $i++) {
        $date = date('Y-m-d h:i:s',strtotime("-$periodInWeeks weeks"));
/*
You are retrieving projects in a loop and that method itself queries in loops
This code is exponentially complex :(
I know working with ORM's can make complex queries painful, but my guess
is that you really need to look at these two related methods and figure
out how to get only the meaningful user and related project records
from the database in a single query. Don't query all users, iterate of over
all of them, then for each of them iterate through all there projects
You are doing a lot more work here than is needed.
*/
        $projects = self::retrieveProjects($users[$i]['MGL_Id']);
/*
All the code in this section is really hard to read because of all
the $project[0][0][*] stuff. Get away from querying in loops and
you will be able to clean this whole section up, likely also including
these crazy conditionals.
*/
        if($projects[0][0]['updated_at'] <= $date) {
            $mailingList[] = $users[$i];
        }
        $complexity = $currentProject[0]['PRJ_ComplexityType'];
        $target = $currentProject[0]['PRJ_TargetType'];
        if((!is_null($projects[0]) &&
                $complexity == $projects[0][0]['PRJ_ComplexityType'] &&
                $target == $projects[0][0]['PRJ_TargetType'])
            ||
                (!is_null($projects[0][0]) && !is_null($projects[1][0]) &&
                    $complexity == $projects[0][0]['PRJ_ComplexityType'] &&
                    $complexity == $projects[1][0]['PRJ_ComplexityType'])
            ||
                (!is_null($projects[0][0]) && !is_null($projects[1][0]) && !is_null($projects[2][0]) &&
                    $target == $projects[0][0]['PRJ_TargetType'] &&
                    $target == $projects[1][0]['PRJ_TargetType'] &&
                    $target == $projects[2][0]['PRJ_TargetType'])) {
            //invalid recipient - something may happen here
        }
        else {
            $mailingList[] = $users[$i];
        }
    }
    return $mailingList;
}

/*
It doesn't look like this method has changed much.  I think my comments from last review are still valid here.  Mainly that is set up all your
dependencies here, including user list, then pass to Email::executeEmail()
*/
public function sendEmail(Request $request) {
    try {
        $templateConfig = new TemplateConfiguration(
            array(
                'templateName'=>$request->input('emailTemplate'),
                'companyName'=>$request->input('companyText'),
                'projectName'=>$request->input('projectData')['projectName'],
                'projectId'=>intval($request->input('projectData')['projectId'])
            )
        );
        $currentProject = json_decode(Project::where('PRJ_Id',$templateConfig->getProjectId())->get(),true);
        $periodInWeeks = 4;
        $emailConfig = new EmailConfiguration(
            array(
                'host'=>$request->input('mailServerText'),
                'port'=>$request->input('mailPortText'),
                'authUsername'=>$request->input('fromEmailText'),
/*
Can you get this password out of here and just have it as constant or
something. The ideal scenario woudl be if you could populate from
environmental variable and not have passwords in your codebase at all.
*/
                'authPassword'=>'password',
                'fromEmail'=>$request->input('fromEmailText'),
                'subject'=>$request->input('subject'),
                'users'=>self::validateMailingList($currentProject,$periodInWeeks)
            )
        );

        Email::executeEmail($emailConfig,$templateConfig);
    } catch(ConfigurationException $ce) {
        //will be doing something here - what still has yet to be defined (likely just log the exception)
    } catch(EmailException $ee) {
        //will be doing something here - what still has yet to be defined (likely just log the exception)
    }
}

Email

public static function executeEmail(
    EmailConfiguration $emailConfig,
    TemplateConfiguration $templateConfig)
{
    self::setTemplateConfig($templateConfig);
    self::setEmailConfig($emailConfig);

    try {
/*
Same comment as before.  Not really sure why user list is created from
email config object.
*/
        foreach($emailConfig->getUsers() as $user) {
            self::sendEmail($user);
            self::logSentEmail($user);
        }
    } catch(Exception $e) {
        throw new EmailException(__CLASS__ . ' Exception',0,$e);
    }
}


private static function logSentEmail($recipient) {
/*
I really like that you have moved this logging functionality into its
own method, though it is not clear how this method is called.
*/
    $sent_mail = Sent_Mail::create(
        ['SML_UserId'=>$recipient['MGL_Id'],
        'SML_ProjectId'=>self::$templateConfig->getProjectId(),
        'SML_Timestamp'=>Carbon::now()]
    );
}


private static function sendEmail($user) {
    $templateData = array(
        'companyName'=>self::$templateConfig->getCompanyName(),
        'projectName'=>self::$templateConfig->getProjectName(),
        'projectId'=>self::$templateConfig->getProjectId(),
/*
I agree with your thoughts above that user should be an object.
Also consider when making it an object aliasing your DB field name
such that the object properties are like 'lastName', 'username', etc.
instead of 'MGL_LastName' and such. 
*/
        'lastName'=>$user['MGL_LastName'],
        'username'=>$user['MGL_Username'],
        'urlId'=>$user['MGL_UniqueURLId']
    );
    $subject = self::$emailConfig->getSubject();
    $from = self::$emailConfig->getFromEmail();
    $to = $user['MGL_Email'];
    $mailResult = Mail::send(
        ['html' => 'emails.phishing.' . self::$templateConfig->getTemplate()],
        $templateData,
        function($m) use ($from, $to, $subject) {
            $m->from($from);
            $m->to($to);
            $m->subject($subject);
        }
    );
    if(!$mailResult) {
        throw new FailureException('Email failed to send to ' . $to . ', from ' . $from);
    }
}
\$\endgroup\$
  • \$\begingroup\$ In regards to the Eloquent Model, you would think it would come back as an array, but it comes back as JSON. Thus the decoding to an array. \$\endgroup\$ – Trojan404 Jul 29 '16 at 21:24
  • \$\begingroup\$ @Trojan44 It shouldn't return JSON unless you try to act on the result like a string, at which point a conversion to JSON is made. All documentation shows examples as returning array from get(). DO you have some configuration issue? Is there something in your model that is weird? \$\endgroup\$ – Mike Brant Jul 29 '16 at 21:32
  • \$\begingroup\$ Maybe, I'll have to go back in and double check the configuration. In regards to the password part, yea I just had it hard-coded in because of a bug I was working out. \$\endgroup\$ – Trojan404 Jul 29 '16 at 21:36
  • \$\begingroup\$ Would it be smart to design using chunk with the uncertainty of how much data will be consumed? \$\endgroup\$ – Trojan404 Jul 29 '16 at 22:35
  • \$\begingroup\$ @Trojan404 possibly. Typically, you might want to chunk up a result set if you concerns over holding too many models in memory at the same time. You might need to do some application profiling to understand how your application is using memory. \$\endgroup\$ – Mike Brant Jul 30 '16 at 12:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.