#2788 - Expiring Magic Links

[kodinkat]

• fixes: Expiring Magic Links #2788

---

[kodinkat]

@corsacca @cairocoder01 review ready....

[cairocoder01]

I love this! Nice work!

One minor comment: the expiration date in the accordion header feels a bit crowded. Could we use a shortened date format since the full date is inside the accordion? Something like Exp: 2/23/26 14:01? Also, it would be good if that date was in the local time zone instead of UTC - either the WP site timezone or client-side js time zone detection.

Also, what is the difference between setting a link to never expire or just hitting reset to clear the expiration date? By default, they don't expire. But when you set it to not expire, it explicitly says that it never expires. I'm a bit confused at the difference, so I'm guessing users will be too.

[github-actions]

PR Review: #2894 – Expiring Magic Links

Nice feature addition! The implementation structure is solid and the class_exists() guard for Disciple_Tools_Bulk_Magic_Link_Sender_API is the right approach to keep things backward-compatible. Here are several issues worth addressing before merging:

---

Bugs

1. Unused variable oldHash in app_link_reset

oldHash is computed but never used anywhere in the function:

const oldHash = url ? url.replace(appAccordion.dataset.urlBase, '') : '';

Either use it or remove it.

---

2. "Reset link (also clears expiration)" label always shown

<span class="app-link-label"><?php esc_html_e( 'Reset link (also clears expiration)', 'disciple_tools' ) ?></span>

This is rendered unconditionally, even when Disciple_Tools_Bulk_Magic_Link_Sender_API is not present and no expiration UI is shown. The label should only mention expiration when the feature is available:

<?php if ( class_exists( 'Disciple_Tools_Bulk_Magic_Link_Sender_API' ) ): ?>
    <span class="app-link-label"><?php esc_html_e( 'Reset link (also clears expiration)', 'disciple_tools' ) ?></span>
<?php else: ?>
    <span class="app-link-label"><?php esc_html_e( 'Reset link', 'disciple_tools' ) ?></span>
<?php endif; ?>

---

Performance

3. fetch_option_link_objs() called once per row

add_app_row() is called for every registered magic link app. Calling Disciple_Tools_Bulk_Magic_Link_Sender_API::fetch_option_link_objs() inside this method causes N repeated option lookups per page load. The link objects should be fetched once in the caller and passed in as a parameter.

---

Code Quality

4. Fragile REST URL construction

const endpointUrl = window.wpApiShare.site_url + '/wp-json/disciple_tools_magic_links/v1/...';

This hardcodes /wp-json/ and will break if the REST API base path is customized via the rest_url_prefix filter. Use window.wpApiShare.root (which maps to rest_url()) instead:

const endpointUrl = window.wpApiShare.root + 'disciple_tools_magic_links/v1/clear_link_expiration';

This applies to both AJAX calls in the new code.

---

5. Inconsistent error handling UX

app_link_set_expiration uses window.toastr.success() for success but alert() for failure. clear_expiration_after_reset silently swallows errors with only console.warn. These should follow a consistent pattern — prefer toastr throughout with a fallback for when it is not available.

---

6. Inline styles should move to SCSS

There are ~8 instances of inline style="..." attributes added in this PR (flexbox layout, font-size, colors, margins, borders). These should be extracted to dt-assets/scss/ with appropriate class names, per the project's conventions.

---

7. PHP: $link_obj_id potentially undefined

$link_obj_id is only assigned inside the if ( class_exists(...) ) block but referenced with ?? outside it. While PHP 7+ ?? handles undefined variables, initialize it before the block for clarity:

$link_obj_id = null;
if ( class_exists( 'Disciple_Tools_Bulk_Magic_Link_Sender_API' ) ) {
    // ...
}

---

8. Duplicate short date computation in PHP

The short date format is computed twice: once for data-expires-formatted-short and again for the $exp_short badge variable. Extract it to one variable and reuse.

---

9. Indentation inconsistency in badge creation block

In app_link_set_expiration, the if (appLabel) block is at the wrong indent level inside its else branch. The inner block body appears at the same level as the else keyword itself.

---

Minor Note

The data-expires-formatted-short date uses the WP site timezone via date_i18n(), which is appropriate for server-rendered content. Regarding @cairocoder01's earlier timezone comment — the client-side clear_expiration_after_reset updates the badge using data.expires.ts_formatted_short from the API response, so that value will only be as localized as what the backend returns. Worth confirming the backend also uses date_i18n() consistently.

[kodinkat]

@cairocoder01 see latest updates..... Also, good spot re "Never expires"; which, on further reflection, is actually redundant; whilst the "Reset Link" functionality is present - Therefore, I've decided to remove it.

[cairocoder01]

Looks good to me. I wonder about the verbiage of the Reset label. The use of "also" for some reason feels somewhat unprofessional or something. Should it be something like "Reset link (clears expiration)" or "Reset link (and clear expiration"? And should that verbiage only show if there is a current expiration (I haven't tested myself to know if this is already the case)? So if there's no expiration set, it just says "Reset link." If there is an expiration date set, it shows the extra info about clearing expiration. I'm unsure the best route and these are just thoughts.

Regardless of that, I'm happy with this however that verbiage ends up. So on to @corsacca to review.

[kodinkat]

Looks good to me. I wonder about the verbiage of the Reset label. The use of "also" for some reason feels somewhat unprofessional or something. Should it be something like "Reset link (clears expiration)" or "Reset link (and clear expiration"? And should that verbiage only show if there is a current expiration (I haven't tested myself to know if this is already the case)? So if there's no expiration set, it just says "Reset link." If there is an expiration date set, it shows the extra info about clearing expiration. I'm unsure the best route and these are just thoughts.

Regardless of that, I'm happy with this however that verbiage ends up. So on to @corsacca to review.

Hey @cairocoder01 yep, I'll also tweak verbiage and have its display made conditional based on any existing expiration dates. :)

[github-actions]

Follow-up Review

Thanks for the updates @kodinkat — the conditional label text for "Reset link (clears expiration)" is now handled correctly based on $has_expiration. A few items from my previous review are still outstanding:

---

Still Outstanding

1. Unused variable oldHash

This variable is computed but never read anywhere:

const oldHash = url ? url.replace(appAccordion.dataset.urlBase, '') : '';

Remove it to keep the code clean.

---

2. Hardcoded REST URL path

Both AJAX calls still construct the endpoint URL by concatenating site_url with a hardcoded /wp-json/ path:

const endpointUrl = window.wpApiShare.site_url + '/wp-json/disciple_tools_magic_links/v1/clear_link_expiration';

If a site has customized the REST API base path via rest_url_prefix, this will break. Use window.wpApiShare.root instead, which maps to rest_url() and handles the correct base:

const endpointUrl = window.wpApiShare.root + 'disciple_tools_magic_links/v1/clear_link_expiration';

This applies to both clear_expiration_after_reset and app_link_set_expiration.

---

3. $link_obj_id initialized inside the class_exists() guard

$link_obj_id is only assigned inside if ( class_exists('Disciple_Tools_Bulk_Magic_Link_Sender_API') ), but is referenced in the HTML template below with ??. While ?? technically handles undefined variables in PHP 7+, this is surprising to readers. Declare it before the guard:

$link_obj_id = null;
if ( class_exists( 'Disciple_Tools_Bulk_Magic_Link_Sender_API' ) ) {
    // ...assign $link_obj_id inside...
}

---

Nice to Have (non-blocking)

• Inline styles: The ~8 style="..." attributes added in this PR (flex layout, font-size, color, border) should ideally live in dt-assets/scss/ per the project's conventions, rather than being embedded in HTML.
• Performance: Disciple_Tools_Bulk_Magic_Link_Sender_API::fetch_option_link_objs() is called once per add_app_row() invocation. If there are many apps, fetching link objects once in the caller and passing them in would be more efficient.

---

The feature itself looks solid and the UX improvements are good. The REST URL issue (#2) is the only one I'd consider a functional correctness concern worth fixing before merge.

[github-actions]

PR Review: #2788 - Expiring Magic Links

Overall this is a well-structured feature addition. The conditional class_exists( 'Disciple_Tools_Bulk_Magic_Link_Sender_API' ) guard is the right approach for optional plugin integration. Here are some things to address:

---

Bug: Dead code – oldHash variable

In app_link_reset, a variable is computed but never used:

// Store old hash before reset
const appAccordion = evt.target.closest('.app-accordion');
const oldHash = url ? url.replace(appAccordion.dataset.urlBase, '') : '';

oldHash is never referenced after this point. Either remove it or wire it up to whatever it was intended for.

---

Bug: PHP variable $link_obj_id used outside its declaration scope

$link_obj_id is declared only inside the if ( class_exists( ... ) ) block, but then referenced unconditionally in the HTML:

data-link-obj-id="<?php echo esc_attr( $link_obj_id ?? '' ) ?>"

The ?? '' null-coalescing will suppress a fatal error, but with WP_DEBUG enabled this will emit an E_NOTICE (undefined variable). Declare $link_obj_id = null; alongside $expiration_data at the top of the block to fix this cleanly.

---

Performance: N+1 pattern in add_app_row()

$link_objs = Disciple_Tools_Bulk_Magic_Link_Sender_API::fetch_option_link_objs();

This is called on every add_app_row() invocation. When a post has multiple magic link apps, this results in repeated identical option lookups. Consider fetching and caching the result in dt_details_additional_section() and passing it as an argument to add_app_row().

---

Style inconsistency: inline styles vs. CSS block

The PR inlines several CSS rules directly on elements:

style="display: flex; flex-direction: column; align-items: flex-start;"
style="font-size: 0.85em; color: #666; margin-top: 0.25rem;"
style="margin-top: 1rem; padding-top: 1rem; border-top: 1px solid #e0e0e0;"

The file already has a <style> block where all other app accordion styles live. These should be moved into CSS classes there (e.g., .app-expiration-badge, .app-expiration-section) rather than using inline styles, which are harder to override and inconsistent with the rest of the file.

---

UX inconsistency: alert() vs. toastr

In app_link_set_expiration, validation errors use alert():

alert('...');

But the success path uses window.toastr. The error case in the AJAX failure also uses alert(). Prefer window.toastr.error(...) throughout for a consistent, non-blocking UX.

---

Minor: date_i18n() called twice for the same value

In add_app_row(), date_i18n( 'n/j/y G:i', (int) $exp_ts ) is computed twice — once for $exp_short and again for the data-expires-formatted-short attribute. Since $exp_short is already available, just reuse it:

data-expires-formatted-short="<?php echo esc_attr( $exp_short ) ?>"

---

Clarification: REST endpoints not defined in this PR

The JS calls two endpoints:

• disciple_tools_magic_links/v1/clear_link_expiration
• disciple_tools_magic_links/v1/set_link_expiration_direct

These are not registered anywhere in this file. If they come from the dt-bulk-magic-link-sender plugin, that dependency should be clearly documented. It would also be worth confirming that these endpoints enforce proper capability checks (not just nonce authentication), so that e.g. a multiplier can't set expiration on links they don't own.

[corsacca]

I realize i don't have the plugin updated, but so might others.

Should the relevant code be in the theme instead?

[github-actions]

PR Review: #2788 - Expiring Magic Links

Good feature addition! The approach of feature-detecting the plugin API before enabling expiration UI is solid. Here are my findings:

---

Bug: oldHash is computed but never used

In app_link_reset, this line is dead code:

// Store old hash before reset
const appAccordion = evt.target.closest('.app-accordion');
const oldHash = url ? url.replace(appAccordion.dataset.urlBase, '') : '';

oldHash is never referenced after being computed. Either it was meant to be used (e.g., to restore on failure) but was left incomplete, or it should be removed.

---

Performance: fetch_option_link_objs() called on every app row

 = Disciple_Tools_Bulk_Magic_Link_Sender_API::fetch_option_link_objs();

This is called inside add_app_row(), which is invoked once per magic link app. If a post has many apps, this could result in repeated DB/option reads. Consider caching the result with a static variable or fetching it once at the call site and passing it as a parameter:

static  = null;
if (  === null ) {
     = Disciple_Tools_Bulk_Magic_Link_Sender_API::fetch_option_link_objs();
}
 = ;

---

Code Style: Inline styles should move to SCSS

There are numerous inline styles added throughout:

<div class="app-label" style="display: flex; flex-direction: column; align-items: flex-start;">
<span class="app-expiration-badge" style="font-size: 0.85em; color: #666; margin-top: 0.25rem;">
<div class="app-link-row" style="margin-top: 1rem; padding-top: 1rem; border-top: 1px solid #e0e0e0;">

And in JS:

newBadge.style.cssText = 'font-size: 0.85em; color: #666; margin-top: 0.25rem;';

Per the project's build setup, styles belong in dt-assets/scss/. Moving these there would also make the badge color and sizing consistent between the server-rendered and JS-generated badge variants.

---

Minor: Date format is US-centric

date_i18n( 'n/j/y G:i', (int) ['ts'] )

While date_i18n respects locale for day names, the format pattern n/j/y G:i (M/D/YY 24h) is US-style. Consider using WordPress's configured date/time format:

date_i18n( get_option( 'date_format' ) . ' ' . get_option( 'time_format' ), (int) ['ts'] )

This also applies to the hardcoded JS frontend badge format strings referencing the same n/j/y G:i pattern embedded in PHP data attributes.

---

Minor: data-link-obj-id attribute set but unused in JS

data-link-obj-id="<?php echo esc_attr( $link_obj_id ?? '' ) ?>"

This data attribute is rendered into the DOM but is never read by any of the JS functions (app_link_reset, app_link_set_expiration, clear_expiration_after_reset). If it's reserved for future use, a comment explaining that would help. If it's no longer needed, it should be removed.

---

What works well

• The $magic_link_expiration_supported capability check is the right pattern for theme/plugin interop — it degrades gracefully when the plugin isn't present.
• Nonce is correctly sent via X-WP-Nonce header in all AJAX calls.
• PHP output is properly escaped throughout (esc_attr, esc_html, esc_js, esc_url).
• get_app() returns dataset directly, so the new data-sys-type/data-record-id attributes are automatically picked up via camelCase conversion — no changes to get_app needed.
• Input validation on the expiration amount before submission is a nice touch.

[github-actions]

PR Review: Expiring Magic Links (#2788)

Thanks for the implementation! The feature concept is solid and the plugin capability check is a nice touch. A few issues worth addressing:

---

Bug: Incorrect id assignment using || operator

In both app_link_reset and app_link_set_expiration, the id is computed as:

const id = recordId || (corresponds_to_user || post_id);

This is problematic because corresponds_to_user could be falsy (e.g. 0 or false) for non-user posts, causing it to fall through to post_id regardless of intent. Since sys_type is already computed correctly, derive the id from it for clarity and correctness:

const sys_type = sysType || (corresponds_to_user ? 'wp_user' : 'post');
const id = recordId || (sys_type === 'wp_user' ? corresponds_to_user : post_id);

---

Inline styles should move to SCSS

The PR adds many inline style="..." attributes (flexbox layout, font sizes, colors, borders, etc.). Per project conventions, styles belong in dt-assets/scss/, not inline on elements. This would also make the badge style consistent with the .app-expiration-badge class that is added dynamically in JS (which also uses inline style.cssText). Extracting these to the stylesheet would keep styling maintainable.

---

Use toastr consistently for error/validation messages

In app_link_set_expiration, validation failures and errors use alert():

alert('Please enter a valid expiration amount...');
// ...
alert('Error updating expiration. Please try again.');

But success uses toastr. This is inconsistent UX. Prefer toastr.error() / toastr.warning() throughout, matching patterns used elsewhere in the theme.

---

Hardcoded REST endpoint paths

The plugin REST routes are hardcoded in JS:

window.wpApiShare.root + 'disciple_tools_magic_links/v1/clear_link_expiration'
window.wpApiShare.root + 'disciple_tools_magic_links/v1/set_link_expiration_direct'

If these routes change in the plugin, the feature breaks silently. Consider having the PHP side expose these via wpApiShare (e.g. wpApiShare.expiration_endpoints) when $magic_link_expiration_supported is true, so the JS does not depend on hardcoded path strings.

---

Missing max attribute on expiration amount input

<input type="number" min="1" value="">

There is no max attribute. A user could submit e.g. "999999 months". Add a reasonable maximum (e.g. max="365") or ensure the server validates this with a meaningful error response.

---

Minor: clear_expiration_after_reset fires even without plugin support

clear_expiration_after_reset is called after every link reset whenever window.wpApiShare exists — including when $magic_link_expiration_supported is false on the server. The AJAX call will simply 404 (caught silently), but it is an unnecessary network request. Consider gating the call on a JS flag set from PHP:

// In the rendered script block:
const magicLinkExpirationSupported = <?php echo $magic_link_expiration_supported ? 'true' : 'false'; ?>;

---

Overall the approach is well-structured and the backward-compatibility handling via class_exists/method_exists is good. Addressing the bug, inline styles, and UX consistency issues would make this PR ready to merge.

[kodinkat]

I realize i don't have the plugin updated, but so might others.

Should the relevant code be in the theme instead?

Moving those methods into the theme would:

• Duplicate core plugin logic (expiration storage, meta keys, etc.).
• Risk divergence/conflicts once the plugin is updated to a newer version that also ships these methods.

A tighter, safer fix is to feature‑detect the “new expiration API” and only use it when it’s actually available.

....review ready...

[corsacca]

@kodinkat, please display the date in the user's time (pass epoch and let js in the bowser display it).
How often does the service run that expires the links? Set set one for 1 min can access it afterwards.