Publish lifecycle events to routemaster #19

gregbeech · 2017-04-18T13:27:03Z

Here's a proposal for how publishing events could look from model classes. It's based on the code in orderweb but I've cleaned up the naming/implementation and made it more amenable to publishing multiple events from a single model by making multiple publishers the default.

Has an ActiveRecord module, a basic publisher implementation, and a registry for publishers. Still needs a bunch more work, but this should be a slightly cleaner version of what’s in orderweb geared towards more complex publishing and multi-publishing.

mezis · 2017-04-19T06:53:14Z

lib/roo_on_rails/routemaster/lifecycle_events.rb

+    module LifecycleEvents
+      extend ActiveSupport::Concern
+
+      ROUTEMASTER_EVENT_TYPE_MAP = {


I'd add a little comment — this maps ActiveRecord events to Routemaster events?

mezis · 2017-04-19T06:54:40Z

lib/roo_on_rails/routemaster/lifecycle_events.rb

@@ -0,0 +1,49 @@
+require 'active_support/concern'
+require 'new_relic/agent'


Missing a require for roo_on_rails/routemaster/publishers which is referenced.

mezis · 2017-04-19T06:57:39Z

lib/roo_on_rails/routemaster/publisher.rb

+      def publish!
+        return unless publish?
+
+        event_data = data # cache in case it isn't memoised


erm, "cache" ? "memoise"? data is a stub/virtual method so this doesn't actually do anything. I think you could replace lines 18-21 with just:

routemaster.send(event, topic, url, data: recursive_stringify(event_data))

(with a caveat for the nil case of course)

To be fair this pattern is copied from your code ;-)

But yes I did mean memoise. It could be cleaned by handling nil in the stringify method though.

mezis · 2017-04-19T07:00:05Z

lib/roo_on_rails/routemaster/publisher.rb

+    class Publisher
+      attr_reader :model, :event
+
+      def initialize(model, event)


I'd suggest dependency-injecting a client: ::Routemaster::Client here — otherwise people are going to stub like hell when testing publisherS?

mezis · 2017-04-19T07:00:18Z

lib/roo_on_rails/routemaster/publisher.rb

@@ -0,0 +1,49 @@
+module RooOnRails


Missing a require for routemaster/client.

mezis

A series of nitpicks but LGTM. Can't "approve" just yet, of course :P

danielcooper · 2017-04-19T09:17:13Z

Seems like a good plan.

Addresses the review comments and makes things a bit tidier.

gregbeech · 2017-04-19T11:01:02Z

@mezis @danielcooper this is going to be "interesting" to test properly as RooOnRails itself isn't a Rails app so doesn't have any database etc. to actually test an ActiveRecord model against.

The only thing I really need that for is the after_commit hook, so any objections to me faking up an after_commit hook with the same signature on the model and checking that it calls the right thing rather than going through the whole mess of actually have a real ActiveRecord model to test against?

danielcooper · 2017-04-19T11:08:52Z

Can you provide a nice api for doing it, and perhaps a test helper - rather than mocking it out?

danielcooper · 2017-04-19T11:10:19Z

(not that you can test after commit when you've transactional tests anyway)

codecov · 2017-07-17T13:07:58Z

Codecov Report

Merging #19 into master will increase coverage by 0.66%.
The diff coverage is 99.52%.

@@            Coverage Diff             @@
##           master      #19      +/-   ##
==========================================
+ Coverage   91.26%   91.92%   +0.66%     
==========================================
  Files          75       82       +7     
  Lines        1958     2131     +173     
==========================================
+ Hits         1787     1959     +172     
- Misses        171      172       +1

Impacted Files	Coverage Δ
spec/roo_on_rails/routemaster/publishers_spec.rb	`100% <100%> (ø)`
lib/roo_on_rails/config.rb	`91.66% <100%> (+1.66%)`	⬆️
spec/integration/routemaster_spec.rb	`100% <100%> (ø)`
lib/roo_on_rails/routemaster/publisher.rb	`100% <100%> (ø)`
spec/roo_on_rails/routemaster/publisher_spec.rb	`100% <100%> (ø)`
spec/roo_on_rails/config_spec.rb	`100% <100%> (ø)`	⬆️
lib/roo_on_rails/routemaster/publishers.rb	`100% <100%> (ø)`
.../roo_on_rails/routemaster/lifecycle_events_spec.rb	`100% <100%> (ø)`
lib/roo_on_rails/routemaster/lifecycle_events.rb	`96.29% <96.29%> (ø)`
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 197ac44...fddf7bf. Read the comment docs.

danielcooper

LGTM. This will need some docs both in the README and in code docs I think.

You'll probably want some more eyes too

danielcooper · 2017-08-02T09:34:07Z

lib/roo_on_rails/routemaster/lifecycle_events.rb

+
+      module ClassMethods
+        def publish_lifecycle_events(*events)
+          events ||= %i(create update destroy)


This needs to be something along the lines of

events = events.any? ? events : %i(create update destroy)

*events will produce [] with no arguments

Additionally, described_class._commit_callbacks will give you a list of registed callbacks. You should use this in your test.

Oh right, that's super helpful. I'll try that, thank you 🙂

danielcooper · 2017-08-02T10:46:22Z

spec/integration/routemaster_spec.rb

+
+        it 'inserts Routemaster Client into the middleware stack' do
+          app.wait_start
+          expect(output).to include 'Routemaster::Client'


Why does the client have to be in the middleware stack?

rfonglip

Added 1 small question.

There are some models (e.g. restaurant) where we don't want to actually delete/destroy the object, so we mark it as deleted in the db.

Would roo_on_rails handle this scenario?

Else nice work!

rfonglip · 2017-08-03T08:57:25Z

lib/roo_on_rails/routemaster/publisher.rb

+        @client = client
+      end
+
+      def publish?


Why does this method always return true?

The intention is that it's ready to be overloaded in the clients own publishers

The idea is that it's true by default and you can then overwrite this in your implementation. You can see this in the example I provide.

danielcooper · 2017-08-03T09:18:24Z

@rfonglip

You can either overload the publisher's publish! method to switch the event type if the object has it's flag set or try and tie in with paranoia's callbacks - calling publish_lifecycle_event_on_delete (although I assume you'll need to find a way for it not to trigger an update callback).

