Skip to content

Improve UI/UX for Terms and Conditions Review (CFM-501) #419

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
+354 −296
Open

Improve UI/UX for Terms and Conditions Review (CFM-501) #419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4fc83b9
Improve UI/UX for Terms and Conditions Review (CFM-501)
arlen Jun 15, 2026
39d02d3
Move common code and language strings from TermsAgreer plugin to core…
arlen Jun 18, 2026
fb48d8e
Further cleanup to Terms and Conditions (CFM-501)
arlen Jun 26, 2026
36d3b7f
Fix preview link to TandC Mostly Static Page (CFM-501)
arlen Jun 26, 2026
44504dc
Add JavaScript nonce to TandC script tag (CFM-501)
arlen Jun 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions app/plugins/TermsAgreer/resources/locales/en_US/terms_agreer.po
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,21 +37,9 @@ msgstr "Ignore"
msgid "enumeration.TAndCEnrollmentModeEnum.IC"
msgstr "Implied Consent"

msgid "error.TAndCAgreement.missing"
msgstr "Did not receive agreement for \"{0}\" (T&C {1})"

msgid "field.AgreementCollectors.t_and_c_mode"
msgstr "Terms and Conditions Mode"

msgid "information.AgreementCollectors.external"
msgstr "These Terms and Conditions will be loaded in an external browser window. After review, you must return to this window and click \"Agree\" to continue."

msgid "information.AgreementCollectors.review"
msgstr "You must review and agree to these Terms and Conditions before continuing."

msgid "information.AgreementCollectors.review.tc"
msgstr "Review Terms & Conditions"

msgid "result.AgreementCollectors.ignored"
msgstr "Terms and Conditions collection disabled"

Expand Down
2 changes: 1 addition & 1 deletion app/plugins/TermsAgreer/src/Controller/AgreementCollectorsController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ public function dispatch(string $id) {
if(!isset($data[$key]) || $data[$key] != "1") {
$ok = false;

$this->Flash->error(__d('terms_agreer','error.TAndCAgreement.missing', [$tc->description, $tc->id]));
$this->Flash->error(__d('error','TermsAndConditions.agreement.missing', [$tc->description, $tc->id]));
}
}

Expand Down
170 changes: 3 additions & 167 deletions app/plugins/TermsAgreer/templates/AgreementCollectors/dispatch.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?php
/**
* COmanage Registry T&C Agreement Colectors Petition Fields
* COmanage Registry T&C Agreement Collectors Petition Fields
*
* Portions licensed to the University Corporation for Advanced Internet
* Development, Inc. ("UCAID") under one or more contributor license agreements.
Expand All @@ -25,169 +25,5 @@
* @license Apache License, Version 2.0 (http://www.apache.org/licenses/LICENSE-2.0)
*/

declare(strict_types = 1);

use \TermsAgreer\Lib\Enum\TAndCEnrollmentModeEnum;

print $this->element('flash', []);

// Make the Form fields editable
$this->Field->enableFormEditMode();
?>

<p><?= __d('terms_agreer', 'information.AgreementCollectors.review') ?></p>

<?php

// Begin the form
print $this->Form->create(null, [
'id' => 'agreement-form',
'type' => 'post'
]);


?>

<table>
<tr>
<th><?= __d('controller','TermsAndConditions',[99]) ?></th>
<th class="center"><?= __d('operation','review') ?></th>
<th><?= __d('operation','agreement') ?></th>
</tr>

<?php foreach($vv_tandc as $tc): ?>
<tr class="<?= !empty($tc->url) ? 'tc-url' : 'tc-msp' ?>">
<td>
<?= $tc['description'] ?>
</td>
<td class="center">
<button
data-tc-id="tc<?= $tc['id'] ?>"
class="btn btn-primary btn-sm tc-review-button"
type="button">
<?= __d('operation','review') ?>
</button>
<?php if(!empty($tc->url)): // We have a URL based T&C ?>
<button
data-tc-id="tc<?= $tc['id'] ?>"
id="tc<?= $tc['id'] ?>-window-launcher"
class="tc-window-launcher invisible"
data-tc-url="<?= $tc->url ?>"
type="button">
<?= __d('operation','review') . ' tc' . $tc['id'] ?>
</button>
<?php else: // We have a Mostly Static Page based T&C ?>
<?= $this->element('TermsAgreer.agreeDialog', ['vv_tc' => $tc]); ?>
<?php endif; ?>
</td>
<td>
<div class="form-check tc-agree">
<?=
$this->Form->checkbox(
'tc'.$tc['id'],
['id' => 'tc'.$tc['id'], 'class' => 'form-check-input tc-agree-checkbox']
) .
$this->Form->label(
'tc'.$tc['id'],
__d('operation','agree'),
['class' => 'form-check-label']
)
?>
</div>
</td>
</tr>
<?php endforeach; ?>

</table>


<script nonce="<?= $vv_js_nonce ?>">
// Iterate over our T&Cs to see if they're all checked.
function checkAgreements(mode) {
let allAgreed = true;
$('.tc-agree-checkbox').each(function() {
if(!$(this).prop('checked')) {
allAgreed = false;
if(mode == 'EC') {
// We're in Explicit Consent mode, so disable
// the current checkbox; its T&C must be reviewed.
$(this).prop('disabled', true);
}
}
});

// Enable / disable the "Continue" button.
if(allAgreed) {
$('.submit input[type="submit"]').prop('disabled', false);
} else {
$('.submit input[type="submit"]').prop('disabled', true);
}
}

// On page load: check agreement states and attach event handlers.
$(function() {
// Explicit Consent mode ("EC") will disable "I Agree" checkboxes until review.
const tcmode = '<?= $vv_tandc_mode ?>';

// Run on first load.
checkAgreements(tcmode);

// Handle "Review" button for URL-based terms and conditions.
$('tr.tc-url button.tc-review-button').click(function(e) {
e.preventDefault();
jsConfirmGeneric(
'<?= __d('terms_agreer', 'information.AgreementCollectors.external') ?>',
'',
$(this).attr('data-tc-id')+'-window-launcher',
'<?= __d('operation','continue') ?>',
'<?= __d('operation','cancel') ?>',
'<?= __d('terms_agreer','information.AgreementCollectors.review.tc') ?>'
);
});

// Launch the external window for URL-based terms and conditions
$('tr.tc-url button.tc-window-launcher').click(function(e) {
e.preventDefault();

// Calculate window size based on screen size so that we
// can handle mobile and to center the window.
const winWidth = window.outerWidth / 1.5;
const winHeight = window.outerHeight / 1.5;
const left = window.screenLeft + ((window.outerWidth - winWidth) / 2);
const top = window.screenTop + ((window.outerHeight - winHeight) / 2);

// Launch the T&C window with the external URL
let tandcExternalWindow = window.open(
$(this).attr('data-tc-url'),
'tandcExternalWindow',
'width=' + winWidth + ',height=' + winHeight + ',left=' + left + ',top=' + top +
',resizable=yes,scrollbars=yes,toolbar=no,menubar=no,location=no,status=no'
);

// Enable the T&C checkbox
$('#'+$(this).attr('data-tc-id')).prop('disabled', false);
});

// Handle "Review" button for Mostly Static Page-based terms and conditions.
// Clicking this will reveal the T&C agree dialog.
$('tr.tc-msp button.tc-review-button').click(function(e) {
e.preventDefault();
$('#'+$(this).attr('data-tc-id')+'-agree-dialog').modal('show');
});

// Handle "Agree" button click from MSP Dialog
$('.tc-agree-button').click(function(e) {
// Enable and check the T&C checkbox
$('#'+$(this).attr('data-tc-id'))
.prop('checked', true)
.prop('disabled', false)
.trigger('change');
});

// Check agreements when a checkbox is clicked.
// If all are checked, we'll enable "Continue".
$('.tc-agree-checkbox').change(function() {
checkAgreements(tcmode);
});
});
</script>
// This dispatch uses the same user interface as the core TermsAndConditions/tandcs.inc template.
include ROOT . DS . 'templates' . DS . 'TermsAndConditions' . DS . 'tandcs.inc';
3 changes: 3 additions & 0 deletions app/resources/locales/en_US/error.po
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,9 @@ msgstr "No outgoing SMTP Server configuration found"
msgid "TermsAndConditions.document.one"
msgstr "Exactly one of Mostly Static Page or URL must be specified"

msgid "TermsAndConditions.agreement.missing"
msgstr "Did not receive agreement for \"{0}\" (T&C {1})"

msgid "TermsAndConditions.revoke.none"
msgstr "No Agreements available to revoke"

Expand Down
9 changes: 9 additions & 0 deletions app/resources/locales/en_US/information.po
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,15 @@ msgstr "Report for "
msgid "table.list"
msgstr "{0} List"

msgid "tandc.review"
msgstr "You must review and agree to these Terms and Conditions before continuing."

msgid "tandc.review.dialog.title"
msgstr "Review Terms & Conditions"

msgid "tandc.review.external"
msgstr "These Terms and Conditions will be loaded in an external browser window. After review, you must return to this window and click \"Agree\" to continue."

msgid "ug.tasks.post"
msgstr "Executing post-database tasks for version {0}"

Expand Down
67 changes: 55 additions & 12 deletions app/src/Controller/TermsAndConditionsController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,8 @@ public function review() {
// Before we get started, see if a return parameter was requested, and if so if it is permitted.
// If so, we'll override any current return URL. We check the return URL here rather than when
// we're done because URLs stored by AppController are not subject to the allow list check.

$returnUrl = $this->request->getQuery('return');
$request = $this->getRequest();
$returnUrl = $request->getQuery('return');

if(!empty($returnUrl)) {
$returnUrl = base64_decode($returnUrl);
Expand All @@ -160,7 +160,7 @@ public function review() {
if(preg_match($u, $returnUrl)) {
// The requested URL is permitted, so store it in the session, potentially overriding
// the original return URL
$this->request->getSession()->write('TAndC.return', $returnUrl);
$request->getSession()->write('TAndC.return', $returnUrl);
break;
}
}
Expand All @@ -181,23 +181,66 @@ public function review() {

$status = $this->TermsAndConditions->status((int)$personId);

// If there is nothing left to do, redirect to the original request
$done = true;
if($request->is('post')) {
// Walk the set of $status['tandc']s and look for an agreement in the POST data.
$data = $request->getData();

$ok = true;

foreach($status as $s) {
// The post data is keyed on the string "tc" appended to the T&C id,
// and the expected value is "1".

$key = "tc".$s['tandc']->id;

if(!isset($data[$key]) || $data[$key] != "1") {
$ok = false;

foreach($status as $s) {
if($s['status'] != TAndCStatusEnum::Agreed) {
$done = false;
break;
$this->Flash->error(__d('error','TermsAndConditions.agreement.missing', [$s['tandc']->description, $key]));
}
}
}

if($done) {
return $this->redirect($this->request->getSession()->read('TAndC.return'));
if($ok) {
foreach($status as $s) {
try {
$this->TermsAndConditions->TAndCAgreements->record(
termsAndConditionsId: (int)$s['tandc']->id,
personId: $personId,
actorPersonId: $personId,
identifier: $this->getRequest()->getSession()->read('Auth.external.user')
);

// Request provisioning on success

$this->llog('trace', "Requesting provisioning after T&C Agreement for Person " . $personId);

$People = TableRegistry::getTableLocator()->get('People');

$People->requestProvisioning(
id: (int)$personId,
context: ProvisioningContextEnum::Automatic
);
} catch(\Exception $e) {
$this->Flash->error($e->getMessage());
return $this->generateRedirect(null);
}
}

// Set the flash message
$this->Flash->success(__d('result', 'TermsAndConditions.recorded'));

// redirect to the original request
return $this->redirect($request->getSession()->read('TAndC.return'));
}
}

$this->set('vv_tandc_statuses', $status);
$this->set('vv_person_id', (int)$personId);

// The default mode for CO and platform wide T&Cs is "Explicit Consent".
// There is no enumeration for this in core, so set the value explicitly to 'EC'.
$this->set('vv_tandc_mode', 'EC');

$this->set('vv_title', __d('controller', 'TermsAndConditions', 99));
}

Expand Down
20 changes: 11 additions & 9 deletions app/src/Model/Table/TermsAndConditionsTable.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ public function initialize(array $config): void {
$this->setAllowLookupPrimaryLink(['agree', 'proxy', 'revoke']);
$this->setAllowLookupRelatedPrimaryLink(['status' => ['person_id']]);
$this->setAllowUnkeyedPrimaryLink(['review']);
$this->setAllowEmptyPrimaryLink(['review']);
Copy link
Contributor

@benno benno Jun 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I missed this before -- under what circumstances are we allowing the Primary Link to be empty? ie, why are we calling the review action without a CO?

Copy link
Contributor

@benno benno Jul 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Slack, you said "I added that originally because without it I get the runtime exception “Could not find value for Primary Link” at the point at which I attempt to save the T&Cs (e.g. at /registry-pe/terms-and-conditions/review?co_id=2"

The Primary Link (co_id) is provided, so "Allow Empty" is not right. This should be handled by setAllowUnkeyedPrimaryLink instead. However, you say "I attempt to save the T&Cs", which implies a POST of some form that is dropping the co_id from the submission. I'll need to reread your PR to see what's going on, but setAllowEmptyPrimaryLink is almost certainly not the right approach.

Copy link
Contributor Author

@arlen arlen Jul 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah - let me take a look at the post. Maybe the co_id is getting lost.


$this->setIndexContains([
'MostlyStaticPages'
Expand Down Expand Up @@ -179,25 +180,26 @@ public function status(int $personId): array {

// Pull active T&C for the CO/COU $copersonid is a member of. This will NOT include
// any outdated T&C, we'll pull those separately, below.

$whereClause = [
'co_id' => $person->co_id,
'status' => SuspendableStatusEnum::Active
'TermsAndConditions.co_id' => $person->co_id,
'TermsAndConditions.status' => SuspendableStatusEnum::Active
];

if(!empty($couIds)) {
$whereClause['OR'] = [
'cou_id IS NULL',
'cou_id IN' => array_values($couIds)
'TermsAndConditions.cou_id IS NULL',
'TermsAndConditions.cou_id IN' => array_values($couIds)
];
} else {
$whereClause[] = 'cou_id IS NULL';
$whereClause[] = 'TermsAndConditions.cou_id IS NULL';
}

$tandc = $this->find()
->where($whereClause)
->order('ordr ASC')
->all();
->contain('MostlyStaticPages')
->where($whereClause)
->orderBy(['TermsAndConditions.ordr' => 'ASC'])
->all();

if(!empty($tandc)) {
// Pull all the Agreements for $personId at once, we'll walk through them as needed.
Expand Down
2 changes: 1 addition & 1 deletion app/templates/TermsAndConditions/columns.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ $rowActions = [
// There should always be either a Mostly Static Page or a URL to reference.
if(!empty($entity->mostly_static_page->name)) {
// The T&C is a mostly static page. Provide a link to it.
return $vv_base_url . '/' . $entity->mostly_static_page->name;
return $vv_base_url . '/pages/' . $entity->mostly_static_page->name;
}
return $entity->url;
}
Expand Down
Loading