Fix topics filtering by provider #148
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oli0li
force-pushed
the
fix-topics-sorted-by-provider
branch
2 times, most recently
from
2800332 to
0f6687d
Compare
Oli0li
force-pushed
the
fix-topics-sorted-by-provider
branch
from
0f6687d to
e969ece
Compare
|
@Oli0li please fix conflicts |
Oli0li
force-pushed
the
fix-topics-sorted-by-provider
branch
from
e969ece to
7c95492
Compare
Thank you @dmitrytrager! I just resolved the conflict and added a commit to fix a warning I noticed in the tests. |
app/controllers/topics_controller.rb
Outdated
| else | ||
| Current.user.topics | ||
| end.includes(:language) | ||
| @scope ||= current_provider&.topics&.includes(:language) || Topic.none |
There was a problem hiding this comment.
I like the idea of having Topic.none here.
What if we also define null object for provider and return it if there is no current provider.
We can define topics association for this null object and will return Topic.none there. Using this approach we will have just current_provider.topics.includes(:language) without &s. Wdyt?
There was a problem hiding this comment.
Thanks a lot @dmitrytrager! I hadn't used this pattern before, it's very interesting. I added a new commit to add this NullProvider, please let me know what you think.
For Admins as for Contributors, we want to only display the topics that belong to the first provider they are entitled to see, as this provider's name is displayed by default on the index page. Users are then able to see topics from other providers they are entitled to check by selecting them from the dropdown menu. The provider_id param is not part of the search form, it is part of the provider form in the choose_provider partial that allows us to set the current_provider_id cookie in SettingsController, so it wasn't being passed. We can ensure we see only relevant topics by redefining #scope, which will change according to the presence of current_provider_id cookie, instead of having a wider scope and then trying to check the params.
I think this method is currently unnecessary, as we are only showing topics scoped by provider. But if we wanted something like this, I would like to suggest using an association: has_many :topics, through: :providers" or, if we want to keep this as a method, change: Topic.where(provider_id: providers.pluck(:id)) to: Topic.where(provider_id: providers) as it would allow us to get the topics in 1 query instead of having 1 query for plucking the providers ids and then another for getting the topics.
This change fixes the "searching by tags" tests by addressing this warning: "warning: Locator Language:#<Language id: 448, name: "Basic", created_at: "2025-04-16 21:45:50.059603000 +0000", updated_at: "2025-04-16 21:45:50.059603000 +0000"> for selector :option must be an instance of String or Symbol or Integer. This will raise an error in a future version of Capybara."
This would be in case a user does not have a provider set up yet.
Oli0li
force-pushed
the
fix-topics-sorted-by-provider
branch
from
974c6b4 to
00d0c84
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What Issue Does This PR Cover, If Any?
Resolves #136
What Changed? And Why Did It Change?
It redefines the scope of Topics shown on the Topics page to ensure we only display Topics from the currently selected provider.
This fixes a bug that, for Admin users, caused all Topics to be displayed regardless of the Provider, even though the name of the first Provider's was displayed at the top and gave the impression that the Topics were already filtered by Provider.
How Has This Been Tested?
With system tests (spec/system/topic_search_spec.rb) and by hand
Please Provide Screenshots
Changing.provider.as.Admin.mp4
Additional Comments
After making this change, I saw some tests were failing because they are set up in a way where users who are Contributors have no Providers. So I made it so that Contributors can still access the "Topics" page even if they don't have any Providers, in case that's something that may happen when we first add users, but that means they wouldn't be able to see any Topics or create new ones. We could display a message such as "You haven't been added to a provider yet" on the Topics page to clarify the situation, or we could redirect them to the dashboard if they don't have a provider. I wasn't sure what the desired behaviour was, please let me know and I can make the changes.