I suppose it depends on us having a standard gem we use for soft deletes?

scott-ad-riley

LGTM, one minor typo

Thanks for thorough the readme on this 👍

scott-ad-riley · 2017-08-03T09:34:40Z

README.md

+* `ROOBUS_URL` – the full URL of your Routemaster application (mandatory)
+* `ROOBUS_UUID` – the UUID of your application, e.g. `logistics-dashboard` (mandatory)
+
+If you then want to enable the publishing of events onto the event but, you need to set `ROUTEMASTER_PUBLISHING_ENABLED` to `true` and implement publishers as needed. An example of how to do this is detailed in [`README.routemaster_client.md`](README.routemaster_client.md).


minor typo: but -> bus

Good spot, thank you!

gregbeech · 2017-08-03T11:27:46Z

README.routemaster_client.md

+  include Rails.application.routes.url_helpers
+
+  def publish?
+    noop? || model.new_record? || model.previous_changes.any?


Could always move this into the base implementation.

That sounds reasonable to me 👍

gregbeech · 2017-08-03T11:27:56Z

README.routemaster_client.md

+# app/publishers/order_publisher.rb
+class OrderPublisher < BasePublisher
+  def url
+    api_order_url(model, host: ENV.fetch('API_HOST'), protocol: 'https')


Could move this into the base implementation too.

Ah, no, this one can't really move into the base implementation because sometimes there's an api_ prefix on the routes, sometimes not, and sometimes it's different. That's why I just raised I think.

Yeah I think this one is best to handle in the individual service apps indeed.

gregbeech · 2017-08-03T11:30:08Z

lib/roo_on_rails/railties/routemaster.rb

+      end
+
+      def roobus_url
+        ENV.fetch('ROOBUS_URL')


It strikes me that using ROUTEMASTER_URL and ROUTEMASTER_UUID would probably be cleaner names, but if people are dead set on ROOBUS_ then I guess that's fine.

I just assumed that these were prefixed with ROOBUS_ for a good reason and didn't even question it, but it would definitely make sense to be more consistent and use ROUTEMASTER_. Happy to make this change if people agree.

Note: It looks like it would just leave ROOBUS_CALLBACK_URL using ROOBUS_.

i'd prefer ROUTEMASTER_ over ROOBUS_

gregbeech

LGTM; couple of suggestions about docs that could be moved into the Publisher class as they look like they're sensible things to have in there anyway, but that could always be done at a later date.

mezis · 2017-08-07T09:41:06Z

README.md

