New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CRM-19690 - CiviMail - Add MailingSystemTest. Small code cleanups. #9564

Merged

totten merged 6 commits into civicrm:master from totten:master-19690-cleanup

Dec 23, 2016

Member

totten commented Dec 20, 2016 •

edited

Loading

The main change here is to expand test-coverge of CiviMail's email
composition functionality via CRM_Mailing_MailingSystemTest and
CRM_Mailing_BaseMailingSystemTest. It improves coverage in some
areas which are conspicuously lacking coverage (e.g email tracking codes).

Additionally, this PR does some smaller cleanups to the BAO's -- for
details, check each commit separately.

Note: The new test is actually written as two classes, MailingSystemTest
and BaseMailingSystemTest. This is a bit unusual but serves a purpose
looking forward: under CRM-19690, we want to allow a replacement for
CiviMail's composition/delivery engine with a refactored version (aka
FlexMailer). The scenarios defined by BaseMailingSystemTest will be
shared by the two delivery engines to ensure that they behave the same way.

CRM-19690: Allow alternative email templating systems

totten added 4 commits

December 19, 2016 17:20


          CRM-19690 - MailingJob - Extract findPendingTasks().

453c7bb


          CRM-19690 - CRM_Utils_Token::getAnonymousTokenDetails() - Add missing…

ecd95cd

… `static`

This function is only called once, and it previously lacked test coverage,
so the `static` warning previously snuck through. Fix it!


          CRM-19690 - Mailing - Make getTemplates and getVerpAndUrlsAndHeaders …

926d841

…public

These two functions will be useful in FlexMailer's DefaultEngine.


          CRM-19690 - CRM_Mailing_MailingSystemTest - Improve CiviMail test cov…

b4a332a

…erage

civicrm-builder added the master label

totten mentioned this pull request

CRM-19690 - CiviMail/FlexMailer - Allow alternative email templating systems #9497

Closed

Member Author

totten commented Dec 20, 2016 •

edited

Loading

The test failure CRM_Contact_BAO_ContactType_ContactTest.testCRM19133 is a common false-negative.

Member Author

totten commented Dec 20, 2016

jenkins, test this please

Contributor

seamuslee001 commented Dec 20, 2016

Jenkins re test this please

omarabuhussein suggested changes

View reviewed changes

Member

omarabuhussein left a comment

left some notes

CRM/Mailing/BAO/MailingJob.php

@@ @@ -581,6 +530,7 @@ public function deliver(&$mailer, $testParams = NULL) { @@
                   // make sure that there's no more than $mailerBatchLimit mails processed in a run
                   $mailerBatchLimit = Civi::settings()->get('mailerBatchLimit');
+                  $eq = self::findPendingTasks($this->id, $mailing->sms_provider_id ? 'sms' : 'email');

Member

omarabuhussein Dec 22, 2016

can you do this in a variable then pass it :

self::findPendingTasks( .... , $mailProvider );

it is more cleaner

CRM/Mailing/BAO/MailingJob.php

+                 * @return \CRM_Mailing_Event_BAO_Queue
+                 *   A query object whose rows provide ('id', 'contact_id', 'hash') and ('email' or 'phone').
+                 */
+                public static function findPendingTasks($jobId, $medium) {

Member

omarabuhussein Dec 22, 2016

this is an implementation details ... is their any benefits from making it public ?

Member Author

totten Dec 22, 2016

Yeah, that's actually the main purpose of the commit. FlexMailer will want to use the exact same query -- which can be accomplished by pulling the query into its own public function.

Member

omarabuhussein Dec 22, 2016

oh I see .. the commit message doesn't say about that nor the PR description so I wasn't sure about it

CRM/Mailing/BAO/MailingJob.php

@@ @@ -581,6 +530,7 @@ public function deliver(&$mailer, $testParams = NULL) { @@
                   // make sure that there's no more than $mailerBatchLimit mails processed in a run
                   $mailerBatchLimit = Civi::settings()->get('mailerBatchLimit');
+                  $eq = self::findPendingTasks($this->id, $mailing->sms_provider_id ? 'sms' : 'email');
                   while ($eq->fetch()) {
                     // if ( ( $mailsProcessed % 100 ) == 0 ) {

Member

omarabuhussein Dec 22, 2016

not related to ur work , but can u please remove these commented out lines

CRM/Mailing/BAO/MailingJob.php

+                /**
+                 * @param int $jobId
+                 * @param string $medium
+                 *   Ex: 'email' or 'sms'.

Member

omarabuhussein Dec 22, 2016

there should be a space between @param & @return .. also could you add a description for this method

CRM/Mailing/BAO/MailingJob.php

+                  $eq = new CRM_Mailing_Event_BAO_Queue();
+                  $queueTable = CRM_Mailing_Event_BAO_Queue::getTableName();
+                  $emailTable = CRM_Core_BAO_Email::getTableName();
+                  $phoneTable = CRM_Core_DAO_Phone::getTableName();

Member

omarabuhussein Dec 22, 2016

I think this need to be consisitant .. CRM_Core_DAO_Phone should be changed to CRM_Core_BAO_Phone

tests/phpunit/CRM/Mailing/BaseMailingSystemTest.php

+               * @see \Civi\FlexMailer\FlexMailerSystemTest
+               * @see CRM_Mailing_MailingSystemTest
+               */
+              abstract class CRM_Mailing_BaseMailingSystemTest extends CiviUnitTestCase {

Member

omarabuhussein Dec 22, 2016

why this is an abstract class .. I see no need for that and also u don't have any abstract method in it

Member

omarabuhussein Dec 22, 2016

also same as in the other PR ... cannot use just use the headless interface for the tests

Member

omarabuhussein Dec 22, 2016 •

edited

Loading

forget about this comment above this one since we already discussed it

also same as in the other PR ... cannot use just use the headless interface for the tests

Member Author

totten Dec 22, 2016

It's true that no function is specifically abstract, so syntactically there's no obligation to mark the class abstract.

However, semantically, the class is abstract. There will be two subclasses (MailingSystemTest and FlexMailerSystemTest) which extend BaseMailingSystemTest. BaseMailingSystemTest does not do everything needed for a test. It's abstract because

We don't want phpunit to execute BaseMailingSystemTest by itself. (It's not a complete test.)
Subclasses are expected to make some changes -- by overriding setUp().

Member

omarabuhussein Dec 22, 2016 •

edited

Loading

hmm , sorry... but tbh I am not really convinced with this reasoning ...

We don't want phpunit to execute BaseMailingSystemTest by itself. (It's not a complete test.)

so you are defining it as abstract as a kind of protection from execution these test methods in the abstract class .. but I am looking at this class again and I think that you might do not need to add the word test before the method names .. so in this case you don't need to define the class as abstract.

Subclasses are expected to make some changes -- by overriding setUp().

but overriding methods is something and abstract class is something else , right ?

Member Author

totten Dec 22, 2016

Let's try this question again:

why this is an abstract class .. I see no need for that and also u don't have any abstract method in it

From a design perspective, the answer is: because BaseMailingSystemTest should not be instantiated by itself. The content within the file BaseMailingSystemTest.php is a collection of general test scenarios which should be independent of particular implementations (e.g. current CiviMail vs FlexMailer). It would be meaningless to execute those general test scenarios unless you provide more details about the specific implementation to test.

so you are defining it as abstract as a kind of protection from execution these test methods in the abstract class .. but I am looking at this class again and I think that you might do not need to add the word test before the method names .. so in this case you don't need to define the class as abstract.

From a functional perspective, it is desirable behavior that phpunit does not try to execute the BaseMailingSystemTest content by itself. That behavior could be achieved in multiple ways, but I don't see a point exploring those hypotheticals when there's no specific problem with the current concrete solution. FWIW, putting abstract on the class seems both simple and semantically accurate to me.

totten added 2 commits

December 22, 2016 13:39


          CRM-19690 - MailingJob::deliver() - Remove disabled debug code

2cef7e0


          CRM-19690 - MailingJob::findPendingTasks() - Tweak style

5f9481d

omarabuhussein approved these changes

View reviewed changes

Member

omarabuhussein commented Dec 23, 2016

things looks fine now @totten

totten merged commit 4e098b3 into civicrm:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels