Skip to content

Conversation

@takenory
Copy link
Collaborator

This is a feature addition to realize issue #30.

Copy link
Contributor

@ishikawa999 ishikawa999 left a comment

Choose a reason for hiding this comment

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

I have checked the code and verified that it works.
It looks good.

I especially liked the fact that having separate allowed_ips_with_comments and allowed_ips does not affect processing speed because there is no need to move the process of excluding comments when actually accessing the site.

Will changes to util.rake create another pull request?

@takenory
Copy link
Collaborator Author

Thanks for the review @ishikawa999.

Will changes to util.rake create another pull request?

I will add a commit to this PR.

@takenory
Copy link
Collaborator Author

@ishikawa999
Commited dcb5b0c . Please review it.

@ishikawa999
Copy link
Contributor

This is a feature addition to realize issue #30.

Sorry for the late review. I don't think there is any problem.

@ishikawa999 ishikawa999 removed their assignment Aug 28, 2023
@yoshiokaCB yoshiokaCB closed this Jan 15, 2024
@yoshiokaCB yoshiokaCB reopened this Jan 15, 2024
@ishikawa999 ishikawa999 requested review from maeda-m and removed request for vividtone January 15, 2024 06:43
Copy link

@maeda-m maeda-m left a comment

Choose a reason for hiding this comment

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

@takenory
Thank you for the pull request.

It's great that the 'Allowed IP addresses' becomes easier to manage with effective commenting. I have confirmed that it works in my environment below. I was able to register an example of #30 in the browser.

Environment:
  RedMica version     2.4.1.stable (based on Redmine 5.1.1.devel)
  Ruby version        3.2.2-p53 (2023-03-30) [x86_64-linux]
  Rails version       6.1.7.6
  Environment         development
  Database adapter    PostgreSQL

I confirmed that delete and show_with_comments in util.rake work even with comments.

That's fantastic 👍


I have one suggestion for util.rake. It would be great to have add_with_comments similar to show_with_comments for add. Having a CLI that can also add comments like the following would be convenient for me, as I won't need to launch a browser.

Usage:

$ bin/rails redmine_ip_filter:filters:add_with_comments ADDR_WITH_COMMENTS='
> # Our offices
> 192.0.2.1  # headquarter
> 192.0.2.100  # branch A
> 
> 198.51.100.21 # Foo Corporation
> '

Example implementation:

desc 'Add IP addresses to the allowed IP addresses with comments'
task :add_with_comments => :environment do
  filter_rule = FilterRule.find_or_default
  address_with_comments = ENV['ADDR_WITH_COMMENTS']
  if address_with_comments.blank?
    abort 'IP addresses to add must be specified with ADDR_WITH_COMMENTS environment variable'
  else
    filter_rule.allowed_ips = ([filter_rule.allowed_ips_with_comments, address_with_comments]).join("\n")
  end
  unless filter_rule.save
    STDERR.puts filter_rule.errors.messages[:base]
    exit 1
  end
  puts "ADD:\n#{address_with_comments}"
end

I would appreciate your opinion on this.

@maeda-m maeda-m self-requested a review February 5, 2024 02:07
Copy link

@maeda-m maeda-m left a comment

Choose a reason for hiding this comment

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

@takenory Thank you for the great PR.
I've confirmed that the feature to add comments works in RedMica v2.4.1, so it looks good to me.

The add_with_comments I suggested was about a different topic, so I wrote it in #42.
I apologize.

@takenory
Copy link
Collaborator Author

takenory commented Feb 5, 2024

Thanks to all the reviewers!
I'm merging this PR change.

@takenory takenory merged commit 0d24c38 into redmica:master Feb 5, 2024
@takenory takenory deleted the feature/enable_to_add_comments branch February 5, 2024 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants