Merge branch 'develop'

.gitlab-ci.yml

@@ -41,10 +41,8 @@ deploy_qa:
     - pip install ecs-deploy==1.11.0
     - aws ssm get-parameters-by-path --region $AWS_DEFAULT_REGION --path /qa/pender/ --recursive --with-decryption --output text --query "Parameters[].[Name]" | sed -E 's#/qa/pender/##' > env.qa.names
     - rm -f qa-pender-c.env.args; for NAME in `cat env.qa.names`; do echo -n "-s qa-pender-c $NAME /qa/pender/$NAME " >> qa-pender-c.env.args; done
-    - echo -n "-s qa-pender-c GITHUB_TOKEN arn:aws:secretsmanager:eu-west-1:848416313321:secret:GithubToken-Plain-BUhwIw" >> qa-pender-c.env.args
     - ecs deploy ecs-qa  qa-pender --image qa-pender-c $ECR_API_BASE_URL/qa/pender/api:$CI_COMMIT_SHA --exclusive-env -e qa-pender-c APP pender -e qa-pender-c DEPLOY_ENV qa -e qa-pender-c AWS_REGION $AWS_DEFAULT_REGION --timeout 3600 --exclusive-secrets `cat qa-pender-c.env.args`
     - rm -f qa-pender-background.env.args; for NAME in `cat env.qa.names`; do echo -n "-s qa-pender-background $NAME /qa/pender/$NAME " >> qa-pender-background.env.args; done
-    - echo -n "-s qa-pender-background GITHUB_TOKEN arn:aws:secretsmanager:eu-west-1:848416313321:secret:GithubToken-Plain-BUhwIw" >> qa-pender-background.env.args
     - ecs deploy ecs-qa  qa-pender-background --image qa-pender-background $ECR_API_BASE_URL/qa/pender/api:$CI_COMMIT_SHA --exclusive-env -e qa-pender-background APP pender -e qa-pender-background DEPLOY_ENV qa -e qa-pender-background AWS_REGION $AWS_DEFAULT_REGION --timeout 3600 --exclusive-secrets `cat qa-pender-background.env.args`
     - echo "new Image was deployed $ECR_API_BASE_URL/qa/pender/api:$CI_COMMIT_SHA"
   only:
@@ -90,10 +88,8 @@ deploy_live:
     - pip install ecs-deploy==1.11.0
     - aws ssm get-parameters-by-path --region $AWS_DEFAULT_REGION --path /live/pender/ --recursive --with-decryption --output text --query "Parameters[].[Name]" | sed -E 's#/live/pender/##' > env.live.names
     - rm -f live-pender-c.env.args; for NAME in `cat env.live.names`; do echo -n "-s live-pender-c $NAME /live/pender/$NAME " >> live-pender-c.env.args; done
-    - echo -n "-s live-pender-c GITHUB_TOKEN arn:aws:secretsmanager:eu-west-1:848416313321:secret:GithubToken-Plain-BUhwIw" >> live-pender-c.env.args
     - ecs deploy ecs-live  live-pender --image live-pender-c $ECR_API_BASE_URL/live/pender/api:$CI_COMMIT_SHA --exclusive-env -e live-pender-c APP pender -e live-pender-c DEPLOY_ENV live -e live-pender-c AWS_REGION $AWS_DEFAULT_REGION --timeout 3600 --exclusive-secrets `cat live-pender-c.env.args`
     - rm -f live-pender-background.env.args; for NAME in `cat env.live.names`; do echo -n "-s live-pender-background $NAME /live/pender/$NAME " >> live-pender-background.env.args; done
-    - echo -n "-s live-pender-background GITHUB_TOKEN arn:aws:secretsmanager:eu-west-1:848416313321:secret:GithubToken-Plain-BUhwIw" >> live-pender-background.env.args
     - ecs deploy ecs-live  live-pender-background --image live-pender-background $ECR_API_BASE_URL/live/pender/api:$CI_COMMIT_SHA --exclusive-env -e live-pender-background APP pender -e live-pender-background DEPLOY_ENV live -e live-pender-background AWS_REGION $AWS_DEFAULT_REGION --timeout 3600 --exclusive-secrets `cat live-pender-background.env.args`
     - echo "new Image was deployed $ECR_API_BASE_URL/live/pender/api:$CI_COMMIT_SHA"
   only:

.travis.yml

@@ -10,8 +10,8 @@ before_install:
 before_script:
 - docker-compose exec pender test/setup-parallel
 script:
-- docker-compose exec pender bundle exec rake "parallel:test[3]"
-- docker-compose exec pender bundle exec rake parallel:spec
+- docker-compose exec -e TEST_RETRY_COUNT=5 pender bundle exec rake "parallel:test[3]"
+- docker-compose exec -e TEST_RETRY_COUNT=5 pender bundle exec rake parallel:spec
 after_script:
 - docker-compose exec pender cat tmp/performance.csv
 - docker-compose exec -e TRAVIS_PULL_REQUEST=$TRAVIS_PULL_REQUEST -e TRAVIS_TEST_RESULT=$TRAVIS_TEST_RESULT -e TRAVIS_BRANCH=$TRAVIS_BRANCH -e CC_TEST_REPORTER_ID=$CC_TEST_REPORTER_ID -e GIT_COMMIT_SHA=$GIT_COMMIT_SHA -e GIT_COMMITTED_AT=$GIT_COMMITTED_AT pender test/test-coverage

CODEOWNERS

@@ -1 +1 @@
-* @caiosba @melsawy
+* @caiosba @melsawy @vasconsaurus

app/models/concerns/media_archiver.rb

@@ -61,10 +61,6 @@ def declare_archiver(name, patterns, modifier, enabled = true)
 
     def give_up(info = {})
       url, archiver, key_id = info[:args][0], info[:args][1], info[:args][2]
-      PenderSentry.notify(
-        StandardError.new(info[:error_message]),
-        info.merge({ url: url, archiver: archiver, key_id: key_id })
-      )
       Rails.logger.warn level: 'WARN', message: "[#{info[:error_class]}] #{info[:error_message]}", url: url, archiver: archiver
       data = { error: { message: info[:error_message], code: Lapis::ErrorCodes::const_get('ARCHIVER_FAILURE') }}
       Media.notify_webhook_and_update_cache(archiver, url, data, key_id)

app/models/metrics.rb

@@ -97,21 +97,22 @@ def verify_facebook_metrics_response(url, response)
 
       error = JSON.parse(response.body)['error']
       is_retryable = (RETRYABLE_FACEBOOK_ERROR_CODES + FACEBOOK_RATE_LIMIT_CODES).include?(error['code'].to_i)
+      fb_error_hash = {
+        'app.api_key' => ApiKey.current&.id,
+        'facebook.metrics.error.code' => error['code'],
+        'facebook.metrics.error.message' => error['message'],
+        'facebook.metrics.url' => url,
+        'facebook.metrics.retryable' => is_retryable
+      }
 
       Rails.logger.warn level: 'WARN', message: "Facebook metrics error: #{error['code']} - #{error['message']}", url: url, key_id: ApiKey.current&.id, error: error, retryable: is_retryable
       TracingService.set_error_status(
         "Facebook metrics error",
-        attributes: {
-          'app.api_key' => ApiKey.current&.id,
-          'facebook.metrics.error.code' => error['code'],
-          'facebook.metrics.error.message' => error['message'],
-          'facebook.metrics.url' => url,
-          'facebook.metrics.retryable' => is_retryable
-        }
+        attributes: fb_error_hash
       )
       if is_retryable
         @locker.lock(3600) if FACEBOOK_RATE_LIMIT_CODES.include?(error['code'].to_i)
-        raise Pender::Exception::RetryLater, 'Metrics request failed'
+        raise Pender::Exception::RetryLater, "Metrics request failed.\n#{fb_error_hash.to_json}"
       end
     end
 

app/models/parser/twitter_profile.rb

@@ -9,10 +9,7 @@ def type
 
       def patterns
         [
-          /^https?:\/\/(www\.)?twitter\.com\/(?<username>[\w\d]+)(\?+.*)$/,
-          /^https?:\/\/(0|m|mobile)\.twitter\.com\/(?<username>[\w\d]+)(\?+.*)$/,
-          /^https?:\/\/(www\.)?twitter\.com\/(?<username>[^\/]+)$/,
-          /^https?:\/\/(0|m|mobile)\.twitter\.com\/(?<username>[^\/]+)$/
+          /^https?:\/\/((0|m|mobile|www)\.)?twitter\.com\/(?<username>[\w]{4,15})[\/]*(\?.*)?[\/]*$/
         ]
       end
     end

config/initializers/02_sentry.rb

@@ -10,4 +10,7 @@
 
   # Any exceptions we want to prevent sending to Sentry
   config.excluded_exceptions += ['Pender::Exception::RetryLater']
+
+  # report_after_job_retries when turned on, the SDK will only report the exception after all retries have failed.
+  config.sidekiq.report_after_job_retries = true
 end

config/initializers/03_sidekiq.rb

@@ -10,11 +10,11 @@
   Sidekiq.configure_server do |config|
     config.redis = redis_config
 
-    config.death_handlers << ->(job, ex) do
-      if ex.is_a?(Pender::Exception::RetryLater)
-        ex = Pender::Exception::RetryLimitHit.new(ex)
+    config.death_handlers << ->(job, original_exception) do
+      if original_exception.is_a?(Pender::Exception::RetryLater)
+        limit_hit_exception = Pender::Exception::RetryLimitHit.new(original_exception)
       end
-      Sentry.capture_exception(ex)
+      PenderSentry.notify(limit_hit_exception, {job: job, original_exception: original_exception.cause})
     end
   end
 

pull_request_template.md

@@ -21,6 +21,7 @@ Please describe parts of the change that require extra attention during code rev
 - [ ] I have added unit and feature tests, if the PR implements a new feature or otherwise would benefit from additional testing
 - [ ] I have added regression tests, if the PR fixes a bug
 - [ ] I have added logging, exception reporting, and custom tracing with any additional information required for debugging
+- [ ] I considered secure coding practices when writing this code. Any security concerns are noted above.
 - [ ] I have commented my code in hard-to-understand areas, if any
 - [ ] I have made needed changes to the README
 - [ ] My changes generate no new warnings

test/integration/parsers/facebook_item_test.rb

@@ -0,0 +1,60 @@
+require 'test_helper'
+
+class FacebookItemIntegrationTest < ActiveSupport::TestCase
+  test "should get facebook post with valid data from crowdtangle" do
+    m = create_media url: 'https://www.facebook.com/144585402276277/posts/1127489833985824'
+    data = m.as_json
+
+    assert_equal 'facebook', data['provider']
+    assert_equal 'item', data['type']
+    assert_equal '144585402276277_1127489833985824', data['external_id']
+    assert data['error'].nil?
+    assert !data['title'].blank?
+    assert !data['username'].blank?
+    assert !data['author_name'].blank?
+    assert !data['author_picture'].blank?
+    assert !data['author_url'].blank?
+    assert !data['description'].blank?
+    assert !data['text'].blank?
+    assert !data['picture'].blank?
+    assert !data['published_at'].blank?
+    # data['html'] started to be returned as an empty string for this test
+    # which is extra weird since we get it even when the page does not exist
+    # will come back to this
+    # assert !data['html'].blank?
+  end
+
+  test "should get facebook data even if crowdtangle fails" do
+    m = create_media url: 'https://www.facebook.com/ECRG.TheBigO/posts/pfbid036xece5JjgLH7rD9RnCr1ASnjETq7QThCHiH1HqYAcfUZNHav4gFJdYUY7nGU8JB6l'
+    data = m.as_json
+
+    assert_equal 'facebook', data['provider']
+    assert_equal 'item', data['type']
+    assert data['external_id'].blank?
+    assert data['error'].nil?
+    assert !data['raw']['crowdtangle']['error'].blank?
+    assert !data['title'].blank?
+    assert !data['description'].blank?
+    assert !data['picture'].blank?
+    assert !data['html'].blank?
+  end
+
+  test "should return data even if post does not exist" do
+    m = create_media url: 'https://www.facebook.com/111111111111111/posts/1111111111111111'
+    data = m.as_json
+
+    assert_equal 'facebook', data['provider']
+    assert_equal 'item', data['type']
+    assert_equal '111111111111111_1111111111111111', data['external_id']
+    assert_equal 'https://www.facebook.com/111111111111111/posts/1111111111111111', data['title']
+    assert !data['raw']['crowdtangle']['error'].blank?
+    assert_equal '', data['username']
+    assert_equal '', data['author_name']
+    assert_equal '', data['author_picture']
+    assert_equal '', data['author_url']
+    assert_equal '', data['description']
+    assert_equal '', data['picture']
+    assert_equal '', data['published_at']
+    assert !data['html'].blank?
+  end
+end

test/integration/parsers/facebook_profile_test.rb

@@ -0,0 +1,48 @@
+require 'test_helper'
+
+class FacebookProfileIntegrationTest < ActiveSupport::TestCase
+  test "should parse Facebook page" do
+    media = create_media url: 'https://www.facebook.com/ironmaiden/?fref=ts'
+    data = media.as_json
+
+    assert !data['title'].blank?
+    assert_equal 'ironmaiden', data['username']
+    assert_equal 'facebook', data['provider']
+    assert_equal 'profile', data['type']
+
+    # Requires login, so cannot fetch ID from HTML
+    assert data['id'].blank?
+    assert data['external_id'].blank?
+  end
+
+  test "should parse Facebook page with numeric id" do
+    media = create_media url: 'https://www.facebook.com/pages/Meedan/105510962816034?fref=ts'
+    data = media.as_json
+
+    assert !data['title'].blank?
+    assert_equal 'Meedan', data['username']
+    assert_not_nil data['description']
+    assert_not_nil data['picture']
+    assert_not_nil data['published_at']
+    assert_equal 'facebook', data['provider']
+    assert_equal 'profile', data['type']
+
+    # Parsed from URL
+    assert_equal '105510962816034', data['id']
+    assert_equal '105510962816034', data['external_id']
+  end
+
+  test "should return data even if Facebook page does not exist" do
+    media = create_media url: 'https://www.facebook.com/pages/fakepage/1111111111111'
+    data = media.as_json
+
+    assert_equal 'https://www.facebook.com/pages/fakepage/1111111111111', data['title']
+    assert_equal 'fakepage', data['username']
+    assert data['description'].blank?
+    assert data['picture'].blank?
+    assert data['published_at'].blank?
+    assert_equal 'facebook', data['provider']
+    assert_equal 'profile', data['type']
+  end
+end
+

test/models/archiver_test.rb

@@ -742,4 +742,36 @@ def quietly_redefine_constant(klass, constant, new_value)
   ensure
     WebMock.disable!
   end
+
+  test "MediaArchiver should not notify Sentry when the worker hits the maximum number of retries" do
+    WebMock.stub_request(:post, /example.com\/webhook/).to_return(status: 200, body: '')
+
+    Media.any_instance.stubs(:follow_redirections)
+    Media.any_instance.stubs(:get_canonical_url).returns(true)
+    Media.any_instance.stubs(:try_https)
+    Media.any_instance.stubs(:parse)
+    Media.any_instance.stubs(:archive)
+
+    a = create_api_key application_settings: { config: { 'perma_cc_key': 'my-perma-key' }, 'webhook_url': 'https://example.com/webhook.php', 'webhook_token': 'test' }
+    url = 'http://example.com'
+
+    data = {}
+    sentry_call_count = 0
+    arguments_checker = Proc.new do |e|
+      sentry_call_count += 1
+      assert_equal StandardError, e.class
+    end
+
+    assert_raises StandardError do
+      Media.new(url: url, key: a).as_json(archivers: 'perma_cc')
+    end
+
+    PenderSentry.stub(:notify, arguments_checker) do
+      Media.give_up({ args: [url, 'perma_cc', nil], error_message: 'Test Archiver' })
+    end      
+
+    assert_equal 0, sentry_call_count
+    ensure
+      WebMock.disable!
+  end
 end