Skip to content

Fix reminder notification#323

Merged
luke- merged 3 commits into
masterfrom
fix/322-reminder-notification
Jul 7, 2026
Merged

Fix reminder notification#323
luke- merged 3 commits into
masterfrom
fix/322-reminder-notification

Conversation

@yurabakhtin

Copy link
Copy Markdown
Contributor

@luke-

luke- commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Thanks for digging into this, @yurabakhtin! The param-passing fix in views/remind.php is correct and resolves the actual crash. But I'd push back on the isConsoleRequest switch in RemindStart::init() — I think it's the wrong mechanism and works only by coincidence.

Web vs. mail rendering isn't driven by the request type — it's driven by the target/renderer. HumHub already separates them:

  • WebRenderer searches notifications/views/
  • MailRenderer sets subPath = 'mails', so it searches notifications/views/mails/

Both views already exist in the module:

  • notifications/views/remind.php → web (via layouts/remind.php)
  • notifications/views/mails/remind.php → email (uses MailHeadline / MailContentEntry / MailButtonList and the @notification/views/layouts/mail.php layout)

So with a constant viewName = 'remind', the web target automatically renders the web view and the mail target automatically renders the mail view — no condition needed. isConsoleRequest only correlates with "email" because reminders happen to be sent from cron (console) while the dropdown is rendered in the browser (web). That breaks for queued/async mail, digest/summary mails that render web notifications in console context, mail sent from a web request, and tests.

On the "layout was developed for Email mode" point — I don't think that's the case. layouts/remind.php produces a <li data-notification-id><a href>…</li> with TimeAgo and the "New"/"Reminder" badges, which is the web dropdown item, not an email layout (the email view is the separate views/mails/remind.php). The reason it renders wrong is that it's stale pre-BS5 markup: current core renders the item as <a class="d-flex" data-notification-id> (see @notification/views/layouts/web.php), not as an <li>.

Also note a side effect of the current approach: in a web request viewName now falls back to the inherited default 'default.php', so the dropdown uses the generic core view and views/remind.php + layouts/remind.php become dead code for web display — meaning the params we just fixed there are never reached on the web path.

Suggested alternatives:

  • Option A (minimal): keep viewName = 'remind' as a plain property (drop init() / isConsoleRequest) and delete the stale views/remind.php + views/layouts/remind.php. Web → generic core view, mail → views/mails/remind.php. No dead code, no condition.
  • Option B (if we want the "Reminder" badge in the dropdown): keep viewName = 'remind' constant, but update layouts/remind.php to match the current core structure (<a class="d-flex" data-notification-id> instead of <li>) and pass the params (as this PR already does). Both targets then resolve correctly via subPath.

Either way, the view selection belongs to the target/renderer, not the execution context.

@yurabakhtin

Copy link
Copy Markdown
Contributor Author

@luke- Ok, new changes f412b47:

email-task notif-all-task notif-modal-task

@luke- luke- left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @yurabakhtin — this is exactly Option B, nicely done. I diffed the new layouts/remind.php against core's @notification/views/layouts/web.php: it's a 1:1 copy plus the "Reminder" badge, and with the constant viewName = 'remind' both targets now resolve correctly (web → views/remind.php, mail → views/mails/remind.php via subPath). The BS5 markup it mirrors has been in core since 1.18.0-beta.2, so the current minVersion is fine.

Nice side effect of the validation change: the old $remindModes array was missing REMIND_ONE_WEEK, so "1 Week before" would have failed validation — array_keys(getRemindModeItems()) fixes that. I also verified the removed $remindModes / getRemindMode() aren't referenced anywhere else.

One small thing before merge (see inline suggestion): views/remind.php doesn't pass relativeUrl through to the layout, so $relativeUrl ?? $url always falls back to the absolute URL — unlike all other notifications in the dropdown. Nit: trailing newline at the end of views/remind.php is missing.

Comment thread notifications/views/remind.php
@luke- luke- merged commit 9b7324d into master Jul 7, 2026
13 checks passed
@luke- luke- deleted the fix/322-reminder-notification branch July 7, 2026 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants