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.

Sign up for GitHub

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

Jump to bottom

add auto-generated timestamp and sent_at (ruby-sdk) #97

Merged
merged 7 commits into from
Jan 12, 2018
Merged

add auto-generated timestamp and sent_at (ruby-sdk) #97

merged 7 commits into from
Jan 12, 2018

Conversation

bartes
Copy link
Contributor

@bartes bartes commented Jan 10, 2018 •
edited
Loading

related https://github.com/castle/api/issues/1753

  • replaced with_context with context_sanitizer class
  • made hash options, params immutable
  • added auto-generating timestamp when creating client from request
  • added to_options method which automatically adds timestamp to options
  • added possibility to either add timestamp to client options or pass it along with the query params

@bartes bartes added the wip label Jan 10, 2018
@bartes bartes force-pushed the created_at branch 6 times, most recently from bcb72dc to 5a23ada Compare January 10, 2018 02:49
@bartes bartes removed the wip label Jan 10, 2018
@bartes bartes requested review from lluft and wallin January 10, 2018 02:51
@nijikon nijikon self-requested a review January 10, 2018 09:41
lluft
lluft suggested changes Jan 10, 2018
View reviewed changes
lib/castle/client.rb Outdated
def to_context(request, options = {})
default_context = Castle::DefaultContext.new(request, options[:cookies]).call
Castle::ContextMerger.new(default_context).call(options[:context] || {})
def to_context(request, context_options = {})
Copy link
Contributor

Choose a reason for hiding this comment

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

why renaming to context_options, I think if any I would call it additional_context see other methods

lib/castle/client.rb Outdated
end

def to_options(options = {})
options[:timestamp] ||= Time.now.iso8601
Copy link
Contributor

Choose a reason for hiding this comment

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

use Time.now.utc.iso8601(3)

lib/castle/client.rb Outdated
@@ -25,6 +34,7 @@ def authenticate(options = {})
options = Castle::Utils.deep_symbolize_keys(options || {})

if tracked?
options[:timestamp] = @timestamp if @timestamp
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure if we should allow it for authenticate thoughts @wallin?

lib/castle/client.rb Outdated
@@ -40,6 +50,7 @@ def identify(options = {})
options = Castle::Utils.deep_symbolize_keys(options || {})

return unless tracked?
options[:timestamp] = @timestamp if @timestamp
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd change it for all methods to options[:timestamp] ||= that way you can also pass in a timestamp to identify/track

lib/castle/commands/authenticate.rb Outdated

Castle::Command.new('authenticate', options, :post)
Castle::Command.new('authenticate',
options.merge(context: context, sent_at: Time.now.iso8601),
Copy link
Contributor

Choose a reason for hiding this comment

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

Time.now.utc.is8601(3) also shouldn't this be the responsibility of Castle::Command will DRY it up and add responsibility to the Object making the request

lib/castle/commands/identify.rb Outdated

Castle::Command.new('identify', options, :post)
Castle::Command.new('identify',
options.merge(context: context, sent_at: Time.now.iso8601),
Copy link
Contributor

Choose a reason for hiding this comment

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

see comment on authenticate

lib/castle/commands/track.rb Outdated

Castle::Command.new('track', options, :post)
Castle::Command.new('track',
options.merge(context: context, sent_at: Time.now.iso8601),
Copy link
Contributor

Choose a reason for hiding this comment

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

see comment on authenticate

@bartes
Copy link
Contributor Author

bartes commented Jan 10, 2018

@lluft thanks - corrected

@bartes
Copy link
Contributor Author

bartes commented Jan 11, 2018 •
edited
Loading

@wallin mentioned that we don't need to send it for identify, but anyway we are setting that value for created_at so it is better to set it in the right way

@bartes bartes changed the title added auto-generated timestamp and sent_at added auto-generated timestamp and sent_at (ruby-sdk) Jan 12, 2018
@bartes bartes changed the title added auto-generated timestamp and sent_at (ruby-sdk) add auto-generated timestamp and sent_at (ruby-sdk) Jan 12, 2018
@bartes bartes merged commit dc7cb87 into master Jan 12, 2018
@bartes bartes deleted the created_at branch January 12, 2018 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lluft lluft lluft approved these changes

@wallin wallin Awaiting requested review from wallin

@nijikon nijikon Awaiting requested review from nijikon

Assignees

@bartes bartes

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bartes @lluft