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

Search updates #8570

Merged
Merged

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Nov 1, 2021

Summary

  • Bootstraps all available channel data into the learn page
  • Uses this throughout the learn plugin to avoid API calls
  • Creates a composable helper for search functionality
  • Creates a SearchChips component to display and translate currently selected search terms
  • Adds a route for searching within topics and uses route state for most behaviour on topics page
  • Gets rid of TOPICS_CHANNEL route in favour of just using TOPICS_TOPIC
  • Fixes some issues with breadcrumbs
  • Makes search in topics constrained to descendants of the currently navigated to topic
  • Deletes unused code that referenced the now non-existent TOPICS_ROOT and SEARCH routes
  • Upgrades kolibri-constants and adds additional unhandled translations to core translator

References

Fixes #8565
Fixes #8555
Fixes #8556
Fixes #8557
Also resolves some TODOs from #8563:

  • Remove breadcrumbs for search
  • Remove "channels" breadcrumb (although changes to Library - maybe should remove completely?)

Reviewer guidance

search


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@rtibbles rtibbles added the TODO: needs review Waiting for review label Nov 1, 2021
@rtibbles rtibbles added this to the 0.15.0 milestone Nov 1, 2021
MisRob
MisRob previously requested changes Nov 1, 2021
Copy link
Member

@MisRob MisRob left a comment

Choose a reason for hiding this comment

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

Thank you, @rtibbles. I haven't managed to understand and review all parts of this PR yet. I added some comments to parts that I went through in more detail and I also see the following issues:

1. I can't remove search by clicking on the delete button in a search chip

bug-chip

2. Navigation from a channel card to a channel page on the home page stopped working
bug-navigation

3. Home page and classes pages content area is wider, is that change intentional?

Before After
classes-page-before classes-page-after
class-page-before class-page-after
home-page-before home-page-after

@rtibbles
Copy link
Member Author

rtibbles commented Nov 1, 2021

Thanks @MisRob:

  1. I can't remove search by clicking on the delete button in a search chip

Looks like this was specific to key word searches only. Was unable to replicate with a non-keyword search item.

  1. Navigation from a channel card to a channel page on the home page stopped working

This is probably related to the route simplification, will fix.

  1. Home page and classes pages content area is wider, is that change intentional?

This was not intentional - it's the result of slightly overzealous LearnIndex cleanup. Will revert.

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

I am going to do a closer code read-through (I've done a quick pass to understand the general changes). Overall the changes look good, although I appreciate that @MisRob is able to weigh in on the composables because I'm not sure I would things there.

The one bug I've noticed in manual QA is that the options for the filter options persist the previous state after clearing the search:
filters-persist

Will update with additional questions/comments this afternoon after a closer code review.

@rtibbles
Copy link
Member Author

rtibbles commented Nov 1, 2021

Ah yes - good catch, can clear labels too on search change.

Remove width setting only used by non-existent route.
Remove TOPICS_CHANNEL and TOPICS_ROOT references.
Fix JS tests.
Remove breadcrumbs reference to Library.
@rtibbles
Copy link
Member Author

rtibbles commented Nov 1, 2021

Resolved review feedback, except for adding tests for the search composable.

@rtibbles
Copy link
Member Author

rtibbles commented Nov 2, 2021

Tests for search composable have now been added.

@marcellamaki
Copy link
Member

Seems like feedback has been addressed, although I know @jtamiace sent some feedback about the homepage -- not sure if that will be included in this PR or not. Otherwise, the only thing I see is this "extra chip" popping up. Seems like one empty one is appended to the end of the list of filters.

Screen Shot 2021-11-02 at 10 40 38 AM

@rtibbles
Copy link
Member Author

rtibbles commented Nov 2, 2021

not sure if that will be included in this PR or not

I think best to do as a follow up.

The extra chip is probably coming from an empty keywords parameter, will update.

@rtibbles
Copy link
Member Author

rtibbles commented Nov 2, 2021

The extra chip is probably coming from an empty keywords parameter, will update.

Replicated and updated.

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Code look good. I really love seeing the composition API stuff in action and seeing it mixed with Vuex stuff - love it. I didn't see anything that jumped out at me.

I found a bug in the keywords processing in Python which is that it fails with the following queries, separated by line:

10
100
hello 10
hello 100
10 hello
10 x 10
10 10 10
100 10 100
or
and or

These are all unlikely queries, of course, but they may signal that there are more that I'm not finding. I wouldn't block merging on this but it's worth following up on, perhaps after bug bash to see if people run into any more normal queries that return 500.

The server gives a message from Django that it needs to be valid JSON (traceback).

In any case - the fail state there when it gets the 500 is to show a spinner forever so that could use some better error handling for the UX - maybe just showing 0 results along with some existing "we encountered an error" strings. The user can return to normal by clearing the filters that appear once you click something in the side bar.

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Oh just noticed this as I was wrapping up but the X shows in the search bar when you haven't searched any kind of text query.

So - I click "Read" and the X shows in both searches (top bar and left nav).

When I click it, nothing happens. This is actually a bug altogether - even when I type text in and click the X to clear the search, it doesn't have any effect on the chips or query results.

If this isn't in scope I can make sure to create a follow up issue.

@rtibbles rtibbles mentioned this pull request Nov 2, 2021
9 tasks
@rtibbles
Copy link
Member Author

rtibbles commented Nov 2, 2021

Fixed the 500 in #8579

Have fixed the search box X issue:

Screenshot from 2021-11-02 14-39-45

Screenshot from 2021-11-02 14-39-50

@marcellamaki marcellamaki dismissed MisRob’s stale review November 2, 2021 22:05

Requested changes have been added

@marcellamaki marcellamaki merged commit 72e06e2 into learningequality:release-v0.15.x Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
4 participants