-    - [HireFire Workers](#hirefire-workers)
-    - [Logging](#logging)
-    - [Google Oauth](#google-oauth)
+- [Configuration and usage](#configuration-and-usage)


Can we keep the "library usage" v "command features" sections? Users have been confused by the dual nature of roo_on_rails (it's both a library/framework complement and a CLI, usable separately).

Absolutely! I was a bit confused by this table of content myself, partly because it changed while I was working on this PR. I'll rename "Configuration and usage" to "Library usage" 👍

Crunch09

LGTM

Crunch09 · 2017-08-07T09:46:34Z

lib/roo_on_rails/railties/routemaster.rb

+      private
+
+      def bus_credentials_blank?
+        routemaster_url.blank? && routemaster_uuid.blank?


Shouldn't this be routemaster_url.blank? || routemaster_uuid.blank? ? (Assuming both are mandatory)

Yes indeed, good spot, thank you.

gregbeech added 2 commits April 12, 2017 15:44

Simplify publisher registration and rename mixing

f8176e9

mezis reviewed Apr 19, 2017

View reviewed changes

lib/roo_on_rails/routemaster/publisher.rb

@@ -0,0 +1,49 @@

module RooOnRails

Copy link

Contributor

mezis Apr 19, 2017

Choose a reason for hiding this comment

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

Missing a require for routemaster/client.

Merge branch 'master' into publish-lifecycle-events-to-routemaster

0601a2f

mezis reviewed Apr 19, 2017

View reviewed changes

Tidy up some of the publisher code

e9df9b8

Addresses the review comments and makes things a bit tidier.

gregbeech added 3 commits April 19, 2017 16:48

Add helper methods to check the event type

d1cb948

Merge branch 'master' into publish-lifecycle-events-to-routemaster

82d1e97

Add publishers spec

16c13ba

deliveroo deleted a comment from codecov bot Jul 17, 2017

Merge branch 'master' into publish-lifecycle-events-to-routemaster

431a46f

deliveroo deleted a comment from codecov bot Jul 17, 2017

Add routemaster-client to dependencies

25eb951

kplattret added 4 commits July 17, 2017 17:21

Add tests for Routemaster::Publisher

7e347e9

Add test example for #publish!

914c7ee

Merge branch 'master' into publish-lifecycle-events-to-routemaster

346f6ff

Try with method stubs instead

e8028ac

danielcooper approved these changes Jul 19, 2017

View reviewed changes

kplattret added 3 commits July 25, 2017 14:25

Merge branch 'master' into publish-lifecycle-events-to-routemaster

b8d7b2a

Update instructions in README

3214b11

Merge branch 'master' into publish-lifecycle-events-to-routemaster

79c37aa

kplattret added 2 commits July 31, 2017 12:31

Move RM client config to RooOnRails and add further documentation

97b1d63

Require Routemaster and refactor config_spec

11cf4de

danielcooper reviewed Aug 2, 2017

View reviewed changes

Fix empty array issue and add Routemaster integration test

2b94c46

danielcooper reviewed Aug 2, 2017

View reviewed changes

kplattret added 6 commits August 2, 2017 13:16

Fix the integration tests

1221947

Add lifecycle method for each verb and add tests

5609396

Refactor LifecycleEvents and add tests

ebe8518

Introduce ROUTEMASTER_PUBLISHING_ENBALED env var

b3bcd14

Update documentation to reflect latest changes

fed4763

Small tweaks to LifecycleEvents spec

f9a97d9

rfonglip reviewed Aug 3, 2017

View reviewed changes

scott-ad-riley approved these changes Aug 3, 2017

View reviewed changes

Fix tiny typo in the README

fbb7baa

gregbeech commented Aug 3, 2017

View reviewed changes

Check if create, update or noop event before publishing

1c207a4

kplattret force-pushed the publish-lifecycle-events-to-routemaster branch from 147023b to 1c207a4 Compare August 3, 2017 13:19

kplattret added 4 commits August 4, 2017 09:53

Rename ROOBUS_ to ROUTEMASTER_

5fded8e

Unblock CodeClimate

b42f01c

Fix CodeClimate issue

8afb4e6

Fix Routemaster integration test

e6b6926

mezis reviewed Aug 7, 2017

View reviewed changes

Crunch09 approved these changes Aug 7, 2017

View reviewed changes

Address PR comments

fddf7bf

mezis merged commit 30ef8f9 into master Aug 8, 2017

kplattret deleted the publish-lifecycle-events-to-routemaster branch August 8, 2017 13:54

		@@ -0,0 +1,49 @@
		require 'active_support/concern'
		require 'new_relic/agent'

Publish lifecycle events to routemaster #19

Publish lifecycle events to routemaster #19

Conversation

gregbeech commented Apr 18, 2017 • edited by kplattret Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mezis Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mezis left a comment

Choose a reason for hiding this comment

danielcooper commented Apr 19, 2017

gregbeech commented Apr 19, 2017

danielcooper commented Apr 19, 2017

danielcooper commented Apr 19, 2017

codecov bot commented Jul 17, 2017 • edited Loading

Codecov Report

danielcooper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfonglip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielcooper commented Aug 3, 2017

scott-ad-riley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kplattret Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregbeech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crunch09 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregbeech commented Apr 18, 2017 •

edited by kplattret

Loading

mezis Apr 19, 2017 •

edited

Loading

codecov bot commented Jul 17, 2017 •

edited

Loading

kplattret Aug 3, 2017 •

edited

Loading