Phishing Project - Refactor Using Models

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'=>'gaig_user',                 '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!

Replay

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'=>'gaig_user',
                '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);
    }
}

Category: php Time: 2016-07-29 Views: 0

Related post

iOS development

Android development

Python development

JAVA development

Development language

PHP development

Ruby development

search

Front-end development

Database

development tools

Open Platform

Javascript development

.NET development

cloud computing

server

Copyright (C) avrocks.com, All Rights Reserved.

processed in 0.152 (s). 12 q(s)