Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugin Search: Add proof of concept #10611

Merged
merged 42 commits into from Feb 26, 2019

Conversation

@beaulebens
Copy link
Member

beaulebens commented Nov 13, 2018

This adds a super super ugly/rough proof of concept of making suggestions when people search for functionality that Jetpack already provides.

We've seen that people with Jetpack installed and activated often search for Jetpack features (even by name) in the Plugins > Add New screen in wp-admin. This new module attempts to spot those searches, and provide an artificial search result that calls out that what they're looking for is in Jetpack, which they already have, and which is already active.

This code is definitely not ready for prime time; it should hook into our existing search terms for settings search, should use better descriptions etc, needs to use real plugin data vs the hardcoded bits I used, and needs to make it really clear that this is an artificial search result, not something coming from the plugin directory directly.

Testing Instructions

  1. Switch to this branch
  2. Plugins > Add New
  3. Search for something that Jetpack provides (e.g "contact form")
  4. See that the first result should be a Jetpack-powered card.

Proposed changelog entry

  • When people search plugins for features that Jetpack provides, give them hints.
@jetpackbot

This comment has been minimized.

Copy link

jetpackbot commented Nov 13, 2018

Warnings
⚠️ "Testing instructions" are missing for this PR. Please add some

This is automated check which relies on PULL_REQUEST_TEMPLATE.We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against c6b2902

@matticbot

This comment has been minimized.

Copy link

matticbot commented Nov 13, 2018

This PR looks like it might contain user tracking functions. We need to make sure that it is GDPR Compliant.

Rules triggering this positive scan:

  • Called "record_user_event" function.

cc: @pesieminski

/**
* Put some more appropriate links on our custom result cards.
*/
public function insert_module_related_links( $links, $plugin ) {

This comment has been minimized.

Copy link
@beaulebens

beaulebens Nov 30, 2018

Author Member

We should make sure to add Tracks to these links, so that we understand how people are using it/which links are most useful, and probably also something that indicates whenever one of these cards/results are shown.

@beaulebens

This comment has been minimized.

Copy link
Member Author

beaulebens commented Dec 5, 2018

Note we're seeing some messages with WP_DEBUG enabled;

Notice: Undefined index: module in /srv/users/userec716f4b/apps/userec716f4b/public/wp-content/plugins/jetpack-dev/modules/plugin-search.php on line 208

@beaulebens

This comment has been minimized.

Copy link
Member Author

beaulebens commented Dec 5, 2018

And for reference, here's an example of what it's currently inserting into the results:

screen shot 2018-12-05 at 8 30 03 am

@georgestephanis

This comment has been minimized.

Copy link
Member

georgestephanis commented Dec 14, 2018

Conditions to test:

  • Jetpack is activated, but not connected (yet) to WordPress.com -- probably display a "Connect" link.
  • Local dev mode -- no connection.
  • Module is not activatable by the current user (for whatever reason -- maybe that specific module is disabled)
  • Module is active, but has no configurable options.
  • ????
@beaulebens

This comment has been minimized.

Copy link
Member Author

beaulebens commented Dec 14, 2018

I was just playing around with this and noticed that it doesn't kick in/load properly until Jetpack is connected. I think ideally this would be active as long as Jetpack is installed and activated, even before it's connected (to let people know that they've got stuff available, but it's not working yet because they're not connected).

@georgestephanis

This comment has been minimized.

Copy link
Member

georgestephanis commented Jan 21, 2019

Just pushed up some things I've had staged for a while along with some other changes.

Current status:

This now works well both with the ajax-y loading of plugin search, as well as deep linking with the query string search (which would be what happens when you reload a search page). It's also dynamically pulling Jetpack's plugin info in to populate the card, and has a number of other enhancements.

What does not exist yet:

  • Modules without configuration pages will display a configuration link after clicking 'activate' but on subsequent reloads will not. This is because of the current way we determine whether a module is configurable, which depends on filters in the module itself, that we can't test until it's active.
  • When Jetpack is not yet connected, nor in development mode, we need to create a "CONNECT JETPACK" button that will kick off the connection flow, and then auto-enable the relevant module (if it's not already in auto-activated modules or the like)
  • Probably should do an audit of the keywords we're responding to searches for -- and handle translations for them.
  • Add more tracks for seeing how this is useful to users. What are our relevant events that we would want to compare? I'd suggest tracking occurrence of a module card being displayed, occurrence of a module being activated / configured off a card, and perhaps occurrence of another plugin being installed with / without one of our modules being displayed, to provide some context. Perhaps split it out per module. This may also be too much data. Unsure.
  • Add a filter per-module that will let us entirely customize the card displayed for a given module, rather than just use the existing format. For example, for the Jetpack Contact Form, we may want to add a "Try It Out" link -- that will take the user to a new post page, with a contact form already inserted, that they can then begin tweaking.
  • Add a kill switch if users want to disable these?
  • Find a better, less-hacky way to do this? 8e512a3

None of these I think are strictly necessary, and I think most (all?) could probably be punted to a v1.1 done after we get initial feedback from a first quiet launch / user testing.

I believe we're also waiting for a Master Thread post from @jessefriedman but I've not circled back with him directly about that since we spoke just before the holidays.

@jessefriedman

This comment has been minimized.

Copy link
Member

jessefriedman commented Jan 28, 2019

We need to figure out how to handle Jetpack showing in the results as in this example:
screen shot 2019-01-28 at 1 35 00 pm

@jessefriedman

This comment has been minimized.

Copy link
Member

jessefriedman commented Jan 28, 2019

Also here: #10611 (comment) Jetpack has a weird state of "inactive" but Jetpack is active. So the "Activate" button should be greyed out and say "Active" like this:

screen shot 2019-01-28 at 1 42 48 pm

@rickybanister

This comment has been minimized.

Copy link
Member

rickybanister commented Feb 1, 2019

Just a note, we'll need to confirm that the styles we write for this have commensurate versions in Calypso-ify.

@matticbot

This comment has been minimized.

Copy link

matticbot commented Feb 26, 2019

Caution: This PR has changes that must be merged to WordPress.com
Hello beaulebens! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D24833-code before merging this PR. Thank you!

@dereksmart

This comment has been minimized.

Copy link
Member

dereksmart commented Feb 26, 2019

I think the Fusion diff will apply cleanly if this were rebased

@jeherve

This comment has been minimized.

Copy link
Member

jeherve commented Feb 26, 2019

  • search for Contact Form or another module that does not have visual settings. When it's active, a "Get started" button should be displayed and the "Learn more" link should be hidden.

Where should that get started button lead? I tried searching for "Publicize" when the module was inactive. I then activated the module via the hint card, and the "Get Started" button was linking to https://jetpack.com/redirect/?source=plugin-hint-learn-publicize. I assume, like for JITM, that is how we want to track the clicks on those links? Do we need to test with a specific Phab diff applied, where those redirects are set up and that plugin-hint-learn-publicize source redirects to the Sharing page in Calypso?

@jeherve

This comment has been minimized.

Copy link
Member

jeherve commented Feb 26, 2019

Do we want to track dismissals as well, to see if some suggestions are not deemed relevant by a lot of users?

@jeherve

This comment has been minimized.

Copy link
Member

jeherve commented Feb 26, 2019

The suggestion for "Spam" seems a bit weird, given that the Akismet plugin also appears in the search results:

image

@jeherve

This comment has been minimized.

Copy link
Member

jeherve commented Feb 26, 2019

When I click on the card title or the card image, I would expect to learn more about the feature. Instead, the modal that pops up is for the Jetpack plugin itself. Is that okay? Could / should we use the same link we use for the "Learn more" text instead?

@jeherve
Copy link
Member

jeherve left a comment

This will be super useful! Nice work!

I left a few questions in comments above, and inline.

$jetpack_modules_list = Jetpack_Admin::init()->get_modules();
// Never suggest this module.
unset( $jetpack_modules_list['plugin-search'] );

This comment has been minimized.

Copy link
@jeherve

jeherve Feb 26, 2019

Member

We probably do not need this anymore.

JetpackTracking::record_user_event( 'wpa_plugin_search_match_found', array( 'feature' => $matching_module ) );
$inject = (array) self::get_jetpack_plugin_data();
$image_url = plugins_url( 'modules/plugin-search/psh', JETPACK__PLUGIN_FILE );

This comment has been minimized.

Copy link
@jeherve

jeherve Feb 26, 2019

Member

Given that it's a fairly generic image, I wonder if we should add it to the images directory instead; maybe we can re-use it for other things in the future?

$siteFragment = Jetpack::build_raw_urls( get_home_url() );
switch ( $feature ) {
case 'sharing':
case 'publicize':

This comment has been minimized.

Copy link
@jeherve

jeherve Feb 26, 2019

Member

I don't think that worked in my tests. The "configure" link for Publicize was still the link to the Jetpack Settings screen.

(
Jetpack::is_active() ||
(
Jetpack::is_development_mode() &&

This comment has been minimized.

Copy link
@jeherve

jeherve Feb 26, 2019

Member

Do we need this here, given that we don't seem to initiate anything when the plugin is not connected to WordPress.com?
https://github.com/Automattic/jetpack/pull/10611/files#diff-f33103bd412ed49a26cdf8f3756f0e58R11

jeherve added a commit that referenced this pull request Feb 26, 2019

@matticbot

This comment has been minimized.

Copy link

matticbot commented Feb 26, 2019

beaulebens, Your synced wpcom patch D24833-code has been updated.

@jeherve
Copy link
Member

jeherve left a comment

This looks good for now. Merging and including this in the Beta. we can iterate further in the next few days 👍

@jeherve jeherve merged commit 593ee4c into master Feb 26, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
wpcom No changes in affected files detected: D24833-code
Details

@jeherve jeherve deleted the add/plugin-search-hints branch Feb 26, 2019

jeherve added a commit that referenced this pull request Feb 26, 2019

Plugin Search: Add proof of concept (#10611)
This adds a super super ugly/rough proof of concept of making suggestions when people search for functionality that Jetpack already provides.

We've seen that people with Jetpack installed and activated often search for Jetpack features (even by name) in the Plugins > Add New screen in wp-admin. This new module attempts to spot those searches, and provide an artificial search result that calls out that what they're looking for is in Jetpack, which they already have, and which is already active.

This code is _definitely_ not ready for prime time; it should hook into our existing search terms for settings search, should use better descriptions etc, needs to use real plugin data vs the hardcoded bits I used, and needs to make it really clear that this is an artificial search result, not something coming from the plugin directory directly.

### Testing Instructions

1. Switch to this branch
2. Plugins > Add New
3. Search for something that Jetpack provides (e.g "contact form")
4. See that the first result should be a Jetpack-powered card.

### Proposed changelog entry
- When people search plugins for features that Jetpack provides, give them hints.

Co-authored-by: Yaroslav Kukharuk <i.kukharuk@gmail.com>
Co-authored-by: George Stephanis <georgestephanis@automattic.com>
Co-authored-by: Elio Rivero <eliorivero@gmail.com>
Co-authored-by: Derek Smart <smart@automattic.com>
@jeherve

This comment has been minimized.

Copy link
Member

jeherve commented Feb 26, 2019

Cherry-picked to branch-7.1 in 9c7f761

jeherve added a commit that referenced this pull request Feb 26, 2019

jeherve added a commit that referenced this pull request Feb 26, 2019

jeherve added a commit that referenced this pull request Feb 26, 2019

General: update module headings. (#11422)
#### Changes proposed in this Pull Request:
Follow-up from #10611

#### Testing instructions:


* none

#### Proposed changelog entry for your changes:


* none
public static function dismiss( WP_REST_Request $request ) {
return self::add_to_dismissed_hints( $request['hint'] )
? rest_ensure_response( array( 'code' => 'success' ) )
: new WP_Error( 'not_dismissed', esc_html__( 'The card could not be dismissed', 'jetpack' ), array( 'status' => 400 ) );

This comment has been minimized.

Copy link
@yoavf

yoavf Mar 6, 2019

Member

Translators were asking what type of card this is. Please add a context or a translator comment to clarify.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.