Phishing Project Refactoring to Use a User Object

I have added thoughts within multi-line comments below.

User_test

/*
I know you mentioned that you need to add validation here,
which I agree with.

I don't really like the approach of having a DB query outside of this class
to read this information and then sending to this class to instantiate.
Why not have that query here in this class so that, given DB connection
as dependency, this class has full ability to set up the user, rather
than having some other logic in the application to do this? Why should
some other part of the code need to know things like what table the user
data is stored in, what the schema for that table looks like, etc.

Should DBManager be passed to constructor as dependency since it is used
in other methods?

Just try to be consistent as to whether this class is supposed to interact
with database or not. It seems odd to mix approaches here. If the intent
is to handle ALL database access for users through this class (i.e. this
class IS the model), then it should truly handle ALL access including
getting the user info which is currently being passed to constructor.

This model seems really unclear in terms of how projects are handled.
Should projects (and/or a collection of projects) have their own object
definitions?  At a minimum, should this be an array of projects rather
than previous, most recent, last (which seems unclear as to what the
difference between these is anyway). An ordered array would let you 
get to whatever project you were interested in. Also, should there not
be 1 to 1 relationship between projects and unique URL's since they
are based on project ID's?  Here it seems like the user object can
have multiple projects, but only one unique URL, which doesn't make
sense to me.
*/
public function __construct($user)
{
    $this->id = $user['USR_UserId']; //required
    $this->username = $user['USR_Username']; //required
    $this->email = $user['USR_Email']; //required
    $this->firstName = $user['USR_FirstName']; //required
    $this->lastName = $user['USR_LastName']; //required
    $this->uniqueURLId = $user['USR_UniqueURLId'];
    $this->password = $user['USR_Password']; //required
    $this->mostRecentProject = $user['USR_ProjectMostRecent'];
    $this->previousProject = $user['USR_ProjectPrevious'];
    $this->lastProject = $user['USR_ProjectLast'];
}


/*
Is this method named properly?  It does more than check, so perhaps it should
be called setURLIdForProjectId().
It seems unclear to me in this model if a user could have multiple URL's in
case where they have multiple project ID's. If this is not the case,
what value does projectID even have with regards to this URL?
This method only considers happy path.  What if query fails (for example if
all URL's in database must be unique and generated string matches existing
URL in database)?
*/
private function checkURLId($projectId) {
    if(is_null($this->uniqueURLId)) {
        $db = new DBManager();
        $this->uniqueURLId = RandomObjectGeneration::random_str(15) . $projectId;
        $sql = "UPDATE gaig_users.users SET USR_UniqueURLId=? WHERE USR_UserId=?;";
        $bindings = array($this->uniqueURLId,$this->id);
        $db->query($sql,$bindings);
    }
}

/*
Not a very good method name. This method doesn't return anything and
isn't operating against $validUsers by reference so it is really unclear
to me what you are intending to do.
*/
public function pushUser($validUsers, $periodInWeeks, TemplateConfiguration $templateConfig) {
    try {
        $this->checkURLId($templateConfig->getProjectId());
        if($this->isValid($periodInWeeks,$templateConfig)) {
            $validUsers[] = $this;
        }
    } catch(Exception $e) {
/*
Either do something meaningful here or don't catch. Right now you are silently
swallowing this exception.
*/
    }
}

/*
Method name does not seem useful. What does "isValid" mean? You are not
validating that the user is valid here right? But rather that the user
is eligible for some email to be sent.  I would think User::isValid() 
method would tell you if the user itself is valid.

My worry is that you are beginning to pollute the user object with methods, knowledge
that is should not have.  Why would the User class need to read through
a template object to understand if the user needs to get a mailing?
I would think this would live in a class that prepares a list of users for
email send, not within user class itself.

Pass DBManager to object as dependency (probably in constructor)
*/
private function isValid($periodInWeeks, TemplateConfiguration $templateConfig) {
    try {
        $db = new DBManager();
        $sql = "
            SELECT MAX(SML_SentTimestamp) AS 'timestamp_check' 
            FROM gaig_users.sent_email 
            WHERE SML_UserId = ? AND SML_ProjectName = ?;";
        $bindings = array($this->id,$this->mostRecentProject);
        $data = $db->query($sql,$bindings);
        if($data->rowCount() > 0) {
            $result = $data->fetch();
/*
Why do you need to set date on object? If you are going to do this,
at least use more descriptive property name.
*/
            $this->date = date('Y-m-d',strtotime('-' . $periodInWeeks . ' weeks')) . ' 00:00:00';
            if($this->checkPeriod($this->date,$result['timestamp_check'])) {
                return true;
            }
/*
Free the query resource somewhere. You just fetch(), read out the only record
and then leave the resource open.
*/
/*
If you already had all of user's project information loaded into object,
then you could avoid this following query here.
*/
            $sql = "SELECT * FROM gaig_users.projects WHERE PRJ_ProjectId = ?;";
            $data = $db->query($sql,array($this->mostRecentProject));
/*
Ah so you are thinking about having Project be it's own class. Good.
But, why would you just not load all project info into user object at
instantiation if you feel you are always going to need to information?
You could lazy load as done here,but you are potentially allowing
caller to call against methods that don't have all appropriate dependencies
established.

This is only happy path here.
*/
            $mostRecentProj = new Project($data->fetch());
            $newComplexity = $templateConfig->getTemplateComplexityType();
            $newTarget = $templateConfig->getTemplateTargetType();
            if($this->checkMRP($mostRecentProj,$newComplexity,$newTarget)) {
                return false;
            }
            $data = $db->query($sql,array($this->previousProject));
            $previousProj = new Project($data->fetch());
            if($this->checkPP($mostRecentProj,$previousProj,$newComplexity)) {
                return false;
            }
            $data = $db->query($sql,array($this->lastProject));
            $lastProj = new Project($data->fetch());
            if($this->checkLP($mostRecentProj,$previousProj,$lastProj,$newTarget)) {
                return false;
            }
        }
        return true;
    } catch(Exception $e) {
        //unsure how to manage any exceptions thrown yet, if at all. further design to come
    }
}

/*
Kind of a trivial method here if all you are doing is comparing dates and
not doing any input validation or similar. Not sure that this method should
exist here unless it is used multiple places. I think you already have
input validation for these values further up call stack, so don't know
if it even makes sense to add that here. Put comparison in parenthesis.
return ($timestamp <= $date);
*/
private function checkPeriod($date,$timestamp) {
    return $timestamp <= $date;
}

/*
Do you need to think about name here?  What is MRP?

Seems off to validate a project here in User class.

Put comparison in parenthesis.
*/
private function checkMRP(Project $mrp, $complexity, $target) {
    return $complexity == $mrp->getTemplateComplexityType() &&
        $target == $mrp->getTemplateTargetType();
}

/*
Again, why is project validation in a user class?

Again, put comparison in parenthesis.
*/
private function checkPP(Project $mrp, Project $pp, $complexity) {
    return !is_null($pp) &&
        $complexity == $mrp->getTemplateComplexityType() &&
        $complexity == $pp->getTemplateComplexityType();
}

/*
Same comments as for checkPP()
*/
private function checkLP(Project $mrp, Project $pp, Project $lp, $target) {
    return !is_null($lp) &&
        !is_null($pp) &&
        $target == $mrp->getTemplateTargetType() &&
        $target == $pp->getTemplateTargetType() &&
        $target == $lp->getTemplateTargetType();
}

public function getLastName() {
    return $this->lastName;
}

public function getUsername() {
    return $this->username;
}

public function getUniqueURLId() {
    return $this->uniqueURLId;
}

public function getEmail() {
    return $this->email;
}

TemplateConfiguration Validation Function

/*
What class is this a method on? TemplateConfiguration class?
Why would this functionality exist in that class?
You might consider a class whose specific purpose is to build the list
of recipients given all necessary dependencies.

Pass DBManager dependency to class or method.
*/
public function getValidUsers($returnUsers, $periodInWeeks) {
    $db = new DBManager();
/*
Don't use SELECT *. It is a terrible habit.
*/
    $sql = "SELECT * FROM gaig_users.users;";
/*
Don't leak PDO implementation outside of DBManager.  Why does template
class need to know about how to instantiate user objects?
*/
    $users = $db->query($sql,array(),array('\PDO::ATTR_CURSOR'),array('\PDO::CURSOR_SCROLL'));
    $usersIterator = new PDOIterator($users);
    foreach($usersIterator as $user) {
        $tempUser = new User_Test($user);
/*
As noted in other code comments, this pushUser method is not working as you expect
*/
        $tempUser->pushUser($returnUsers,$periodInWeeks,$this);
    }
    return $returnUsers;
}

PhishingController Instantiation and Email Execution

