avo icon indicating copy to clipboard operation
avo copied to clipboard

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

Open jetienne opened this issue 1 year ago • 11 comments

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
  • [x] Advanced

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

  • [ ] Yes. If so, please post code samples.
  • [x] 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.)
  • [x] 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.)
  • [x] 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.)

jetienne avatar Jan 23 '24 10:01 jetienne

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: https://github.com/avo-hq/avo/issues/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.

Paul-Bob avatar Jan 23 '24 10:01 Paul-Bob

@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 avatar Jan 23 '24 10:01 jetienne

@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

jetienne avatar Jan 23 '24 10:01 jetienne

@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?

Paul-Bob avatar Jan 23 '24 10:01 Paul-Bob

Related comment: https://github.com/avo-hq/avo/issues/2276#issuecomment-1872087497

adrianthedev avatar Jan 23 '24 20:01 adrianthedev

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.

adrianthedev avatar Jan 23 '24 20:01 adrianthedev

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)

Paul-Bob avatar Jan 24 '24 09:01 Paul-Bob

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.

jcn avatar Jan 26 '24 22:01 jcn

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

adrianthedev avatar Jan 26 '24 22:01 adrianthedev

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.

adrianthedev avatar Jan 28 '24 10:01 adrianthedev

Ensure that if default scope visible block return true page loads without any scope: https://github.com/avo-hq/avo/issues/2587

Paul-Bob avatar Mar 12 '24 17:03 Paul-Bob