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

Add wagtail-inventory to search pages by block type #2816

Merged
merged 6 commits into from
Sep 22, 2017
Merged

Conversation

chosak
Copy link
Member

@chosak chosak commented Mar 23, 2017

I wrote some code that allows Wagtail users to search for pages by the blocks they do or do not contain:

image

This PR adds a new dependency on a new wagtailinventory app to add this functionality.

There's a management command that needs to be run to setup the initial database table used for the query; subsequent page creates, edits, and deletes should update search results automatically.

Additions

Testing

  • This is best tested with a production data dump so you have lots of pages to search against. You'll need to python cfgov/manage.py migrate (possibly with --fake-initial) to create the new DB table used by this app.

  • You'll need to run this management command to setup the initial list of page/block pairs:

    $ python cfgov/manage.py block_inventory

    This might take a little while to run.

  • Run a local server and visit http://localhost:8000/admin/inventory/ to do some searches.

Screenshots

Access to this new search is via the sidebar:

image

You can click through to edit the page or visit its live URL:

image

Notes

  • This is just a first cut at some functionality which a few people have expressed interest in. Things to add/fix:

    • The repo needs a better README with examples using the Wagtail test site.
    • This code also needs tests. (ಥ﹏ಥ)
    • Use JS to dynamically add/remove blocks to search for instead of hardcoding a max of 3; this isn't as simple to do in Django as one might hope.
    • I know there are some nested, nested, nested blocks (for example BureauStructureDivision) that aren't being picked up right now due to some issue with this nutty logic.
  • It'd be nice to get review by some FEWDs who have more familiarity with some of our more exotic block types to see if it properly returns the right pages.

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows the standards laid out in the front end playbook
  • Passes all existing automated tests
  • New functions include new tests
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged
  • Visually tested in supported browsers and devices
  • Project documentation has been updated
  • Reviewers requested with the Assignee tool ➡️

@sebworks
Copy link
Contributor

sebworks commented May 1, 2017

This looks good, the only problem I have is the initial loading time which can be slow.

@Scotchester
Copy link
Contributor

@chosak Any idea if it would be possible to extend this to search for blocks with a specific value in a particular field? For example, to find all link blobs where "Show links as button" is checked.

@chosak
Copy link
Member Author

chosak commented Jul 28, 2017

@Scotchester this PR is pretty out of date (and that kind of question should perhaps go on the repository instead of here), but that's a good question.

At first thought that'd be a bit trickier to do. The way this plugin works is that it just stores a list of pages and blocks (in this model) that's easy to query. Potentially you could do some two-stage filtering where first you find pages with a block and then filter by the block configuration.

While we're revisiting this, is there sufficient interest in this to warrant polishing up that other repo and trying to merge this in?

@Scotchester
Copy link
Contributor

Ah, I didn't look carefully enough at the OP to notice that it was using a different repo for the meat of the thing.

I think there is sufficient interest. I just used it successfully to test a simple search of what pages have 27/75 groups, in order to browse through them and see which were using the "Has rule" option and which weren't, which we wanted to do to consider whether or not to eliminate that option in #3181.

@chosak
Copy link
Member Author

chosak commented Sep 8, 2017

@sebworks @Scotchester @willbarton this is ready for review again; I've released a new 0.2 release of this project that adds a few things that were missing (README, migrations, better tests) and also fixes some minor issues with the logic.

The testing instructions above still apply. Assuming you have a production database locally and are checking out this branch:

  1. pip install -r requirements/local.txt (installs the package)
  2. cfgov/manage.py migrate wagtailinventory (adds DB table)
  3. cfgov/manage.py block_inventory (fills in DB table with current pages/blocks)

Subsequent page saves/updates should update the DB table properly as changes are made.

There's still much that could be done to make this package better but it might be a nice addition to our project. What would you think about including it, even at this early stage?

@chosak chosak removed the hold label Sep 8, 2017
Copy link
Contributor

@Scotchester Scotchester left a comment

Choose a reason for hiding this comment

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

This works great, and I'd love to have it available to us in production.

@schaferjh
Copy link
Member

I like the concept! @chosak what about calling this "Block search" or "Block inventory search" to be more explicit about the functionality? "Code block search"? I would like the label to signal the functionality, i.e. that this isn't just a static inventory list -- you have to be looking for something specific.

@chosak
Copy link
Member Author

chosak commented Sep 22, 2017

@schaferjh that's an interesting suggestion. I like where you're headed with it. The only concern I have is consistency with the built-in Wagtail menu items, which look like this:

image

These are all nouns, as opposed to names of actions. Perhaps something like "Blocks" would be better? In the interest of merging this dormant PR in, I'm going to merge in the current version - let's keep that in mind as one of a long list of improvements to wagtail-inventory to make in future.

@schaferjh
Copy link
Member

@chosak "Block search" seems to fit the space just fine. It's just -- unlike "Users" -- where you're presented with a list and search ability, this presents users with search, no list. (I understand, the inventory is under the hood....)

@chosak chosak merged commit b7d82f0 into master Sep 22, 2017
@chosak chosak deleted the wagtail-inventory branch September 22, 2017 00:33
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.

4 participants