/*
It seems like PhishingContrller is most logical place to do dependency
set up, as here you are reading request, instantiating templates, etc.
Perhaps this is place where you need to instantiate your additional
dependencies such as user collection.

I would suggest that perhaps you break this sendEmail method up. I don't
see a constructor for this class, but you might want to think about a
structure such as
public function __construct(Request $request) {
    // Handle the request.
    // This method could set dependencies on object such as
    // template and email config
    this.handleRequest($request);

    // instantiate DB connection
    // this controller can then pass this dependency to classes
    // functions it calls that need it
    this.db = new DBManager();

    // set time period. Right now you hard code this but perhaps it
    // should be passed to class or derived fromn request
     this.periodInWeeks = 4;

    // get user list
    this.users = ...; // some class/method that builds your user list
}

public function sendEmail() {
    // pass all dependencies here to static executeEmail method
    // of email class
    Email::executeEmail(
        this.emailConfig,
        this.templateConfig,
        this.users
    );
}
*/
public function sendEmail(Request $request) {
    try {
        $templateConfig = new TemplateConfiguration(
            array(
                'templateName'=>$request->input('emailTemplate'),
                'companyName'=>$request->input('companyName'),
                'projectName'=>$request->input('projectData')['projectName'],
                'projectId'=>intval($request->input('projectData')['projectId'])
            )
        );

        $periodInWeeks = 4;
        $users = array();
        $emailConfig = new EmailConfiguration(
            array(
                'host'=>$request->input('hostName'),
                'port'=>$request->input('port'),
                'authUsername'=>$request->input('username'),
                'authPassword'=>$request->input('password'),
                'fromEmail'=>$request->input('fromEmail'),
                'subject'=>$request->input('subject'),
                'users'=>$templateConfig->getValidUsers($users,$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

/*
I would try to pass the user list as argument to this method as noted above.

Maybe name should be executeEmailToUserList() or something more specific
*/
public static function executeEmail(
    EmailConfiguration $emailConfig,
    TemplateConfiguration $templateConfig)
{
    self::setTemplateConfig($templateConfig);
    self::setEmailConfig($emailConfig);


    try {
        foreach($emailConfig->getUsers() as $user) {
            self::sendEmail($user);
/*
I would suggest that updateUserProjects should be method on user class,
such that you would change this to:
$user->updateUserProjects()
*/
            self::updateUserProjects($user);
        }
    } catch(Exception $e) {
        throw new EmailException(__CLASS__ . ' Exception',0,$e);
    }
}

/*
Move this method to user class, as it has nothing to do with email sending.
Parameter passed to this method probably need to change so as to 
provide to this method the information in needs about the project. 
Perhaps pass Project object?
*/
private function updateUserProjects($user) {
    $db = new DBManager();
/*
Line is too long.  A suggestion on writing readable SQL might be to do:
$sql = "
    UPDATE gaig_users.users
    SET USR_ProjectMostRecent=?,
        USR_ProjectPrevious=?, 
        USR_ProjectLast=?
    WHERE USR_Username=?
";
I also find heredoc/nowdoc syntax helpful for more complex queries.
*/
    $sql = "UPDATE gaig_users.users SET USR_ProjectMostRecent=?, USR_ProjectPrevious=?, 
            USR_ProjectLast=? WHERE USR_Username=?;";
    $bindings = array(self::$templateConfig->getProjectName(),
        $user['USR_ProjectMostRecent'],
        $user['USR_ProjectPrevious'],
        $user['USR_Username']
    );
    $db->query($sql,$bindings);
}


private static function sendEmail($user) {    
    $templateData = array(
        'companyName'=>self::$templateConfig->getCompanyName(),
        'projectName'=>self::$templateConfig->getProjectName(),
        'projectId'=>self::$templateConfig->getProjectId(),
        'lastName'=>$user->getLastName(),
        'username'=>$user->getUsername(),
        'urlId'=>$user->getUniqueURLId()
    );
    $subject = self::$emailConfig->getSubject();
    $from = self::$emailConfig->getFromEmail();
/*
You getter name on user class might lead to some confusion here.
getEmailAddress?
*/
    $to = $user->getEmail();
    $mailResult = Mail::send(
        ['html' => self::$templateConfig->getTemplate()],
        $templateData,
        function($m) use ($from, $to, $subject) {
/*
Don't mix method chaining here. I would either chain them all (if possible)
or chain none of them.
*/
            $m->from($from);
            $m->to($to)
                ->subject($subject);
        }
    );
    if(!$mailResult) {
        throw new FailureException('Email failed to send to ' . $to . ', from ' . $from);
    }
}

/*
Probably need similar function here for setting user collection
*/
private static function setTemplateConfig(TemplateConfiguration $templateConfig) {
    self::$templateConfig = $templateConfig;
}

private static function setEmailConfig(EmailConfiguration $emailConfig) {
    self::$emailConfig = $emailConfig;
}

RandomObjectGeneration

class RandomObjectGeneration
{

/*
Consider placing default "keyspace" value as constant on class.
Method signature might then look like:
public static function random_str($length,
    $keyspace = RandomObjectGeneration::KEYSPACE)
*/
public static function random_str($length, $keyspace = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ')
{
/*
You should validate that $length is positive integer, not just numeric.
You don't want a values like 3.1415926, 0, -50, etc. passed in here.
These would all pass your current validation.
*/
    if(is_null($length) || !is_numeric($length)) {
/*
Give meaningful exception message.
*/
        throw new Exception();
    }
/*
Validate $keyspace as well. Non-zero length string as minimum validation.
*/
    $str = '';
    $max = mb_strlen($keyspace) - 1;
    for ($i = 0; $i < $length; ++$i) {
        $str .= $keyspace[random_int(0, $max)];
    }
    return $str;
}
}