Please Review Code

[aljawaid]

@creecros
https://github.com/aljawaid/KanboardEmailHistory/blob/61d9b914b32351efdfd4f92c501b65a0d75e9f09/Action/EmailTaskHistory.php

Please review to see if I did it all correctly:

• Add Default for Email Subject Add Default for Email Subject #6
• Use Project Name & Identifier in Email Subject Use Project Name & Identifier in Email Subject #7
• Add More Email Recipient Options Add More Email Recipient Options #5

[creecros]

you can get rid of the error_logs on lines 216,179, 143

I dont see where you accounted for assignee_project_email or creator_project_email so at first glance, i would suspect no email would be sent if those are the parameters.

[aljawaid]

you can get rid of the error_logs on lines 216,179, 143

• they show up in my cron log file so I will leave them for now as it's handy until i get it all working.

I dont see where you accounted for assignee_project_email or creator_project_email so at first glance, i would suspect no email would be sent if those are the parameters.

• I didnt as I thought it would be covered in if ($send_to == 'assignee' || $send_to == 'both' || $send_to == 'all') {... but I was waiting for your response, so I could ask.... should I change it to:
• 1. if ($send_to == 'assignee' || $send_to == 'both' || $send_to == 'all' || ($send_to == 'assignee' && $send_to == 'project_email')) {... to account for each one?
• 2. A new section as if ($send_to == 'assignee' && $send_to == 'project_email') {...

[creecros]

niether. Read your if statements, and ask yourself this question: "if $send_to ever equals assignee_project_email or creator_project_email what will the if statment do?". The answer is nothing, because you never accounted for it. I see assignee, both, all, project_email, thats it.

[aljawaid]

errm a hint on the right answer then? I'm baffled how to tackle it....

or do I just do: if ($send_to == 'assignee' || $send_to == 'both' || $send_to == 'all' || $send_to == 'project_email')) {... in the existing sections?

[creecros]

I'm a user. From this dropdown i choose Assignee & Project Email as my selection.

$send_to now equals assignee_project_email

your if statment:

if ($send_to == 'assignee' || $send_to == 'both' || $send_to == 'all' || $send_to == 'project_email'))

the answer to the question:
if ('assignee_project_email'== 'assignee' || 'assignee_project_email' == 'both' ||'assignee_project_email' == 'all' || 'assignee_project_email' == 'project_email'))

in this above example, 'assignee_project_email' will never == ANY of your conditions. Thus nothing will ever happen.

[creecros]

to better illustrate, let me put it in human language.

My variable equals assignee_project_email

if My variable equals assignee or both or all or project_email, i want you to send an email.

Do you think the computer will send that email? Answer, No. Why? Because I just told you My variable equals assignee_project_email.

How to fix? Add the question to the if statment.

I'd rather not just give you the answer. Or you wont learn.

Hints:
$send_to is the variable you are checking for the condition
|| represents an OR
&& represents an AND

given all that knowledge, these two possible variable values, are not accounted for:

[aljawaid]

I realised I dont need the && as it will use the assignee from the previous block, fire that and move on, until it hits the project_email part, then fire that... so in practicality, I would get an email to the assignee and an email to the project_email.

I think the && would be used if the IF was coded differently. Just explaining my thinking. I was wrong thinking of && for the current code.

[creecros]

correct, no &&. I only mentioned it because I wanted you to know what && was for your future self.

[creecros]

here is a simple example of an if statement:

$variable = 1;

if ($variable = 2) { 
 echo $variable;
}

The result of the above will be nothing.

If you can figure out why this result is nothing, then you have the answer you need to fix yours.

[aljawaid]

Does this solve my problem? 074aed8

[creecros]

now we are talking. good job. 👍

[aljawaid]

thanks, does the rest of the code seem to be correct now?

[creecros]

I'm on my phone, but will take a look later. Hard to read the full code on such little device.

[aljawaid]

The project email doesn't seem to be working, nothing in errors either. I even tried it just as project_email and nothing

[aljawaid]

I just fixed with c6e6dd2 but still the same error

the params file doesnt even show the placeholder.... let me try without it

I get the same error, if defaulting to your original code.... where have I gone wrong... it works for user/creator.... but nothing to do with the project email address... I've gone through the code a million times

[aljawaid]

568ec2a

[aljawaid]

Which one is correct, or are they both correct?
I think this one might be wrong...

KanboardEmailHistory/Action/EmailTaskHistory.php

Line 186 in 568ec2a

$project = $this->projectModel->getById($project_id);

this was there from the beginning and seems to work in the other sections for assignee and creator...

KanboardEmailHistory/Action/EmailTaskHistory.php

Line 193 in 568ec2a

'project' => $this->projectModel->getById($data['task']['project_id']),

The only other thing I can think of is this....

KanboardEmailHistory/Action/EmailTaskHistory.php

Line 200 in 568ec2a

$project['email'],

[aljawaid]

This is fixed in 1869adb ... the project_email works now

[aljawaid]

ya you did the same thing again and left that parameter out.

if ($send_to == 'creator' || $send_to == 'creator_project_email' || $send_to == 'both' || $send_to == 'all') {

you are now missing project_email from both conditionals.

fixed in bf399c5 .... I think!

[aljawaid]

@creecros hello, is everything okay? I have done everything and no errors so far and everything seems to work.... you been quiet last few days to all my comments but all of them are resolved now so I thought I would just checkup on you.

[creecros]

sorry, life kind of happens. just found out my company lost an RFP, which means I'm out of a job in 5 months. have not had the mentality to do much with kanboard.

[aljawaid]

whats an RFP? Sorry for your loss.. hope things get better soon. Set up the sponsor button on here, maybe it will work for you and generate income. I'm also looking into it but I'm no developer so can't do much.

[creecros]

RFP = Request for proposal, basically I work for an Agency for a specific client, every 3 years our agency sends an RFP to said client to renew the contract, and they did not renew.
I am also, not a developer. I am in the packaging industry.(Project Management Specifically)

[aljawaid]

oh i see