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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add search feature #789

Merged
merged 94 commits into from Jan 22, 2018

Conversation

@JackMorganNZ
Member

JackMorganNZ commented Jan 19, 2018

This pull request adds search functionality to the website. There is some tiny changes still to make before this is completely finished, but these can be made alongside changes based off code reviews.

Fixes #264.

Features added

  • Able to search by text.
  • Able to filter search by model type.
  • Able to filter search by curriculum areas.
  • Quick search in navigation bar.

Changes made

  • Originally when the user filtered by the curriculum areas, there was confusion when all child curriculum areas are selected that it would not include items only connected to the parent. Therefore changes have been made to prevent any items being connected to any curriculum areas with children.
    • Loaders have been updated.
    • Content has been updated.
  • Bootstrap is updated to 4.0 (was required for fix to UI).

Final changes to make

  • Update documentation on curriculum area changes.
  • Remove filter from model filter widget.
  • Fix bug when viewing results, changing form, then clicking page navigation will change results based off new form.
  • Add areas to indexed text.

Issues to create after PR is merged

  • Add autocomplete prompt using Ngram index and AJAX.
  • Lower SQL queries on results.
  • Add internationalisation support.
  • Add in-browser tests #191
  • Add ability to search for general application pages.
  • Fork https://github.com/wenzhixin/multiple-select to own organisation for enhancements, including:
    • Improve styling of multiple select widget and add overflow indicator.
    • If possible, allow removal of items from dropdown window.
    • Add ability to search for optgroup (see here)

Addressed feedback

Fixed by hiding <select> elements, though due to using a JS solution, the filters might not appear for a second. However the flicker switch should not occur.

  • Maybe a bit more padding required around the text in the filter drop downs where it shows the selected values, to match the search bar and also to give some white space around the coloured curric areas.

Was definitely planned and improved now. Could be improved more in a new PR if needed.

  • In the table for the results, it's not quite clear what the bread crumbs mean (e.g. Binary numbers > Binary numbers or Searching algorithms > Sequential and binary search). Are these Topic > Unit Plan ? I'm not sure it's obvious enough, I know csu reasonably well and it stumped me for a moment. The results table's first column gives the "type" (e.g. Lesson) so maybe it makes sense to also give the terms "Unit Plan" and "Topic" ? Trying to think about someone new to unplugged, there is no information anywhere to tell them what the breadcrumbs are referring to.

Added context to the breadcrumbs.

  • A button to clear all filters would be helpful

Added clear form button.

  • After playing around with searching different things and filters and what not, I noticed that the width of the columns in the search table seem to change (a noticeable amount apparently), what causes this and can they be static for my page size rather than dynamic for my search results?

Fixed.

  • "Search" is in the title, breadcrumbs, text entry label, and on a button, that's a lot of places in close proximity, can it be removed as the field label and instead some placeholder text can be in the text entry field? (some generic "type here text" e.g. what are you looking for? or something ...prompty... of the like). This also allows the search field to cover 100% of the content area of the page, which is good 馃憤 as it helps to make it the main focus

This is good for accessibility, however I removed the breadcrumbs as they are not needed for this page. I also added placeholder. Keen to discuss this more if you think it requires it.

  • "Previous page" - what happens if I was super committed and searched all the way to page 5 then realised I actually wanted what was on page 1 because our search engine is the best thing ever and got it right first time? I have to click it 4 times? Can there be a shortcut to go back to the first page of results (effectively the same as doing the search again, but think of links on pages like "Go to top of page")

Added better page navigation.

After you search something, when you clear form/change the filters or what's in the search bar and then click on next page or any other number in the pagination, it searches whatever is in the form currently (be it empty or something different to the original search term) rather than the original search terms.

Fixed.

"All selected" could be the initial option, so people don't have to feel like they have to click on every checkbox to search everything.

Added placeholder text.

An 'x' after each label on the filter bar could make it easier for users to quickly and easily delete the filters that they don't want anymore rather than go through the list and going through and unchecking the box for it.

