Overview

Experience Builder provides a working space where content creators can work on several changes at once. In order to provide a seamless editing experience, it is desired to make navigating between pages as seamless as possible by implementing navigation within Experience Builder. For this reason, we have implemented a navigator which is initially used for just XB pages. This will be expanded in future to include content templates and later even CMS content. Eventually, users will be able to navigate between different content creations UIs without having to leave Experience Builder.

Proposed resolution

The navigator already has a text field for searching for content in the editor navigation that is not functional. Update `\Drupal\experience_builder\Controller\ApiContentControllers::list` to read a `search` query parameter and do a `LIKE` query on the `label` entity key.

Update `contentApi` and `getContentList` to pass the `search` query param.

Update `PageInfo` to pass a function to `Navgation` component's `onSearch` prop to perform a refresh of `useGetContentListQuery` when search is performed.

User interface changes

Pressing "enter" in search no longer shows the "not implemented yet" alert and pages can be searched

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

mglaman created an issue. See original summary.

wim leers’s picture

Update `\Drupal\experience_builder\Controller\ApiContentControllers::list` to read a `search` query parameter and do a `LIKE` query on the `label` entity key.

If only we had QUERY!

But at least in this case, the querystring itself should be comparatively tiny, so ?search=hello%20world will do just fine 👍

Note: ApiContentControllers::list() may end up finding hundreds of matches. But the API response does not yet support pagination. Thoughts?

mglaman’s picture

But the API response does not yet support pagination. Thoughts?

Technically it'd still be a subset of the default result without a query string. But I do think getting pagination is going to become important pretty soon

deepakkm made their first commit to this issue’s fork.

deepakkm’s picture

Assigned: mglaman » deepakkm
Status: Active » Needs work

deepakkm’s picture

Assigned: deepakkm » Unassigned
Status: Needs work » Needs review

All the threads are resolved hence moving into review.

wim leers’s picture

Status: Needs review » Needs work
deepakkm’s picture

Status: Needs work » Needs review
wim leers’s picture

Status: Needs review » Needs work
deepakkm’s picture

Status: Needs work » Needs review
wim leers’s picture

Component: Page builder » Page
Status: Needs review » Needs work
Parent issue: » #3521002: [META] Maintainable client-side data model + internal HTTP API

Per @penyaskito's review.

catch’s picture

I'm a bit confused why this is a 1.0 blocker, there's already ways to search for content in Drupal. Where are the requirements for this feature documented?

lauriii’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Added some more context to the issue summary.

deepakkm’s picture

Status: Needs work » Needs review

I think its good to go now. Hence moving it in review.

wim leers’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

@catch's question in #14 is totally fair. Can we please get the designs we're implementing linked here, @lauriii? 🙏


See detailed review on the MR. Test coverage is not complete enough yet for this to be mergeable. 😅

lauriii’s picture

deepakkm’s picture

created a separate ticket to cover autocomplete feature as per discussion here https://www.drupal.org/project/experience_builder/issues/3522488

deepakkm’s picture

Status: Needs work » Needs review
deepakkm’s picture

Status: Needs review » Needs work
deepakkm’s picture

Status: Needs work » Needs review
wim leers’s picture

Title: Allow searching for content in the editor navigation » Allow searching for content in the navigator
Status: Needs review » Needs work
Issue tags: +JavaScript
Related issues: +#3522488: Follow-up for #3518292: `ApiContentControllers::list()`: search should exclude entity query matches for entities with auto-save data
deepakkm’s picture

Status: Needs work » Needs review
Issue tags: -JavaScript +JavaScript
deepakkm’s picture

Issue tags: -Needs tests, -JavaScript
wim leers’s picture

Component: Page » Internal HTTP API
Status: Needs review » Needs work
Issue tags: +Needs tests

The List with search test case of ApiContentControllersList has been failing.

Also: I found a pretty massive bug which indicates the current test coverage is not adequate.

deepakkm’s picture

Status: Needs work » Needs review
wim leers’s picture

Status: Needs review » Needs work

I know the sense of urgency y'all were under, so I spent ~30 mins reviewing this during my PTO today.

This MR is unfortunately not at all ready.

  1. The tests are still hard failing on SQLite and PostgreSQL. You didn't implement what we agreed upon on Wednesday.
  2. navigation.cy.js is consistently failing on the new test coverage, even after I re-tested it.
  3. The information disclosure security vulnerability I pointed out on May 8 is still present, and the review thread was closed with a non-explanation 😅

… and that's just the big ones. There's plenty of smaller concerns, most of which I pointed out long ago. I have not yet re-reviewed all closed MR threads, because a bunch seem to have been closed prematurely 🙈

deepakkm’s picture

Assigned: Unassigned » deepakkm
deepakkm’s picture

Assigned: deepakkm » Unassigned
lauriii’s picture

Issue tags: +beta blocker
deepakkm’s picture

Assigned: Unassigned » deepakkm
deepakkm’s picture

Assigned: deepakkm » Unassigned
Status: Needs work » Needs review

Mostly covered everything, but in case something is left do let me know.

deepakkm’s picture

Assigned: Unassigned » larowlan
wim leers’s picture

Title: Allow searching for content in the navigator » Allow searching for content in the navigator, via `/xb/api/v0/content/{entity_type}`
Priority: Normal » Major
Issue tags: -Needs tests

Did a high-level review. Addressed all my own nits. Since this touches auto-save things I'd like that to be reviewed by @tedbow or @larowlan.

And I'd definitely like @larowlan to do a thorough review of the changes to ApiContentControllers

larowlan’s picture

Assigned: larowlan » deepakkm

Addressed the test fails and remaining items I could.

There are 3 items left that I'm not sure about but @deepakkm is best placed to answer

1 of them I think is already handled in a follow-up
The other two I'm not sure of the status/reasoning

deepakkm’s picture

Assigned: deepakkm » Unassigned

This is good for a review now.

deepakkm’s picture

Assigned: Unassigned » wim leers

wim leers changed the visibility of the branch 0.x to hidden.

wim leers changed the visibility of the branch 3532268-xbs-testsite-recipe to hidden.

wim leers’s picture

I wanted to review, RTBC and merge this, but found myself unable to — so I spent a few hours getting this ready for commit.

Searching article nodes (which have XB fields and are editable in XB) did not work yet. And that's apparently intentional; but the route definition does not make that clear, nor does the bounds checking. Hardened that, to pave a clearer path for #3498525.

Most importantly: the logic in \Drupal\experience_builder\Controller\ApiContentControllers::list() would've made XB harder to maintain, because its complexity was disproportionate to the functionality it provided: it was very complicated, not helped by the 8 (!!!) helper methods, incorrectly named variables and unfortunately about a dozen small bugs that made it even harder to understand. 🫣

Before vs after: improved maintainability and quite literally a simpler MR:

 src/Controller/ApiContentControllers.php | 192 +++++++++++--------------------
 1 file changed, 69 insertions(+), 123 deletions(-)

(See attached diff. @deepakkm, I hope you find the sequence of commits I pushed to this MR useful as a reference in the future. I tried many times to express this in MR comments, but I seem to have failed to convey my message. This is probably the only MR with >200 comment threads. Given this MR needs to land before #3515932: Milestone 1.0.0-beta1: Enable creation of non-throwaway sites, and the GitLab UI is starting to struggle to still load at all, I figured it's reasonable for me to step in and show by example what I failed to convey in my review comments.)


Landed this despite one known bug: #3533096: Follow-up for #3518292: fix front-end performance and scalability of search functionality in navigator

Also ran into a pre-existing bug: #3529924-17: Add access check for using Experience Builder at all: if >=1 content entity type with an XB field can be created or edited..

  • wim leers committed f3ca3ddf on 0.x authored by deepakkm
    Issue #3518292 by deepakkm, wim leers, larowlan, mglaman: Allow...
wim leers’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.