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

when remove_scope_all, the first load is not filtering by the first scope #2402

Closed
4 of 11 tasks
jetienne opened this issue Jan 23, 2024 · 11 comments · Fixed by #3081
Closed
4 of 11 tasks

when remove_scope_all, the first load is not filtering by the first scope #2402

jetienne opened this issue Jan 23, 2024 · 11 comments · Fixed by #3081
Assignees
Labels
Enhancement Not necessarily a feature, but something has improved Help wanted We could use some help with this Task Something to get done

Comments

@jetienne
Copy link

Describe the bug

On a given resource deals, add some scopes like:

  def scopes
    remove_scope_all
    scope Avo::Scopes::DealsMine
    scope Avo::Scopes::DealsEnquiry
    scope Avo::Scopes::DealsProposal
    scope Avo::Scopes::DealsSignedAndMore
    scope Avo::Scopes::DealsCanceled
  end

When accessing the URL /admin/resources/deals, the content initially appears unfiltered. However, selecting the first tab refreshes the page and correctly applies the filters to the displayed data.

Expected behavior & Actual behavior

When all scope is removed, the first scope is correctly being displayed.

System configuration

Avo version:
3.3.0

Rails version:
7.1.3

Ruby version:
3.3.0

License type:

  • Community
  • Pro
  • Advanced

Are you using Avo monkey patches, overriding views or view components?

  • Yes. If so, please post code samples.
  • No

Screenshots or screen recordings

https://www.loom.com/share/8c60793f3815495bb7d431a284325002?sid=3eba226a-c7c6-4f14-9cdc-e9e3ee9e322b

Impact

  • High impact (It makes my app un-usable.)
  • Medium impact (I'm annoyed, but I'll live.)
  • Low impact (It's really a tiny thing that I could live with.)

Urgency

  • High urgency (I can't continue development without it.)
  • Medium urgency (I found a workaround, but I'd love to have it fixed.)
  • Low urgency (It can wait. I just wanted you to know about it.)
@Paul-Bob
Copy link
Contributor

Hello @jetienne that is indeed the wanted and expected behavior, there isn't a way yet to apply a default scope and the all scope is there by default to clean applied scopes.

We debated it when implemented remove_scope_all in this issue: #2276

Possible APIS to be implemented:

def scopes
  remove_scope_all
  scope Avo::Scopes::CustomAll
  default_scope Avo::Scopes::Admins
end
def scopes
  remove_scope_all
  scope Avo::Scopes::CustomAll
  scope Avo::Scopes::Admins, default: -> { true }
end

As a possible workaround for now you can add params on the menu link something like scope=Avo%3A%3AScopes%3A%3AAdmins will apply the Avo::Scopes::Admins when clicked from that link.

@jetienne
Copy link
Author

@Paul-Bob Thanks. But what's the point of hiding a tab (All) if you still display the content in it ? This super counter-intuitive for the users!

IMHO, default: -> { true } is the way to go, since you can have some dynamic default-ing based on some context...

@jetienne
Copy link
Author

@Paul-Bob I tried your workaround, works like a charm. I would not even call it a workaround IMHO, it's just the way to set the default scope... :D

@Paul-Bob
Copy link
Contributor

@jetienne I called it workaround because it works only when that link is clicked, for example when the table is rendered on a association context ( i.e. has_many) it will not respect the "default scope".

I'm also inclined to the default: -> { true } solution, @adrianthedev WDYT?

@adrianthedev adrianthedev added the Enhancement Not necessarily a feature, but something has improved label Jan 23, 2024
@adrianthedev
Copy link
Collaborator

Related comment: #2276 (comment)

@adrianthedev
Copy link
Collaborator

I see the need for some dynamic way to set and unset the default scope.
I wonder if instead of doing default: -> { } we could just do default_scope SCOPE_CLASS.

I mean

# this looks better
if current_user.admin?
  default_scope CLASS
else
  default_scope ANOTHER_CLASS
end

# than this
scope CLASS, default: -> { current_user.admin? }
scope ANOTHER_CLASS, default: -> { !current_user.admin? }

We're just choosing one.

@Paul-Bob
Copy link
Contributor

Let's do a quick analyze to the above snippet

scope CLASS, default: -> { current_user.admin? }
scope ANOTHER_CLASS, default: -> { !current_user.admin? }

Result: With this API when user is admin, CLASS will be the default scope but ANOTHER_CLASS will still there as not selected.

if current_user.admin?
  default_scope CLASS
else
  default_scope ANOTHER_CLASS
end

Result: With this API when user is admin, CLASS will be the default and only present scope, ANOTHER_CLASS would not even be visible since was declared on the else that is never executed.

This is weird IMHO, if you want always both scopes but different default you would need to do something like:

if current_user.admin?
  default_scope CLASS
  scope ANOTHER_CLASS
else
  default_scope ANOTHER_CLASS
  scope CLASS
end

From a distant perspective none are great. One makes you duplicate and invert the logic inside default block and the other one makes you duplicate and invert the scopes definition. Here is something crazy:

def scopes
  scope CLASS
  scope ANOTHER_CLASS
  scope ONE_MORE_CLASS
end

# Called to define the default scope class
self.default_scope = -> do
  if current_user.admin?
    CLASS
  else
    ANOTHER_CLASS
  end
end

One last thought: It seems to me that people would expect the first scope as always the default one after removing the all scope (this approach would be the less flexible)

@jcn
Copy link

jcn commented Jan 26, 2024

One last thought: It seems to me that people would expect the first scope as always the default one after removing the all scope (this approach would be the less flexible)

Yes, in the absence of any other default, I would expect the first scope listed to be the default when landing on the resource index page.

@adrianthedev
Copy link
Collaborator

Yes. That's my expectation as well.
We'll work on adding a default.

@adrianthedev adrianthedev added Help wanted We could use some help with this Task Something to get done labels Jan 28, 2024
@adrianthedev
Copy link
Collaborator

Another nice to have would be to have the first scope applied without changing the URL.

So if Admin scope is the default one, /avo/resources/users would have a query with the active scope applied and it highlighted in the UI.

@Paul-Bob
Copy link
Contributor

Paul-Bob commented Mar 12, 2024

Ensure that if default scope visible block return true page loads without any scope: #2587

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Not necessarily a feature, but something has improved Help wanted We could use some help with this Task Something to get done
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants