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

Add the ability to remove the "All" default scope #2276

Closed
3 tasks done
adrianthedev opened this issue Dec 21, 2023 · 6 comments · Fixed by #2323
Closed
3 tasks done

Add the ability to remove the "All" default scope #2276

adrianthedev opened this issue Dec 21, 2023 · 6 comments · Fixed by #2323
Assignees

Comments

@adrianthedev
Copy link
Collaborator

adrianthedev commented Dec 21, 2023

Let's just do remove_scope_all and leave default_scope for later.

Tasks

@adrianthedev adrianthedev self-assigned this Dec 21, 2023
@Paul-Bob
Copy link
Contributor

From here https://discord.com/channels/740892036978442260/1189990451269341234 I think it makes sense to be able to show the count on the All scope.

  1. One possible approach would be to add a config on the resource display_all_scope = false and each user implement the All scope as desired.

  2. Another approach would be to remove the automatic All scope and who wants it manually add it

I think 1 is more friendly and 2 require less maintenance, we can also explore other approaches

@adrianthedev
Copy link
Collaborator Author

I was thinking about adding a custom method remove_scope_all that the user can call and add their own all scope that has the count.

def scopes
  remove_scope_all
  scope Avo::Scopes::CustomAll
  scope Avo::Scopes::Admins
end

With this API we could even configure a default scope form when a user click on the menu item.

def scopes
  remove_scope_all
  scope Avo::Scopes::CustomAll
  default_scope Avo::Scopes::Admins
end

The default_scope would be applied to the query without appending the scope param to the URL.

WDYT about the remove_scope_all approach?

@Paul-Bob
Copy link
Contributor

Great! Maybe scope Avo::Scopes::Admins, default: true instead default_scope Avo::Scopes::Admins? Just thinking about scalability and adding more options (if necessary) in the future

@adrianthedev
Copy link
Collaborator Author

Yeah, that could work too.

@Paul-Bob
Copy link
Contributor

Paul-Bob commented Jan 4, 2024

@adrianthedev the logic was implemented on the 2 linked PRs. I'll do docs and tests after PRs review on the logic.

Tests on CI will fail since it's testing on avo main branch. Tests should be run locally

@Paul-Bob
Copy link
Contributor

Paul-Bob commented Jan 4, 2024

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

Successfully merging a pull request may close this issue.

2 participants