Will be added in fork of https://github.com/wenzhixin/multiple-select.

The filter bar could expand vertically as the number of filters checked increases. This improves visibility for the user.

Will be added in fork of https://github.com/wenzhixin/multiple-select.

JackMorganNZ added some commits May 4, 2017

Add curriculum areas filter
Currently the given filter is provided as a QuerySet, but the search
index saves the curriculum areas for objects (currently only
curriculum integrations) as a list of primary keys, but stored as
strings rather than integers. This may be due to the way data is
stored.

Because of this, the logic below must covert the QuerySet of the
filter into a list of primary key strings.

I also could not get Django/Haystack to filter by checking if any
item in the filter was any item in each object in the index.
Something similar to the following:
search_query_set = search_query_set.filter(
   curriculum_areas__in=self.cleaned_data["curriculum_areas"]
)
Add custom HTML for search results
Should look into lowering SQL queries to 1 per
result object, by making queries in context method.

JackMorganNZ added some commits Jan 20, 2018

@JackMorganNZ JackMorganNZ requested a review from saranglove Jan 21, 2018

@saranglove

Bug:
After you search something, when you clear form/change the filters or what's in the search bar and then click on next page or any other number in the pagination, it searches whatever is in the form currently (be it empty or something different to the original search term) rather than the original search terms. Hard to explain but I showed @JackMorganNZ in person and now he is sad.

Suggestions:

  1. "All selected" could be the initial option, so people don't have to feel like they have to click on every checkbox to search everything.

  2. An 'x' after each label on the filter bar could make it easier for users to quickly and easily delete the filters that they don't want anymore rather than go through the list and going through and unchecking the box for it.

  3. The filter bar could expand vertically as the number of filters checked increases. This improves visibility for the user.

JackMorganNZ added some commits Jan 22, 2018

Fix bug where switching search result pages used current form
Now the last submitted form is used for
page navigation.
@hayleyavw

This comment has been minimized.

Member

hayleyavw commented Jan 22, 2018

UI looks GREAT, the page navigation for "go to page: 1 2 3" etc however is a bit broken, I was on page 5 and clicked 1 and it took me to 4, then I clicked page 1 again and the url took me to 5 but it gave me a broken template.

JackMorganNZ added some commits Jan 22, 2018

Add text of curriculum areas to related model indexes
This allows items to be searched through the text field
</form>
{% else %}
{# Show some example queries to run, maybe query syntax, something else? #}

This comment has been minimized.

@hayleyavw

hayleyavw Jan 22, 2018

Member

should this be addressed?....

self.test_data = TopicsTestDataGenerator()
def test_rebuild_index_command_no_items(self):
management.call_command("rebuild_index", "--noinput")

This comment has been minimized.

@hayleyavw

hayleyavw Jan 22, 2018

Member

should these methods be asserting something? how does this magic work?

This comment has been minimized.

@JackMorganNZ

JackMorganNZ Jan 22, 2018

Member

Address via Slack. Is management called for building search index.

@@ -86,6 +86,16 @@ def test_curriculum_areas_undefined(self):
lo_loader.load
)
def test_parent_curriculum_area_raises_exception(self):

This comment has been minimized.

@hayleyavw

hayleyavw Jan 22, 2018

Member

can this be more descriptive? under what condition does it throw an area?

@JackMorganNZ

This comment has been minimized.

Member

JackMorganNZ commented Jan 22, 2018

All final fixes are done and I've updated description regarding latest comments. Now ready for testing and review changes.

JackMorganNZ added some commits Jan 22, 2018

@JackMorganNZ JackMorganNZ merged commit 308123d into develop Jan 22, 2018

4 checks passed

codecov/patch 100% of diff hit (target 99.47%)
Details
codecov/project 99.5% (+0.03%) compared to 2b72276
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@JackMorganNZ JackMorganNZ deleted the issue/264 branch Jan 22, 2018

@JackMorganNZ JackMorganNZ referenced this pull request Feb 5, 2018

Merged

Release 4.0.0 #852

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment