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

fix: resource search #2656

Merged
merged 4 commits into from Apr 5, 2024
Merged

fix: resource search #2656

merged 4 commits into from Apr 5, 2024

Conversation

gabrielgiroe1
Copy link
Collaborator

@gabrielgiroe1 gabrielgiroe1 commented Apr 3, 2024

Description

Implement resource search functionality in cases where there are no existing policies defined for the resource.

Fixes https://discord.com/channels/740892036978442260/1223913103998193664

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Copy link

codeclimate bot commented Apr 3, 2024

Code Climate has analyzed commit 494aa59 and detected 0 issues on this pull request.

View more on Code Climate.

@@ -136,7 +136,8 @@ def show_search_input
def authorized_to_search?
# Hide the search if the authorization prevents it
return true unless resource.authorization.respond_to?(:has_action_method?)
return false unless resource.authorization.has_action_method?("search")
return true unless resource.authorization.has_action_method?("search")
return false unless resource.authorization.has_action_method?("avo_search?")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

has_action_method?(argument) will pick the argument and use it as key to access the Avo.configuration.authorization_methods hash that looks like:

      @authorization_methods = {
        index: "index?",
        show: "show?",
        edit: "edit?",
        new: "new?",
        update: "update?",
        create: "create?",
        destroy: "destroy?"
      }

avo_search? is something specific that the user can set like:

Avo.configure do |config|
  # ...

  ## == Authorization ==
  config.authorization_methods = {
    # ...
    search: "avo_search?" # override this method
  }

  # ...
end

It can be even can_search_or_not? if the user configure it with:

Avo.configure do |config|
  # ...

  ## == Authorization ==
  config.authorization_methods = {
    # ...
    search: "can_search_or_not?" # override this method
  }

  # ...
end

We should always access the search key to verify if the method exists.

Copy link
Contributor

@Paul-Bob Paul-Bob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @gabrielgiroe1 I let a comment about readability, otherwise the logic seems correct.

app/components/avo/views/resource_index_component.rb Outdated Show resolved Hide resolved
@Paul-Bob Paul-Bob changed the title Add conditional check in authorized_to_search? fix: resource search Apr 4, 2024
@Paul-Bob Paul-Bob added the Fix label Apr 4, 2024
Copy link
Contributor

@Paul-Bob Paul-Bob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gabrielgiroe1 just noticed that we can simplify the method.

Lets try this approach, I think this will have the desired output:

  def authorized_to_search?
    resource.authorization.authorize_action("search", raise_exception: false)
  end

Can you please check this scenarios?

  1. On avo, without avo-pro installed resource search should be working.
  2. On avo-pro with config.authorization_client = nil should be working.
  3. On avo-pro with pundit as client but without policy configured for the resource should be working.
  4. On avo-pro with pundit as client and without the search method defined should search should not work.
  5. On avo-pro with pundit as client and with the search method defined returning false should search should not work.
  6. On avo-pro with pundit as client and with the search method defined returning true search should work.

@gabrielgiroe1
Copy link
Collaborator Author

Yes, you are right. I tested it with these changes and it work as expected.

Copy link
Contributor

@Paul-Bob Paul-Bob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gabrielgiroe1 it's looking great!

@Paul-Bob Paul-Bob merged commit d689a82 into main Apr 5, 2024
19 of 20 checks passed
@Paul-Bob Paul-Bob deleted the show_resource_search branch April 5, 2024 07:55
Copy link
Contributor

github-actions bot commented Apr 5, 2024

This PR has been merged into main. The functionality will be available in the next release.

Please check the release guide for more information.

@Paul-Bob Paul-Bob mentioned this pull request Apr 18, 2024
4 tasks
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.

None yet

2 participants