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

Media search #420

Merged
merged 30 commits into from
Jan 18, 2023
Merged

Media search #420

merged 30 commits into from
Jan 18, 2023

Conversation

zebleck
Copy link
Collaborator

@zebleck zebleck commented Dec 1, 2022

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Generic user search for media (on start page)
Allows for different filtering criteria to be selected (tags, description, teacher, lecture, etc.)
Consistent with admin search if same criteria chosen
Also setup for request tests and implementation for media search
Fixes of some unrelated cypress tests

  • Please check if the PR fulfills these requirements
  • [-] E2E Tests for the changes have been added via Cypress
  • Meaningful rspec tests have been added
  • [-] Docs have been added / updated
  • What is the current behavior? (You can also link to an open issue here)

  • What is the new behavior (if this is a feature change)?

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

  • Other information:

zebleck and others added 28 commits July 19, 2022 14:35
option to change between list and card display
should not be accessible to normal user
removed "subscribed and preceding" option
better phone layout
using the "or" operator now executes two searches,
one where the tags are included and one where
they are ignored. they are then combined so
that those with the selected tags are shown first
in the results.
also fixed the search with the "and" operator.
-fix issues with wrong pagination due to the two concurrent medium searches
-also filter search results for user visibility if access search_parameter is left blank
request tests for media search
remove unnecessary testing gem
init helpers for request tests
Copy link
Collaborator

@fosterfarrell9 fosterfarrell9 left a comment

Choose a reason for hiding this comment

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

One other thing I noticed is that when you hit the back button of the browser after having performed a media search, you are shown the json data of the last search. This does not happen in the media search in the admin area.

@@ -207,17 +211,49 @@ def inspect
# return all media that match the search parameters
def search
authorize! :search, Medium.new

# get all media, then set them to only those that are visible to the current user
if search_params[:access].blank?
Copy link
Collaborator

Choose a reason for hiding this comment

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

If generic users manipulate their search form and add some non-blank entry for the access parameter, they will see unfiltered results, since the filtering is triggered by filter_media, see line 240. This should be prevented. I think they should always be filtered for non-gneric users.

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.

2 participants