Skip to content

Conversation

@swanny85
Copy link
Contributor

Fulfil requires dates and datetimes to be formatted in an object that contains a iso8601 datetime. This PR adds a conversion class and integrates it into the model to update the domains automatically.

@swanny85 swanny85 self-assigned this Dec 21, 2023
@swanny85
Copy link
Contributor Author

I'll fix the reeks.

@swanny85 swanny85 requested a review from cdmwebs December 21, 2023 19:29
@cdmwebs
Copy link
Member

cdmwebs commented Dec 21, 2023

@cdmwebs
Copy link
Member

cdmwebs commented Dec 21, 2023

Actually, let's talk through this. We're already doing some of this in another spot.

end

def disable_escape_html_entities
return unless defined?(ActiveSupport) && ActiveSupport.respond_to?(:escape_html_entities_in_json=)

Choose a reason for hiding this comment

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

Fulfil::DomainParser#disable_escape_html_entities manually dispatches method call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cdmwebs rake test was erroring out when my guard clause was only return unless defined?(ActiveSupport) not sure exactly what to do to fix this codeclimate reek.

end

def enable_escape_html_entities
return unless defined?(ActiveSupport) && ActiveSupport.respond_to?(:escape_html_entities_in_json=)

Choose a reason for hiding this comment

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

Fulfil::DomainParser#enable_escape_html_entities manually dispatches method call


private

def date_as_object(date)

Choose a reason for hiding this comment

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

Fulfil::DomainParser#date_as_object doesn't depend on instance state (maybe move it to another class?)

Converter.date_as_object(date)
end

def datetime_as_object(datetime)

Choose a reason for hiding this comment

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

Fulfil::DomainParser#datetime_as_object doesn't depend on instance state (maybe move it to another class?)

Converter.datetime_as_object(datetime)
end

def disable_escape_html_entities

Choose a reason for hiding this comment

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

Fulfil::DomainParser#disable_escape_html_entities doesn't depend on instance state (maybe move it to another class?)

ActiveSupport.escape_html_entities_in_json = false
end

def enable_escape_html_entities

Choose a reason for hiding this comment

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

Fulfil::DomainParser#enable_escape_html_entities doesn't depend on instance state (maybe move it to another class?)

end

def handle_hash(field, key, value, options)
if %i[gte gt lte lt].any? { |op| value.key?(op) }

Choose a reason for hiding this comment

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

Fulfil::Query#handle_hash refers to 'value' more than self (maybe move it to another class?)

end
end

def handle_hash(field, key, value, options)

Choose a reason for hiding this comment

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

Fulfil::Query#handle_hash has 4 parameters

end
end

def handle_hash(field, key, value, options)

Choose a reason for hiding this comment

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

Fulfil::Query#handle_hash has approx 7 statements

@swanny85
Copy link
Contributor Author

swanny85 commented Jan 4, 2024

Figuring out how to fix these codeclimate reeks. I've got dates in the queries. Do those look right @cdmwebs?


module Fulfil
module Concerns
module HtmlEntityHandler

Choose a reason for hiding this comment

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

Fulfil::Concerns::HtmlEntityHandler has no descriptive comment

end

def disable_escape_html_entities
return unless defined?(ActiveSupport) && ActiveSupport.respond_to?(:escape_html_entities_in_json=)

Choose a reason for hiding this comment

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

Fulfil::Concerns::HtmlEntityHandler#disable_escape_html_entities manually dispatches method call

end

def enable_escape_html_entities
return unless defined?(ActiveSupport) && ActiveSupport.respond_to?(:escape_html_entities_in_json=)

Choose a reason for hiding this comment

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

Fulfil::Concerns::HtmlEntityHandler#enable_escape_html_entities manually dispatches method call

enable_escape_html_entities
end

def disable_escape_html_entities

Choose a reason for hiding this comment

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

Fulfil::Concerns::HtmlEntityHandler#disable_escape_html_entities doesn't depend on instance state (maybe move it to another class?)

ActiveSupport.escape_html_entities_in_json = false
end

def enable_escape_html_entities

Choose a reason for hiding this comment

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

Fulfil::Concerns::HtmlEntityHandler#enable_escape_html_entities doesn't depend on instance state (maybe move it to another class?)

end
end

def handle_operator_comparison(key, value)

Choose a reason for hiding this comment

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

Fulfil::Query#handle_operator_comparison doesn't depend on instance state (maybe move it to another class?)

@qlty-cloud-legacy
Copy link

Code Climate has analyzed commit 857569a and detected 9 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 9

The test coverage on the diff in this pull request is 96.6% (50% is the threshold).

This pull request will bring the total coverage in the repository to 88.9% (1.2% change).

View more on Code Climate.

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.

3 participants