Skip to content
This repository has been archived by the owner on Aug 21, 2023. It is now read-only.

Incremental activities #36

Merged
merged 5 commits into from
Dec 7, 2017
Merged

Incremental activities #36

merged 5 commits into from
Dec 7, 2017

Conversation

esjee
Copy link
Collaborator

@esjee esjee commented Nov 20, 2017

This is a feature based on the discussion in #25. Essentially, it allows you to fetch all activities of your program incrementally. The idea here is this acts like a stream; you start at one point and keep moving forward in time until you run out of activities. As mentioned in #25, this feature has multiple usages:

  • The ability to detect new activity on a report or a new report coming in using a single endpoint.
  • Being able to take actions on reports based on user activity. Automatically assign after triaging, for example.
  • Monitor activities on program, acting like an audit log.

The code is still a bit rough, but I envision it being used something like this:

program = HackerOne::Client::Program.find 'my_program'
updated_at_after = ... # load previous session in some fashion
incremental_activities = program.incremental_activities(updated_at_after: updated_at_after)

incremental_activities.traverse do |activity|
  # Loop through every single activity in order of creation
end

# Save progress in some fashion; any activities processed during this run will not be processed again

Before I go on writing tests, I'd be curious to see what you think of this feature, and if you have any suggestions on how to improve it.

@esjee esjee requested a review from oreoshake November 20, 2017 11:00
@page_size = page_size
end

def loop_through_activities
Copy link
Contributor

Choose a reason for hiding this comment

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

traverse is the word cool kids use

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😎

Copy link
Owner

Choose a reason for hiding this comment

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

flatmap :trollface:

Copy link
Contributor

Choose a reason for hiding this comment

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

@oreoshake wow, you're so cool

class Activities
include ResourceHelper

DOTFILE = '.hackerone_client_incremental_activities'.freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

Is your plan to store the latest fetched thing in this file? If so, that isn't going to work if you have more than one machine that uses this lib and I would let this up to the user how to store this data.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, that's the plan! We need some kind of persistent storage to store the updated_at_after value, which essentially acts like a cursor.

I think you have a point, let's remove this logic?

def loop_through_activities
load_dotfile

loop do
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't while next_page what you want?

@Willianvdv
Copy link
Contributor

Nice feature! If this works, we can remove a lot of request to our servers :D

Copy link
Owner

@oreoshake oreoshake left a comment

Choose a reason for hiding this comment

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

I'll take another pass over this but it I'm having some trouble tracing through the code (maybe I haven't had enough ☕

end

def current_page
@current_page ||= make_get_request(
Copy link
Owner

Choose a reason for hiding this comment

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

I think avoiding memoization here can clean up some of the state introduced by the ivars.

File.join(Dir.home, DOTFILE)
end

def next_updated_at_after
Copy link
Owner

Choose a reason for hiding this comment

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

This method name is a little hard to grok.

@@ -0,0 +1,101 @@
module HackerOne
Copy link
Owner

Choose a reason for hiding this comment

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

For this file in general, I think the tiny methods might work better as inline code with a comment rather than a separate one line method that's only called in one place. I think avoiding ivars in favor of locals will help readability. Double emphasis on think.

@esjee
Copy link
Collaborator Author

esjee commented Nov 24, 2017

I tried cleaning it up a bit. Is this helpful?

@oreoshake
Copy link
Owner

Looks like there's a merge conflict, otherwise 👍

@esjee
Copy link
Collaborator Author

esjee commented Nov 29, 2017

It appears there's a bit of an issue with the endpoint, in that it may get stuck repeating the last page infinitely. I'm hoping I'll be able to fix this, and I'll finish off this PR at that point.

@esjee
Copy link
Collaborator Author

esjee commented Dec 7, 2017

Alright! I'm happy with the PR in it's current state. Curious to know what you think!

@oreoshake oreoshake merged commit f0750d9 into oreoshake:master Dec 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants