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

Hotfix: Restore intended behaviour in media search #504

Closed
wants to merge 1 commit into from

Conversation

fosterfarrell9
Copy link
Collaborator

@fosterfarrell9 fosterfarrell9 commented May 13, 2023

As described in #496, PR #420 has introduced some unwanted behaviour (that also leads to exceptions) when the media search was made accesible to generic users via the start page. In this PR the following things have been changed

  • In the 'Import Content' tab in the 'Create vertex' dialogue for quizzes, there is no more radio button that allows you to select all types of media. You can only choose between Questions and Remarks. (This restores previous behaviour)
  • In the 'Media Search' Tab that is available to editors in the admin area, it is now also possible to select media of type Questions/Remark/Erdbeere. (This restores previous behaviour)
  • In the media search form on the start page, checking the 'all types' radio button now means checking all types of media actually listed in this generic search form, which excludes e.g. Questions. For generic users these types have been filtered out anyway if you selected 'all', but for admins they were not, creating an inconsistency. I think that the exclusion of certain types makes sense in that form, because the results are rendered in a way that is not suitable for huge amounts of hits. If you are an editor/admin and want to search Questions, you can use the search form in the admin area. Maybe it would make sense to add a helpdesk in this accordion fold for editors that explains that this media search is kind of restricted and that it is recommended to use the media search in the admin area. (This removes an inconsistency)

@fosterfarrell9 fosterfarrell9 self-assigned this May 13, 2023
@codecov
Copy link

codecov bot commented May 13, 2023

Codecov Report

Merging #504 (0b81084) into main (f6dd9d2) will increase coverage by 0.01%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##             main     #504      +/-   ##
==========================================
+ Coverage   66.64%   66.65%   +0.01%     
==========================================
  Files         311      311              
  Lines        9362     9366       +4     
==========================================
+ Hits         6239     6243       +4     
  Misses       3123     3123              
Impacted Files Coverage Δ
app/controllers/media_controller.rb 26.00% <ø> (ø)
app/helpers/media_helper.rb 32.89% <0.00%> (ø)
app/models/medium.rb 55.32% <87.50%> (+0.12%) ⬆️

... and 1 file with indirect coverage changes

@fosterfarrell9
Copy link
Collaborator Author

I am not sure if this really falls in the 'hotfix' category despite the exceptions it triggers; I mean, nobody has complained about it. So we could also rebase to mampf-next if that fits better.

@fosterfarrell9
Copy link
Collaborator Author

I think this is better as a fix to be merged into mampf-next instead of a hotfix to main, so I close this in favor of #506.

@Splines
Copy link
Member

Splines commented May 15, 2023

@fosterfarrell9 You could have just changed the base branch by clicking on "Edit" above.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants