From 9f3187abdf21974eb4e090b17302f6bbbd02f5d0 Mon Sep 17 00:00:00 2001 From: Urban Rotnik Date: Mon, 4 Feb 2019 14:33:22 +0100 Subject: [PATCH 1/2] Add service for sync system notifications --- Gemfile | 3 + Gemfile.lock | 14 +++ app/models/system_notification.rb | 9 ++ .../sync_system_notifications_service.rb | 84 +++++++++++++++++ config/secrets.yml | 6 ++ spec/models/system_notification_spec.rb | 27 ++++++ .../sync_system_notifications_service_spec.rb | 91 +++++++++++++++++++ spec/spec_helper.rb | 90 ++---------------- 8 files changed, 241 insertions(+), 83 deletions(-) create mode 100644 app/services/notifications/sync_system_notifications_service.rb create mode 100644 spec/services/notifications/sync_system_notifications_service_spec.rb diff --git a/Gemfile b/Gemfile index 312f8c36f..c55ce32cb 100644 --- a/Gemfile +++ b/Gemfile @@ -59,6 +59,7 @@ gem 'delayed_paperclip', git: 'https://github.com/jrgifford/delayed_paperclip.git', ref: 'fcf574c' gem 'faker' # Generate fake data +gem 'httparty' gem 'i18n-js', '~> 3.0' # Localization in javascript files gem 'jbuilder' # JSON structures via a Builder-style DSL gem 'logging', '~> 2.0.0' @@ -119,6 +120,7 @@ group :development, :test do gem 'rubocop', '>= 0.59.0', require: false gem 'scss_lint', require: false gem 'starscope', require: false + gem 'timecop' end group :test do @@ -131,6 +133,7 @@ group :test do gem 'poltergeist' gem 'shoulda-matchers' gem 'simplecov', require: false + gem 'webmock' end group :production do diff --git a/Gemfile.lock b/Gemfile.lock index cd2bda31a..f139120cc 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -170,6 +170,8 @@ GEM coffee-script-source (1.12.2) commit_param_routing (0.0.1) concurrent-ruby (1.0.5) + crack (0.4.3) + safe_yaml (~> 1.0.0) crass (1.0.4) cucumber (3.1.0) builder (>= 2.1.2) @@ -238,7 +240,10 @@ GEM globalid (0.4.1) activesupport (>= 4.2.0) hammerjs-rails (2.0.8) + hashdiff (0.3.8) hashie (3.5.7) + httparty (0.16.2) + multi_xml (>= 0.5.2) i18n (0.9.5) concurrent-ruby (~> 1.0) i18n-js (3.0.3) @@ -452,6 +457,7 @@ GEM ruby-progressbar (1.10.0) ruby_dep (1.5.0) rubyzip (1.2.1) + safe_yaml (1.0.4) sanitize (4.6.6) crass (~> 1.0.2) nokogiri (>= 1.4.4) @@ -510,6 +516,7 @@ GEM thor (0.20.0) thread_safe (0.3.6) tilt (2.0.8) + timecop (0.9.1) tinymce-rails (4.7.13) railties (>= 3.1.1) turbolinks (5.1.1) @@ -524,6 +531,10 @@ GEM uniform_notifier (1.11.0) warden (1.2.7) rack (>= 1.0) + webmock (3.5.1) + addressable (>= 2.3.6) + crack (>= 0.3.2) + hashdiff webpacker (2.0) activesupport (>= 4.2) multi_json (~> 1.2) @@ -581,6 +592,7 @@ DEPENDENCIES faker figaro hammerjs-rails + httparty i18n-js (~> 3.0) jbuilder jquery-rails @@ -633,11 +645,13 @@ DEPENDENCIES sneaky-save! spinjs-rails starscope + timecop tinymce-rails (~> 4.7.13) turbolinks (~> 5.1.1) tzinfo-data uglifier (>= 1.3.0) underscore-rails + webmock webpacker (~> 2.0) whacamole wicked_pdf (~> 1.1.0) diff --git a/app/models/system_notification.rb b/app/models/system_notification.rb index cb1c48cdc..13e1a230a 100644 --- a/app/models/system_notification.rb +++ b/app/models/system_notification.rb @@ -37,4 +37,13 @@ class SystemNotification < ApplicationRecord 'user_system_notifications.seen_at' ) end + + def self.last_sync_timestamp + # If no notifications are present, the created_at of the + # first user is used as the "initial sync time-point" + SystemNotification + .order(last_time_changed_at: :desc) + .first&.last_time_changed_at&.to_i || + User.order(created_at: :desc).first&.created_at&.to_i + end end diff --git a/app/services/notifications/sync_system_notifications_service.rb b/app/services/notifications/sync_system_notifications_service.rb new file mode 100644 index 000000000..aa46a8746 --- /dev/null +++ b/app/services/notifications/sync_system_notifications_service.rb @@ -0,0 +1,84 @@ +# frozen_string_literal: true + +module Notifications + class SyncSystemNotificationsService + extend Service + include HTTParty + base_uri Rails.application.secrets.system_notifications_uri + + attr_reader :errors + + def initialize + @errors = {} + end + + def call + call_api + + save_new_notifications if succeed? + + self + end + + def succeed? + @errors.none? + end + + private + + def call_api + last_sync = SystemNotification.last_sync_timestamp + channel = Rails.application.secrets.system_notifications_channel + + unless last_sync + @errors[:last_sync_timestamp] = 'Cannot find last_sync_timestamp' + return false + end + + query = { query: { last_sync_timestamp: last_sync, + notifications_channel: channel }, + headers: { 'accept': + 'application/vnd.system-notifications.1+json' } } + + # rubocop:disable Lint/ShadowedException: + begin + @api_call = self.class.get('/api/system_notifications', query) + + if @api_call.response.code.to_i != 200 + @errors[:api_error] = + [@api_call.response.code.to_s, @api_call.response.message].join('-') + end + rescue SocketError, HTTParty::Error, StandardError => e + @errors[e.class.to_s.downcase.to_sym] = e.message + end + # rubocop:enable Lint/ShadowedException: + end + + def save_new_notifications + @api_call.parsed_response['system_notifications'].each do |sn| + # Save new notification if not exists or override old 1 + attrs = + sn.slice('title', + 'description', + 'modal_title', + 'modal_body', + 'show_on_login', + 'source_id') + .merge('source_created_at': + Time.parse(sn['source_created_at']), + 'last_time_changed_at': + Time.parse(sn['last_time_changed_at'])) + .symbolize_keys + + n = SystemNotification + .where(source_id: attrs[:source_id]).first_or_initialize(attrs) + + if n.new_record? + n.save! + elsif n.last_time_changed_at < attrs[:last_time_changed_at] + n.update_attributes!(attrs) + end + end + end + end +end diff --git a/config/secrets.yml b/config/secrets.yml index f98279263..a6c77a834 100644 --- a/config/secrets.yml +++ b/config/secrets.yml @@ -59,12 +59,18 @@ common: &common development: secret_key_base: 22f2adf8f5cb73351da28f2292daa840cc2a414ae00ae605b175a8d5c73932f7e5b8ff8ef8f1554a7f1064f9869b15347f7709f0daa6ccb24c50f3cace304f64 + system_notifications_uri: <%= ENV["SYSTEM_NOTIFICATIONS_URI"] %> + system_notifications_channel: <%= ENV["SYSTEM_NOTIFICATIONS_CHANNEL"] %> <<: *common test: secret_key_base: f3719934e04fa8871cf5d33d5c60f05e1b8995e0315265aef9f8b878da49bd2d386eb25ce35545b469a94ccf22f91e0052b93a15194b4f57b0c8d6ce8b150e1e + system_notifications_uri: 'system-notifications-service.test' + system_notifications_channel: 'test-channel' <<: *common production: secret_key_base: <%= ENV["SECRET_KEY_BASE"] %> + system_notifications_uri: <%= ENV["SYSTEM_NOTIFICATIONS_URI"] %> + system_notifications_channel: <%= ENV["SYSTEM_NOTIFICATIONS_CHANNEL"] %> <<: *common diff --git a/spec/models/system_notification_spec.rb b/spec/models/system_notification_spec.rb index 6c6d780c2..a5a85a01e 100644 --- a/spec/models/system_notification_spec.rb +++ b/spec/models/system_notification_spec.rb @@ -42,4 +42,31 @@ describe SystemNotification do describe 'Associations' do it { is_expected.to have_many(:users) } end + + describe 'self.last_sync_timestamp' do + context 'when there is no users or system notifications in db' do + it 'returns nil' do + expect(described_class.last_sync_timestamp).to be_nil + end + end + + context 'when there is no system notifications in db' do + it 'returns last user created_at' do + create :user + + expect(described_class.last_sync_timestamp) + .to be == User.last.created_at.to_i + end + end + + context 'when have some system notifications' do + it 'returns last system notifications last_time_changed_at timestamp' do + create :user + create :system_notification + + expect(described_class.last_sync_timestamp) + .to be SystemNotification.last.last_time_changed_at.to_i + end + end + end end diff --git a/spec/services/notifications/sync_system_notifications_service_spec.rb b/spec/services/notifications/sync_system_notifications_service_spec.rb new file mode 100644 index 000000000..d97875a81 --- /dev/null +++ b/spec/services/notifications/sync_system_notifications_service_spec.rb @@ -0,0 +1,91 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Notifications::SyncSystemNotificationsService do + url = 'http://system-notifications-service.test/api/system_notifications' + let!(:user) { create :user } + let(:service_call) do + Notifications::SyncSystemNotificationsService.call + end + + let(:first_call_result) do + notifications = (1..10).map do |id| + FactoryBot.attributes_for(:system_notification) + .merge('source_id': id) + end + { system_notifications: notifications } + end + + context 'when request is successful' do + before do |test| + if test.metadata[:add_notifications_before] + create :system_notification, + source_id: 10, + last_time_changed_at: 10.days.ago.to_datetime + end + + stub_request(:get, url) + .with(query: { 'last_sync_timestamp': + SystemNotification.last_sync_timestamp, + 'notifications_channel': 'test-channel' }, + headers: { 'accept': + 'application/vnd.system-notifications.1+json' }) + .to_return(body: first_call_result.to_json, + status: 200, + headers: { 'Content-Type': 'application/json' }) + end + + it 'adds 10 notifictions into db' do + expect { service_call }.to(change { SystemNotification.all.count }.by(10)) + end + + it 'does not add 10 notifications because ther are already in DB' do + first_call_result[:system_notifications].each do |sn| + SystemNotification.create(sn) + end + + expect { service_call }.not_to(change { SystemNotification.all.count }) + end + + it 'updates existing notification', add_notifications_before: true do + expect { service_call } + .to(change { SystemNotification.last.last_time_changed_at }) + end + + it 'add only 3 notifications' do + first_call_result[:system_notifications][2..8].each do |sn| + SystemNotification.create(sn) + end + + expect { service_call }.to(change { SystemNotification.all.count }.by(3)) + end + + it 'return error when last_sync_timestamp is nil' do + allow(SystemNotification).to receive(:last_sync_timestamp).and_return(nil) + + expect(service_call.errors).to have_key(:last_sync_timestamp) + end + end + + context 'when request is unsuccessful' do + before do + stub_request(:get, url) + .with(query: { 'last_sync_timestamp': + SystemNotification.last_sync_timestamp, + 'notifications_channel': 'test-channel' }) + .to_return(status: [500, 'Internal Server Error']) + end + + it 'returns api_error with message' do + expect(service_call.errors).to have_key(:api_error) + end + + it 'returns error with description about itself' do + allow(Notifications::SyncSystemNotificationsService) + .to receive(:get).and_raise(SocketError) + + expect(service_call.errors).to have_key(:socketerror) + end + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 95350883c..1d6ea147e 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -1,40 +1,20 @@ -# This file was generated by the `rails generate rspec:install` command. Conventionally, all -# specs live under a `spec` directory, which RSpec adds to the `$LOAD_PATH`. -# The generated `.rspec` file contains `--require spec_helper` which will cause -# this file to always be loaded, without a need to explicitly require it in any -# files. -# -# Given that it is always loaded, you are encouraged to keep this file as -# light-weight as possible. Requiring heavyweight dependencies from this file -# will add to the boot time of your test suite on EVERY test run, even for an -# individual file that may not need all of that loaded. Instead, consider making -# a separate helper file that requires the additional dependencies and performs -# the additional setup, and require it from the spec files that actually need -# it. -# -# See http://rubydoc.info/gems/rspec-core/RSpec/Core/Configuration +# frozen_string_literal: true + require 'capybara/rspec' require 'simplecov' require 'faker' require 'active_record' require 'bullet' -require "json_matchers/rspec" +require 'json_matchers/rspec' +require 'webmock/rspec' # Require all custom matchers -Dir[File.expand_path(File.join(File.dirname(__FILE__),'support','**','*.rb'))].each { |f| require f } +Dir[ + File.expand_path(File.join(File.dirname(__FILE__), 'support', '**', '*.rb')) +].each { |f| require f } RSpec.configure do |config| - # rspec-expectations config goes here. You can use an alternate - # assertion/expectation library such as wrong or the stdlib/minitest - # assertions if you prefer. config.expect_with :rspec do |expectations| - # This option will default to `true` in RSpec 4. It makes the `description` - # and `failure_message` of custom matchers include text for helper methods - # defined using `chain`, e.g.: - # be_bigger_than(2).and_smaller_than(4).description - # # => "be bigger than 2 and smaller than 4" - # ...rather than: - # # => "be bigger than 2" expectations.include_chain_clauses_in_custom_matcher_descriptions = true end @@ -46,64 +26,8 @@ RSpec.configure do |config| # `true` in RSpec 4. mocks.verify_partial_doubles = true end - - # This option will default to `:apply_to_host_groups` in RSpec 4 (and will - # have no way to turn it off -- the option exists only for backwards - # compatibility in RSpec 3). It causes shared context metadata to be - # inherited by the metadata hash of host groups and examples, rather than - # triggering implicit auto-inclusion in groups with matching metadata. config.shared_context_metadata_behavior = :apply_to_host_groups -# The settings below are suggested to provide a good initial experience -# with RSpec, but feel free to customize to your heart's content. -=begin - # This allows you to limit a spec run to individual examples or groups - # you care about by tagging them with `:focus` metadata. When nothing - # is tagged with `:focus`, all examples get run. RSpec also provides - # aliases for `it`, `describe`, and `context` that include `:focus` - # metadata: `fit`, `fdescribe` and `fcontext`, respectively. - config.filter_run_when_matching :focus - - # Allows RSpec to persist some state between runs in order to support - # the `--only-failures` and `--next-failure` CLI options. We recommend - # you configure your source control system to ignore this file. - config.example_status_persistence_file_path = "spec/examples.txt" - - # Limits the available syntax to the non-monkey patched syntax that is - # recommended. For more details, see: - # - http://rspec.info/blog/2012/06/rspecs-new-expectation-syntax/ - # - http://www.teaisaweso.me/blog/2013/05/27/rspecs-new-message-expectation-syntax/ - # - http://rspec.info/blog/2014/05/notable-changes-in-rspec-3/#zero-monkey-patching-mode - config.disable_monkey_patching! - - # Many RSpec users commonly either run the entire suite or an individual - # file, and it's useful to allow more verbose output when running an - # individual spec file. - if config.files_to_run.one? - # Use the documentation formatter for detailed output, - # unless a formatter has already been configured - # (e.g. via a command-line flag). - config.default_formatter = "doc" - end - - # Print the 10 slowest examples and example groups at the - # end of the spec run, to help surface which specs are running - # particularly slow. - config.profile_examples = 10 - - # Run specs in random order to surface order dependencies. If you find an - # order dependency and want to debug it, you can fix the order by providing - # the seed, which is printed after each run. - # --seed 1234 - config.order = :random - - # Seed global randomization in this process using the `--seed` CLI option. - # Setting this allows you to use `--seed` to deterministically reproduce - # test failures related to randomization by passing the same `--seed` value - # as the one that triggered the failure. - Kernel.srand config.seed -=end - # Enable bullet gem in tests if Bullet.enable? config.before(:each) do Bullet.start_request From 5b55860dea03dbc98de570d086856bc7a2d93b0e Mon Sep 17 00:00:00 2001 From: Urban Rotnik Date: Tue, 12 Feb 2019 16:07:20 +0100 Subject: [PATCH 2/2] Add scheduler --- Gemfile | 1 + Gemfile.lock | 9 +++++ .../sync_system_notifications_service.rb | 11 ++++-- config/initializers/scheduler.rb | 35 +++++++++++++++++++ .../sync_system_notifications_service_spec.rb | 10 +++--- 5 files changed, 59 insertions(+), 7 deletions(-) create mode 100644 config/initializers/scheduler.rb diff --git a/Gemfile b/Gemfile index c55ce32cb..20b0406dd 100644 --- a/Gemfile +++ b/Gemfile @@ -85,6 +85,7 @@ gem 'delayed_job_active_record' gem 'devise-async', git: 'https://github.com/mhfs/devise-async.git', branch: 'devise-4.x' +gem 'rufus-scheduler', '~> 3.5' gem 'discard', '~> 1.0' diff --git a/Gemfile.lock b/Gemfile.lock index f139120cc..72bf16352 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -223,6 +223,8 @@ GEM doorkeeper (5.0.0) railties (>= 4.2) erubi (1.7.1) + et-orbi (1.1.7) + tzinfo execjs (2.7.0) factory_bot (4.8.2) activesupport (>= 3.0.0) @@ -236,6 +238,9 @@ GEM ffi (1.9.18) figaro (1.1.1) thor (~> 0.14) + fugit (1.1.8) + et-orbi (~> 1.1, >= 1.1.7) + raabro (~> 1.1) gherkin (5.0.0) globalid (0.4.1) activesupport (>= 4.2.0) @@ -371,6 +376,7 @@ GEM pry (>= 0.10.4) public_suffix (3.0.2) puma (3.11.2) + raabro (1.1.6) rack (2.0.6) rack-attack (5.4.1) rack (>= 1.0, < 3) @@ -457,6 +463,8 @@ GEM ruby-progressbar (1.10.0) ruby_dep (1.5.0) rubyzip (1.2.1) + rufus-scheduler (3.5.2) + fugit (~> 1.1, >= 1.1.5) safe_yaml (1.0.4) sanitize (4.6.6) crass (~> 1.0.2) @@ -633,6 +641,7 @@ DEPENDENCIES rubocop (>= 0.59.0) ruby-graphviz (~> 1.2) rubyzip + rufus-scheduler (~> 3.5) sanitize (~> 4.4) sass-rails (~> 5.0.6) scenic (~> 1.4) diff --git a/app/services/notifications/sync_system_notifications_service.rb b/app/services/notifications/sync_system_notifications_service.rb index aa46a8746..c6d2fc8e7 100644 --- a/app/services/notifications/sync_system_notifications_service.rb +++ b/app/services/notifications/sync_system_notifications_service.rb @@ -36,7 +36,7 @@ module Notifications end query = { query: { last_sync_timestamp: last_sync, - notifications_channel: channel }, + channels_slug: channel }, headers: { 'accept': 'application/vnd.system-notifications.1+json' } } @@ -47,6 +47,13 @@ module Notifications if @api_call.response.code.to_i != 200 @errors[:api_error] = [@api_call.response.code.to_s, @api_call.response.message].join('-') + + # Add message for logging if exists + if @api_call.parsed_response.try('error') + @errors[:api_error] += ': ' + @api_call + .parsed_response['error'] + .flatten&.join(' - ').to_s + end end rescue SocketError, HTTParty::Error, StandardError => e @errors[e.class.to_s.downcase.to_sym] = e.message @@ -55,7 +62,7 @@ module Notifications end def save_new_notifications - @api_call.parsed_response['system_notifications'].each do |sn| + @api_call.parsed_response['notifications'].each do |sn| # Save new notification if not exists or override old 1 attrs = sn.slice('title', diff --git a/config/initializers/scheduler.rb b/config/initializers/scheduler.rb new file mode 100644 index 000000000..0d57a706d --- /dev/null +++ b/config/initializers/scheduler.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +require 'rufus-scheduler' + +scheduler = Rufus::Scheduler.singleton + +if ENV['ENABLE_TEMPLATES_SYNC'] && ARGV[0] == 'jobs:work' + # Templates sync periodic task + scheduler.every '1h' do + Rails.logger.info('Templates, syncing all template projects') + updated, total = TemplatesService.new.update_all_projects + Rails.logger.info( + "Templates, total number of updated projects: #{updated} out of #{total}}" + ) + Rails.logger.flush + end +end + +if Rails.application.secrets.system_notifications_uri.present? && + Rails.application.secrets.system_notifications_channel.present? && + ARGV[0] == 'jobs:work' + + # System notifications periodic task + scheduler.every '5m' do + Rails.logger.info('System Notifications syncing') + Rails.logger.info(Process.pid) + result = Notifications::SyncSystemNotificationsService.call + if result.errors.any? + Rails.logger.info('System Notifications sync error: ') + Rails.logger.info(result.errors.to_s) + else + Rails.logger.info('System Notifications sync done') + end + end +end diff --git a/spec/services/notifications/sync_system_notifications_service_spec.rb b/spec/services/notifications/sync_system_notifications_service_spec.rb index d97875a81..a15cffacb 100644 --- a/spec/services/notifications/sync_system_notifications_service_spec.rb +++ b/spec/services/notifications/sync_system_notifications_service_spec.rb @@ -14,7 +14,7 @@ describe Notifications::SyncSystemNotificationsService do FactoryBot.attributes_for(:system_notification) .merge('source_id': id) end - { system_notifications: notifications } + { notifications: notifications } end context 'when request is successful' do @@ -28,7 +28,7 @@ describe Notifications::SyncSystemNotificationsService do stub_request(:get, url) .with(query: { 'last_sync_timestamp': SystemNotification.last_sync_timestamp, - 'notifications_channel': 'test-channel' }, + 'channels_slug': 'test-channel' }, headers: { 'accept': 'application/vnd.system-notifications.1+json' }) .to_return(body: first_call_result.to_json, @@ -41,7 +41,7 @@ describe Notifications::SyncSystemNotificationsService do end it 'does not add 10 notifications because ther are already in DB' do - first_call_result[:system_notifications].each do |sn| + first_call_result[:notifications].each do |sn| SystemNotification.create(sn) end @@ -54,7 +54,7 @@ describe Notifications::SyncSystemNotificationsService do end it 'add only 3 notifications' do - first_call_result[:system_notifications][2..8].each do |sn| + first_call_result[:notifications][2..8].each do |sn| SystemNotification.create(sn) end @@ -73,7 +73,7 @@ describe Notifications::SyncSystemNotificationsService do stub_request(:get, url) .with(query: { 'last_sync_timestamp': SystemNotification.last_sync_timestamp, - 'notifications_channel': 'test-channel' }) + 'channels_slug': 'test-channel' }) .to_return(status: [500, 'Internal Server Error']) end