Add developer responsiveness badges

[fig]

This PR adds a "responsiveness" badge to a developer's profile.
closes #757

Pull request checklist

• My code contains tests covering the code I modified
• I linted and tested the project with bin/check
• I added significant changes and product updates to the changelog

Requirements from #757

• Add a badge to developer profiles if their response rate is over 90%
• Replace that badge with a different one if their response rate is a perfect 100%
• If needed, ensure the badge status is cached efficiently with no N+1 queries
  Requires review by someone with better database fu.
• Stretch goal: Give developers 6 hours to respond to a new message before dropping or removing their badge
  Too follow

I'm not totally happy with the text used in the badges so any suggestions for alternatives would be gratefully appreciated.
Current text: "Good responder" / "Perfect responder"

I have chosen to implement this feature by writing a concern to be included in the Developer model. The concern utilises the existing Stats::Conversation class to calculate developer response rate.

Sample rendering

[joemasilotti]

Thanks for the submission!

Let's use the badge titles I outlined in the issue for now: "100% response rate" and "90% response rate".

Also, I won't be able to merge this until the third todo item is completed. Without caching the values we end up with a nasty N+1 on /developers. It might make more sense to wait for @troyizzle to finish #758, though.

web      |   Conversation Count (0.3ms)  SELECT COUNT(*) FROM "conversations" WHERE "conversations"."developer_id" = $1 AND "conversations"."developer_blocked_at" IS NULL AND "conversations"."business_blocked_at" IS NULL  [["developer_id", 26]]
web      |   ↳ app/models/stats/conversation.rb:10:in `sent'
web      |   Conversation Count (0.3ms)  SELECT COUNT(*) FROM "conversations" WHERE "conversations"."developer_id" = $1 AND "conversations"."developer_blocked_at" IS NULL AND "conversations"."business_blocked_at" IS NULL  [["developer_id", 25]]
web      |   ↳ app/models/stats/conversation.rb:10:in `sent'
web      |   Conversation Count (0.3ms)  SELECT COUNT(*) FROM "conversations" WHERE "conversations"."developer_id" = $1 AND "conversations"."developer_blocked_at" IS NULL AND "conversations"."business_blocked_at" IS NULL  [["developer_id", 20]]
web      |   ↳ app/models/stats/conversation.rb:10:in `sent'
web      |   Conversation Count (0.3ms)  SELECT COUNT(*) FROM "conversations" WHERE "conversations"."developer_id" = $1 AND "conversations"."developer_blocked_at" IS NULL AND "conversations"."business_blocked_at" IS NULL  [["developer_id", 19]]
web      |   ↳ app/models/stats/conversation.rb:10:in `sent'
web      |   Conversation Count (0.3ms)  SELECT COUNT(*) FROM "conversations" WHERE "conversations"."developer_id" = $1 AND "conversations"."developer_blocked_at" IS NULL AND "conversations"."business_blocked_at" IS NULL  [["developer_id", 18]]
web      |   ↳ app/models/stats/conversation.rb:10:in `sent'
web      |   Conversation Count (0.4ms)  SELECT COUNT(*) FROM "conversations" WHERE "conversations"."developer_id" = $1 AND "conversations"."developer_blocked_at" IS NULL AND "conversations"."business_blocked_at" IS NULL  [["developer_id", 17]]
web      |   ↳ app/models/stats/conversation.rb:10:in `sent'
web      |   Conversation Count (0.4ms)  SELECT COUNT(*) FROM "conversations" WHERE "conversations"."developer_id" = $1 AND "conversations"."developer_blocked_at" IS NULL AND "conversations"."business_blocked_at" IS NULL  [["developer_id", 16]]
web      |   ↳ app/models/stats/conversation.rb:10:in `sent'
web      |   Conversation Count (0.3ms)  SELECT COUNT(*) FROM "conversations" WHERE "conversations"."developer_id" = $1 AND "conversations"."developer_blocked_at" IS NULL AND "conversations"."business_blocked_at" IS NULL  [["developer_id", 15]]
web      |   ↳ app/models/stats/conversation.rb:10:in `sent'
web      |   Conversation Count (0.3ms)  SELECT COUNT(*) FROM "conversations" WHERE "conversations"."developer_id" = $1 AND "conversations"."developer_blocked_at" IS NULL AND "conversations"."business_blocked_at" IS NULL  [["developer_id", 14]]
web      |   ↳ app/models/stats/conversation.rb:10:in `sent'

Let me know if you have appetite to take care of that. If no, no worries! I can take over.

[fig]

All noted.

After sleeping on this, I'm not loving the dependency on Stats::Conversation during the request cycle. Maybe adding a database column for response_rate might be a better way to go architecturally. I envision a background job queued to update this column a configurable time (e.g. 12 hours) after a 'cold message' is received.

This would solve the N+1 and keep the use of Stats::Conversation to a minimum and in a background job. Essentially, we will cache the response_rate in the db.

Your thoughts?

[joemasilotti]

That's very similar to what I was thinking, too!

We can roll ahead and cache this directly in the developers table for now. Then when #758 is implemented we can migrate over the data.

Or, we can hold off and wait until that is merged in first. Your call!

[fig]

Okay, I'll go ahead with the additional table column for now.

I've just been looking at #758 and I can't honestly see what problem it is trying to solve? If anything, it will just trigger more db hits for no real gain. Maybe I'm missing something?

[joemasilotti]

It will generate on additional db query per list of developers, instead of one for every badge for every developer.

So, for each paginated page of 10 developers it will add 1 additional query.

If we had all of the current and upcoming badge logic calculated on the fly it would generate 40 additional queries (4 badges x 10 developers).

[fig]

• The solution now adds a new column response_rate to the developers table.
• A newly created conversation triggers a background job to update the Developer's response rate after a configurable length of time.
• Badge text fixed.

A migration is included in this PR
A rake task is provided to backfill the new table column (rake backfills:developer_response_rate).

[joemasilotti]

Looking good!

I'm now realizing that 90 vs. 100 might not matter very much. Let's consolidate them. If a developer has a 90%+ response rate show a badge that says "High response rate".

Also, it looks like the background job is updating the developer's updated_at column which is used to show the "Recently active" badge. Let's update the job to not update that column for now with a comment explaining why. This will be removed in a future PR when we start to cache the badges in their own model.

I also made some inline questions/comments.

[fig]

I've changed the implementation to have a single "High Response Rate" badge as suggested.

I noticed that "recently active" was relying on the updated_at column. The background job is calling #update_column which updates the attribute directly in the database issuing an UPDATE SQL statement. I chose this method because updated_at/updated_on are not updated. One downside is that validations are also skipped. I've add a comment to change this to a regular #update! at a later date.

I noticed that test/components/developers/badge_component_test.rb wasn't actually doing what it claimed.
It was possible to delete the lines setting the attributes of the developer and the tests would still pass.
I have renamed the test file to test/components/developers/badges_component_test.rb and refactored the tests within to exercise the Developers::BadgesComponent. This class utilises the BadgeComponent for rendering and so now the whole chain is tested.

[fig]

I'm struggling to get my head around the different queue adaptors (:inline v :async)
At the moment, CI is failing during seeding because queuing a job in the future throws an error when queue adaptor is set to :inline.

Any thoughts on how to fix this?

[joemasilotti]

What's even weirder is that when I run bin/rails rake db:reset in development I get the same error. But when starting a new conversation from the UI the job enqueues without issue.

Honestly, I'm not really sure what's going on! I'll have to dig in more.

[fig]

This is what I have learnt in the last few hours.

• Unless specified otherwise, the data seeding task will use the :async queue adaptor.
• We are specifying the :inline adaptor in our seeds file, so this will be honoured.
• The :inline adaptor does not allow scheduling jobs so raises an error when trying to seed Conversations because of the .set(wait: ...) method.
• If we specify :async only for the UpdateDeveloperResponseRateJob,
  (UpdateDeveloperResponseRateJob.queue_adapter = :async in seeds.rb), the db will seed, but will enqueue jobs in our dev environment.

[joemasilotti]

Got it! Thanks for identifying all the moving parts - that's helpful.

Ignoring the implementation details for a second... when running the seeds ideally the column is populated right away. Meaning, the job kicks off immediately.

Is it possible to achieve that? Perhaps via running the job conditionally async/sync depending on the environment? Or something more graceful in the config or options passed to the job?

[fig]

One option might be to set config.developer_response_grace_period to a very low value (5secs) in development and have the job run :async during seeds. The low grace period won't be an issue as our seeded developers will be responding almost instantaneously and the update job will run as soon as a dev server is fired up. My only concern is that there will be jobs left dormant in redis if a server instance is not started after seeding. After saying that, I believe the response rate update job is idempotentish (multiple calls within a short time-frame will produce the same result) so it might not be an issue.

[joemasilotti]

Looks great! Nice work @fig. Thanks for all the updates.