Add support for NLS link to multisearch widget#200
Conversation
matt-bernhardt
force-pushed
the
use-631
branch
2 times, most recently
from
5124912 to
a6b7e4b
Compare
** Why are these changes being introduced: We want users of WordPress to see a different configuration of the search form, depending on whether they have opted in to using natural language search in Search MIT Libraries. We also want to have an ability to declare the default condition (if the user does not have a cookie set), and also the ability to suppress the entire NLS link if it turns out there's a problem on launch day. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/use-631 ** How does this address that need: This defines two new options within the multisearch widget configuration form: - nls_included, which determines whether the link is shown at all - nls_default, which defines whether the NLS option should be shown as enabled or disabled if the user has no cookie set. We also introduce two methods in the widget class, to read the cookie state and set the nls_enabled variable based on the priority of: 1. The user's cookie value 2. The widget's declared default value The link to alter the cookie is also calculated based on this logic, in a separate method. ** Document any side effects to this change: The choice to add an nls_included variable is a late decision between Jeremy and I. Fingers crossed, it should not be necessary. The plugin version is also bumped to 1.7.
There was a problem hiding this comment.
Pull request overview
Adds a Natural Language Search (NLS) opt-in link and widget configuration so the “Unified Search” (USE) multisearch panel can expose and default an NLS mode.
Changes:
- Adds NLS toggle UI to the USE (“All”) tab template, gated by a new widget setting.
- Adds widget admin settings for enabling NLS and choosing the default mode, plus cookie-based state handling at render time.
- Bumps the plugin version to 1.7.0.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| web/app/plugins/mitlib-multisearch-widget/templates/tab-all-use.php | Adds conditional NLS toggle link UI to the USE tab template. |
| web/app/plugins/mitlib-multisearch-widget/class-multisearch-widget.php | Adds widget settings/state for NLS, cookie reading, and toggle value computation. |
| web/app/plugins/mitlib-multisearch-widget/mitlib-multisearch-widget.php | Increments plugin version to reflect the feature addition. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| */ | ||
|
|
||
| ?> | ||
| <!-- nls state: _<?php echo esc_attr( $nls_enabled ); ?>_ --> |
There was a problem hiding this comment.
We are intentionally including this for now, for debugging purposes if it turns out we've missed something on launch day.
| // Determine whether to enable NLS based on widget settings and the user's cookie. | ||
| $nls_enabled = $this->readCookie( $instance['nls_default'] ); | ||
|
|
||
| $nls_link_toggle = $this->setToggleValue( $nls_enabled ); | ||
|
|
||
| $nls_included = $instance['nls_included']; |
There was a problem hiding this comment.
As with the other piece of feedback, I'm choosing to accept the outcome of a handful of PHP notices launched into the ether in the handful of seconds between placing this widget and the first time the configuration form is saved.
This particular block has the advantage of being self-contained, so code consistency isn't as big of a concern - but I'm also not particularly bothered given the scale of use this code will see in its lifespan.
I'm open to pushback on this during code review, but I'm going to propose this as-is to start with.
| $nls_default = $instance['nls_default']; | ||
| if ( '' == $instance['nls_default'] ) { | ||
| $nls_default = 'off'; | ||
| } | ||
| $nls_included = $instance['nls_included']; |
There was a problem hiding this comment.
While I appreciate this feedback, I'm going to choose to keep these initialization assignments the same as the other values defined in this class, rather than introduce an inconsistency between these two values and the rest of what we've built.
Given that this widget will be defined once, on one site, which already swallows PHP notices, I'm willing to accept a few messages like this being thrown into the ether for the moment.
There is a future cleanup of this plugin after the new platform launches that will remove a lot of logic that will no longer be needed. It may be more appropriate to revisit which patterns we implement at that point.
| $instance['nls_default'] = $new_instance['nls_default']; | ||
| $instance['nls_included'] = $new_instance['nls_included']; |
There was a problem hiding this comment.
I'm not sure I agree with this feedback. The nls_default field is a radio input, and is constrained to specific values already. The nls_included field is a checkbox, and is also constrained. Duplicating those constraints here introduced unhelpful replication of business logic that will need to be kept in sync if our needs ever change, without adding any security benefit.
I'm open to counter arguments during code review, but my inclination right now is to leave this as is, as with the other field handling logic in the update method.
| * ReadCookie looks for the 'nls_enabled' domain cookie in the $_COOKIE | ||
| * superglobal. If no value is found, it returns the default value - which is | ||
| * defined via the widget settings form. |
There was a problem hiding this comment.
This makes sense - I'll fix in the next commit.
djanelle-mit
left a comment
djanelle-mit
left a comment
There was a problem hiding this comment.
UX-wise, things look good on my end. Toggle seems to respect the cookie and the ability to turn off the NLS feature entirely is really handy just in case. I did tag Darcy in the ticket so might be worth checking on her feedback too.
JPrevost
left a comment
JPrevost
left a comment
There was a problem hiding this comment.
Functionally this looks like what we asked for. Thanks!
This updates the widget settings form for the Multisearch Widget, adding two new controls (pictured here)
About this feature
The display of the natural language feature is thus meant to be determined along the following lines:
(the result of this calculation can always be seen in markup, in an HTML comment that is rendered even if the "include this feature" checkbox is set to "off". I'm including that debugging step as a hedge against having to turn the checkbox off while still troubleshooting what the behavior would be when we enable it again.)
About this multidev
I've configured the multidev in a specific way, to demonstrate a confirmation check that I'll be performing once this gets deployed to production. Details of that, including links to some internal pages demonstrating what's going on, have been added to the Jira ticket: https://mitlibraries.atlassian.net/browse/USE-631
Developer
Ticket: https://mitlibraries.atlassian.net/browse/USE-631
Stylesheets
string incremented.
Secrets
Documentation
Accessibility
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)
Stakeholder approval
Dependencies
NO dependencies are updated
Code Reviewer
(not just this pull request message)