Skip to content

Commit 907944d

Browse files
authored
Merge pull request #275 from ArloSoftware/contact_hotfix
Fixingin loop for contact_job
2 parents b6789dd + 8d5ed80 commit 907944d

7 files changed

Lines changed: 339 additions & 3 deletions

File tree

classes/local/client.php

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,19 @@ class client {
3939
/** @var $httpclient \GuzzleHttp\Client */
4040
protected $httpclient;
4141

42+
/** @var mixed Guzzle handler override used only under PHPUnit. */
43+
protected static $testhandler = null;
44+
45+
/**
46+
* Inject a Guzzle handler (e.g. a MockHandler stack) for unit testing. Has no effect
47+
* outside PHPUnit. Pass null to reset.
48+
*
49+
* @param mixed $handler
50+
*/
51+
public static function set_test_handler($handler) {
52+
static::$testhandler = $handler;
53+
}
54+
4255
/**
4356
* Construct a guzzle client setup with basic authentication and appropriate
4457
* options and headers set.
@@ -62,6 +75,9 @@ public static function get_instance($headers = []) {
6275
]
6376
];
6477
$config['headers'] = array_merge($config['headers'], $headers);
78+
if (defined('PHPUNIT_TEST') && PHPUNIT_TEST && !is_null(static::$testhandler)) {
79+
$config['handler'] = static::$testhandler;
80+
}
6581
$client = new static();
6682
$client->httpclient = new \GuzzleHttp\Client($config);
6783
return $client;

classes/local/job/contacts_job.php

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -134,17 +134,34 @@ public function run() {
134134
$uri->setPagingTop(250);
135135
$uri->setResourcePath($jobpersistent->get('endpoint'));
136136
$uri->addExpand('Registration/Contact');
137-
$filter = "Contact/LastModifiedDateTime gt datetime('". $jobpersistent->get('lastsourcetimemodified') ."')";
137+
$lastsourcetimemodified = $jobpersistent->get('lastsourcetimemodified');
138+
// Page using a (LastModifiedDateTime, ContactID) keyset cursor. The ContactID
139+
// tiebreaker matches the other sync jobs and stops the cursor stalling when more
140+
// than one page of contacts share the same LastModifiedDateTime.
141+
$filter = "(Contact/LastModifiedDateTime gt datetime('" . $lastsourcetimemodified . "'))";
142+
if ($jobpersistent->get('lastsourceid')) {
143+
$filter .= " OR (Contact/LastModifiedDateTime eq datetime('" . $lastsourcetimemodified . "')";
144+
$filter .= " AND Contact/ContactID gt " . $jobpersistent->get('lastsourceid') . ")";
145+
}
138146
$uri->setFilterBy($filter);
139-
$uri->setOrderBy('Contact/LastModifiedDateTime ASC');
147+
$uri->setOrderBy('Contact/LastModifiedDateTime ASC,Contact/ContactID ASC');
140148
$request = new Request('GET', $uri->output(true));
141149
$response = client::get_instance()->send_request($request);
142150
$collection = response_processor::process($response);
143151
if ($collection->count() > 0) {
152+
$cursortimebefore = $jobpersistent->get('lastsourcetimemodified');
153+
$cursoridbefore = $jobpersistent->get('lastsourceid');
144154
foreach ($collection as $resource) {
145155
try {
146-
// No need to process cancelled registrations.
156+
// No need to process cancelled registrations, but still advance the
157+
// paging cursor so a page made up entirely of cancelled registrations
158+
// does not stall pagination and re-request the same page forever.
147159
if ($resource->Status == RegistrationStatus::CANCELLED) {
160+
$cancelledcontact = $resource->getContact();
161+
if (!empty($cancelledcontact) && !empty($cancelledcontact->LastModifiedDateTime)) {
162+
$jobpersistent->set('lastsourceid', $cancelledcontact->ContactID);
163+
$jobpersistent->set('lastsourcetimemodified', $cancelledcontact->LastModifiedDateTime);
164+
}
148165
continue;
149166
}
150167
$contactresource = $resource->getContact();
@@ -218,6 +235,15 @@ public function run() {
218235
}
219236
// See if need to get another page of records.
220237
$hasnext = (bool) $collection->hasNext();
238+
// Safety net: if Arlo reports more pages but our paging cursor did not move
239+
// while processing this page, stop instead of re-requesting the identical page
240+
// forever (prevents runaway polling of the Arlo API).
241+
if ($hasnext
242+
&& $jobpersistent->get('lastsourcetimemodified') === $cursortimebefore
243+
&& $jobpersistent->get('lastsourceid') == $cursoridbefore) {
244+
$this->add_error(get_string('pagingnoprogress', 'enrol_arlo'));
245+
$hasnext = false;
246+
}
221247
}
222248
}
223249
return true;

lang/en/enrol_arlo.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -331,6 +331,7 @@
331331
$string['contactresourcemissing'] = 'Contact resource missing from Registration.';
332332
$string['contactrecordmissing'] = 'Contact record missing.';
333333
$string['noassociateduser'] = 'No associated Moodle user account.';
334+
$string['pagingnoprogress'] = 'Paging halted: the sync cursor did not advance while more pages were reported. Stopped to avoid re-requesting the same page.';
334335
$string['unsuccessfulenrolment'] = 'Unsuccessful enrolment';
335336
$string['unsuccessfulenrolments'] = 'Unsuccessful enrolments';
336337
$string['unsuccessfulenrolmentscount'] = 'Unsuccessful enrolments: {$a}';

tests/contacts_job_test.php

Lines changed: 177 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,177 @@
1+
<?php
2+
// This file is part of Moodle - http://moodle.org/
3+
//
4+
// Moodle is free software: you can redistribute it and/or modify
5+
// it under the terms of the GNU General Public License as published by
6+
// the Free Software Foundation, either version 3 of the License, or
7+
// (at your option) any later version.
8+
//
9+
// Moodle is distributed in the hope that it will be useful,
10+
// but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
12+
// GNU General Public License for more details.
13+
//
14+
// You should have received a copy of the GNU General Public License
15+
// along with Moodle. If not, see <http://www.gnu.org/licenses/>.
16+
17+
/**
18+
* Tests for contacts_job pagination.
19+
*
20+
* Regression coverage for the Batalas incident: a page of registrations that are all
21+
* Cancelled used to leave the paging cursor (lastsourcetimemodified) unchanged while the
22+
* Arlo response still advertised a "next" page, so contacts_job::run() re-requested the
23+
* identical page forever, flooding the Arlo API.
24+
*
25+
* @package enrol_arlo
26+
* @category phpunit
27+
* @copyright 2026 Arlo
28+
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
29+
*/
30+
31+
defined('MOODLE_INTERNAL') || die();
32+
33+
use enrol_arlo\local\client;
34+
use enrol_arlo\local\job\contacts_job;
35+
use enrol_arlo\local\persistent\job_persistent;
36+
use GuzzleHttp\Handler\MockHandler;
37+
use GuzzleHttp\HandlerStack;
38+
use GuzzleHttp\Middleware;
39+
use GuzzleHttp\Psr7\Response;
40+
41+
class contacts_job_test extends advanced_testcase {
42+
43+
/** @var enrol_arlo_generator $plugingenerator handle to plugin generator. */
44+
protected $plugingenerator;
45+
46+
/** @var array captured Guzzle request/response history for the current test. */
47+
protected $history = [];
48+
49+
/**
50+
* Test setup.
51+
*/
52+
public function setUp(): void {
53+
global $CFG;
54+
parent::setUp();
55+
require_once($CFG->dirroot . '/enrol/arlo/lib.php');
56+
$this->resetAfterTest();
57+
$this->plugingenerator = $this->getDataGenerator()->get_plugin_generator('enrol_arlo');
58+
$this->plugingenerator->enable_plugin();
59+
$this->plugingenerator->setup_plugin();
60+
}
61+
62+
/**
63+
* Reset the injected HTTP handler so static state does not leak between tests.
64+
*/
65+
public function tearDown(): void {
66+
client::set_test_handler(null);
67+
$this->history = [];
68+
parent::tearDown();
69+
}
70+
71+
/**
72+
* Build a course with an Arlo Event enrolment instance.
73+
*
74+
* @return stdClass the enrolment instance record.
75+
*/
76+
protected function create_arlo_instance() {
77+
$course = $this->getDataGenerator()->create_course();
78+
$template = $this->plugingenerator->create_event_template();
79+
$event = $this->plugingenerator->create_event($template);
80+
return $this->plugingenerator->create_event_enrolment_instance($course, $event);
81+
}
82+
83+
/**
84+
* Fetch the auto-registered contacts job for an enrolment instance.
85+
*
86+
* @param int $instanceid
87+
* @return job_persistent
88+
*/
89+
protected function get_contacts_job($instanceid) {
90+
return job_persistent::get_record(
91+
['area' => 'enrolment', 'type' => 'contacts', 'instanceid' => $instanceid]
92+
);
93+
}
94+
95+
/**
96+
* Queue a sequence of fixture files as Arlo XML responses and route the client through them.
97+
*
98+
* @param array $fixtures fixture filenames under tests/fixtures/
99+
*/
100+
protected function mock_arlo_responses(array $fixtures) {
101+
global $CFG;
102+
$responses = [];
103+
foreach ($fixtures as $fixture) {
104+
$xml = file_get_contents($CFG->dirroot . '/enrol/arlo/tests/fixtures/' . $fixture);
105+
$responses[] = new Response(200, ['Content-Type' => 'application/xml'], $xml);
106+
}
107+
$stack = HandlerStack::create(new MockHandler($responses));
108+
$this->history = [];
109+
$stack->push(Middleware::history($this->history));
110+
client::set_test_handler($stack);
111+
}
112+
113+
/**
114+
* A full page of Cancelled registrations must still advance the cursor and stop paging,
115+
* rather than re-requesting the same page forever.
116+
*/
117+
public function test_all_cancelled_page_advances_cursor_and_stops_paging(): void {
118+
$instance = $this->create_arlo_instance();
119+
$job = $this->get_contacts_job($instance->id);
120+
121+
// Sanity: the cursor starts at the epoch default that the flood was stuck on.
122+
$this->assertSame('1970-01-01T00:00:00Z', $job->get('lastsourcetimemodified'));
123+
$this->assertEquals(0, $job->get('lastsourceid'));
124+
125+
// Page 1: 3 Cancelled registrations + a "next" link. Page 2: empty (terminates).
126+
$this->mock_arlo_responses([
127+
'registrations_cancelled_page.xml',
128+
'registrations_empty_page.xml',
129+
]);
130+
131+
$result = (new contacts_job($job))->run();
132+
$this->assertTrue($result);
133+
134+
// Exactly two requests: page 1, then page 2 - NOT an unbounded re-request loop.
135+
$this->assertCount(2, $this->history,
136+
'contacts_job should page once and stop; more requests means the cursor stalled.');
137+
138+
// The cursor advanced past the cancelled page to the last contact on it.
139+
$job->read();
140+
$this->assertSame('2025-07-17T15:39:56.2075043Z', $job->get('lastsourcetimemodified'));
141+
$this->assertEquals(2003, $job->get('lastsourceid'));
142+
143+
// The second request used the advanced keyset cursor (incl. the ContactID tiebreaker).
144+
$secondfilter = urldecode((string) $this->history[1]['request']->getUri());
145+
$this->assertStringContainsString("gt datetime('2025-07-17T15:39:56.2075043Z')", $secondfilter);
146+
$this->assertStringContainsString('Contact/ContactID gt 2003', $secondfilter);
147+
}
148+
149+
/**
150+
* If the cursor genuinely cannot advance (e.g. contacts with no LastModifiedDateTime) while
151+
* the API still reports more pages, the safety net must stop paging instead of looping.
152+
*/
153+
public function test_paging_halts_when_cursor_cannot_advance(): void {
154+
$instance = $this->create_arlo_instance();
155+
$job = $this->get_contacts_job($instance->id);
156+
157+
// Queue the same non-advancing page twice: with the guard only ONE is ever requested.
158+
$this->mock_arlo_responses([
159+
'registrations_cancelled_no_modified_page.xml',
160+
'registrations_cancelled_no_modified_page.xml',
161+
]);
162+
163+
$contactsjob = new contacts_job($job);
164+
$contactsjob->run();
165+
166+
// The guard stopped after a single request rather than re-fetching the identical page.
167+
$this->assertCount(1, $this->history,
168+
'A non-advancing page with a next link must not be requested more than once.');
169+
170+
// Cursor untouched, and the no-progress reason was recorded.
171+
$job->read();
172+
$this->assertSame('1970-01-01T00:00:00Z', $job->get('lastsourcetimemodified'));
173+
$this->assertEquals(0, $job->get('lastsourceid'));
174+
$this->assertTrue($contactsjob->has_errors());
175+
$this->assertContains(get_string('pagingnoprogress', 'enrol_arlo'), $contactsjob->get_errors());
176+
}
177+
}
Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
<Registrations>
2+
<Link rel="http://schemas.arlo.co/api/2012/02/auth/Registration" type="application/xml" title="Registration" href="https://example.arlo.co/api/2012-02-01/auth/resources/registrations/1101/">
3+
<Registration>
4+
<RegistrationID>1101</RegistrationID>
5+
<UniqueIdentifier>00000000-0000-0000-0000-000000001101</UniqueIdentifier>
6+
<Status>Cancelled</Status>
7+
<CreatedDateTime>2023-02-01T09:00:00.0000000Z</CreatedDateTime>
8+
<LastModifiedDateTime>2023-02-10T09:00:00.0000000Z</LastModifiedDateTime>
9+
<Link rel="http://schemas.arlo.co/api/2012/02/auth/related/Contact" type="application/xml" title="Contact" href="https://example.arlo.co/api/2012-02-01/auth/resources/contacts/2101/">
10+
<Contact>
11+
<ContactID>2101</ContactID>
12+
<UniqueIdentifier>00000000-0000-0000-0000-000000002101</UniqueIdentifier>
13+
<FirstName>Test</FirstName>
14+
<LastName>Nolmdone</LastName>
15+
<Email>nolmd1@example.com</Email>
16+
<Status>Active</Status>
17+
</Contact>
18+
</Link>
19+
</Registration>
20+
</Link>
21+
<Link rel="http://schemas.arlo.co/api/2012/02/auth/Registration" type="application/xml" title="Registration" href="https://example.arlo.co/api/2012-02-01/auth/resources/registrations/1102/">
22+
<Registration>
23+
<RegistrationID>1102</RegistrationID>
24+
<UniqueIdentifier>00000000-0000-0000-0000-000000001102</UniqueIdentifier>
25+
<Status>Cancelled</Status>
26+
<CreatedDateTime>2023-03-01T09:00:00.0000000Z</CreatedDateTime>
27+
<LastModifiedDateTime>2023-03-10T09:00:00.0000000Z</LastModifiedDateTime>
28+
<Link rel="http://schemas.arlo.co/api/2012/02/auth/related/Contact" type="application/xml" title="Contact" href="https://example.arlo.co/api/2012-02-01/auth/resources/contacts/2102/">
29+
<Contact>
30+
<ContactID>2102</ContactID>
31+
<UniqueIdentifier>00000000-0000-0000-0000-000000002102</UniqueIdentifier>
32+
<FirstName>Test</FirstName>
33+
<LastName>Nolmdtwo</LastName>
34+
<Email>nolmd2@example.com</Email>
35+
<Status>Active</Status>
36+
</Contact>
37+
</Link>
38+
</Registration>
39+
</Link>
40+
<Link rel="next" type="application/xml" href="https://example.arlo.co/api/2012-02-01/auth/resources/onlineactivities/36/registrations/?top=250&amp;expand=Registration,Registration/Contact"/>
41+
</Registrations>
Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
<Registrations>
2+
<Link rel="http://schemas.arlo.co/api/2012/02/auth/Registration" type="application/xml" title="Registration" href="https://example.arlo.co/api/2012-02-01/auth/resources/registrations/1001/">
3+
<Registration>
4+
<RegistrationID>1001</RegistrationID>
5+
<UniqueIdentifier>00000000-0000-0000-0000-000000001001</UniqueIdentifier>
6+
<Status>Cancelled</Status>
7+
<CreatedDateTime>2023-02-01T09:00:00.0000000Z</CreatedDateTime>
8+
<LastModifiedDateTime>2023-02-10T09:00:00.0000000Z</LastModifiedDateTime>
9+
<Link rel="self" type="application/xml" href="https://example.arlo.co/api/2012-02-01/auth/resources/registrations/1001/"/>
10+
<Link rel="http://schemas.arlo.co/api/2012/02/auth/related/Contact" type="application/xml" title="Contact" href="https://example.arlo.co/api/2012-02-01/auth/resources/contacts/2001/">
11+
<Contact>
12+
<ContactID>2001</ContactID>
13+
<UniqueIdentifier>00000000-0000-0000-0000-000000002001</UniqueIdentifier>
14+
<FirstName>Test</FirstName>
15+
<LastName>Learnerone</LastName>
16+
<Email>learner1@example.com</Email>
17+
<Status>Active</Status>
18+
<CreatedDateTime>2022-11-01T10:00:00.0000000Z</CreatedDateTime>
19+
<LastModifiedDateTime>2023-03-07T13:11:09.9601334Z</LastModifiedDateTime>
20+
<Link rel="self" type="application/xml" href="https://example.arlo.co/api/2012-02-01/auth/resources/contacts/2001/"/>
21+
</Contact>
22+
</Link>
23+
</Registration>
24+
</Link>
25+
<Link rel="http://schemas.arlo.co/api/2012/02/auth/Registration" type="application/xml" title="Registration" href="https://example.arlo.co/api/2012-02-01/auth/resources/registrations/1002/">
26+
<Registration>
27+
<RegistrationID>1002</RegistrationID>
28+
<UniqueIdentifier>00000000-0000-0000-0000-000000001002</UniqueIdentifier>
29+
<Status>Cancelled</Status>
30+
<CreatedDateTime>2023-04-01T09:00:00.0000000Z</CreatedDateTime>
31+
<LastModifiedDateTime>2023-04-10T09:00:00.0000000Z</LastModifiedDateTime>
32+
<Link rel="self" type="application/xml" href="https://example.arlo.co/api/2012-02-01/auth/resources/registrations/1002/"/>
33+
<Link rel="http://schemas.arlo.co/api/2012/02/auth/related/Contact" type="application/xml" title="Contact" href="https://example.arlo.co/api/2012-02-01/auth/resources/contacts/2002/">
34+
<Contact>
35+
<ContactID>2002</ContactID>
36+
<UniqueIdentifier>00000000-0000-0000-0000-000000002002</UniqueIdentifier>
37+
<FirstName>Test</FirstName>
38+
<LastName>Learnertwo</LastName>
39+
<Email>learner2@example.com</Email>
40+
<Status>Active</Status>
41+
<CreatedDateTime>2022-12-01T10:00:00.0000000Z</CreatedDateTime>
42+
<LastModifiedDateTime>2024-01-15T08:30:00.0000000Z</LastModifiedDateTime>
43+
<Link rel="self" type="application/xml" href="https://example.arlo.co/api/2012-02-01/auth/resources/contacts/2002/"/>
44+
</Contact>
45+
</Link>
46+
</Registration>
47+
</Link>
48+
<Link rel="http://schemas.arlo.co/api/2012/02/auth/Registration" type="application/xml" title="Registration" href="https://example.arlo.co/api/2012-02-01/auth/resources/registrations/1003/">
49+
<Registration>
50+
<RegistrationID>1003</RegistrationID>
51+
<UniqueIdentifier>00000000-0000-0000-0000-000000001003</UniqueIdentifier>
52+
<Status>Cancelled</Status>
53+
<CreatedDateTime>2025-06-01T09:00:00.0000000Z</CreatedDateTime>
54+
<LastModifiedDateTime>2025-06-10T09:00:00.0000000Z</LastModifiedDateTime>
55+
<Link rel="self" type="application/xml" href="https://example.arlo.co/api/2012-02-01/auth/resources/registrations/1003/"/>
56+
<Link rel="http://schemas.arlo.co/api/2012/02/auth/related/Contact" type="application/xml" title="Contact" href="https://example.arlo.co/api/2012-02-01/auth/resources/contacts/2003/">
57+
<Contact>
58+
<ContactID>2003</ContactID>
59+
<UniqueIdentifier>00000000-0000-0000-0000-000000002003</UniqueIdentifier>
60+
<FirstName>Test</FirstName>
61+
<LastName>Learnerthree</LastName>
62+
<Email>learner3@example.com</Email>
63+
<Status>Active</Status>
64+
<CreatedDateTime>2023-01-01T10:00:00.0000000Z</CreatedDateTime>
65+
<LastModifiedDateTime>2025-07-17T15:39:56.2075043Z</LastModifiedDateTime>
66+
<Link rel="self" type="application/xml" href="https://example.arlo.co/api/2012-02-01/auth/resources/contacts/2003/"/>
67+
</Contact>
68+
</Link>
69+
</Registration>
70+
</Link>
71+
<Link rel="next" type="application/xml" href="https://example.arlo.co/api/2012-02-01/auth/resources/onlineactivities/36/registrations/?top=250&amp;expand=Registration,Registration/Contact"/>
72+
</Registrations>
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
<Registrations>
2+
<Link rel="self" type="application/xml" href="https://example.arlo.co/api/2012-02-01/auth/resources/onlineactivities/36/registrations/"/>
3+
</Registrations>

0 commit comments

Comments
 (0)