Add author links on articles #25

MatheusRich · 2023-08-24T20:28:53Z

Until now, the article writer had to manually add links for every author
mentioned in the article. This PR automates this process.

Now, we load an AuthorConfig with a preferred link for each author. If the
author is not in the config, we use the default #to-do link.

This class also has two additional advantages:

It allows future improvements, like adding a preferred name instead of
automatically using the one from Slack.
It allows users to update their links instead of asking someone else to
do so. This is in line with our value of self-management.

Other changes:

I updated the prompt on OpenAiWriter to prevent it from removing the author
links.
I updated some specs to use a Factory instead of ad-hoc objects.
Updated ruby-openai gem and use ChatGPT 3.5 model

Also add a small test factory module.

Up to now, the article writer had to manually add links for every author mentioned in the article. This PR automates of this process. Now we load an `AuthorConfig` that has a preferred link for each author. If the author is not in the config, we use the default `#to-do` link. This class also has two additional advantages: 1. It allows future improvements, like adding a preferred name, instead of automatically using the one from Slack. 2. It allows users to update their own links, instead of asking someone else to do it. This is in line with [our value of self-management]. Other changes: - I updated the prompt on `OpenAiWriter`, to prevent it from removing the author links. - I update some specs to use a Factory instead of ad-hoc objects. [our value of self-management]: https://thoughtbot.com/playbook/our-company/purpose-mission-values#self-management

MatheusRich · 2023-08-24T20:30:10Z

lib/config/author_config.yml

This list is based on previous editions of This Week in #dev, so I know which links these authors want to use.

MatheusRich · 2023-08-24T20:31:30Z

lib/gold_miner/slack/client.rb

@@ -9,8 +9,8 @@ class Slack::Client

    extend Dry::Monads[:result]

-    def self.build(api_token:, slack_client: ::Slack::Web::Client)
-      client = new(api_token, slack_client)
+    def self.build(api_token:, slack_client: ::Slack::Web::Client, author_config: AuthorConfig.default)


It feels a bit strange to do this inside the Slack::Client class.

Does it feel like Slack::Client knows too much about your domain?
Should it also know about the "rupee-gold" emoji, your keywords "TIL", "tip"?

Just rubber-ducking :)

Should it also know about the "rupee-gold" emoji, your keywords "TIL", "tip"?

Those bother me less than the AuthorConfig, because at least they're related to a slack message. We do have a MessagesQuery class, which is a builder for a Slack message search query.

If we could extract the "Interesting Messages" concept somehow, that could be useful.

Maybe a new class?

# I'm not a fan of this name, but this is just to spark an idea class InterestingMessagesQueries def initialize(slack_channel) @slack_channel = slack_channel end def til_messages_query = interesting_messages.with_topic("TIL") def tip_messages = interesting_messages.with_topic("tip") def golden_messages = interesting_messages.with_reaction("rupee-gold") private def interesting_messages MessagesQuery .new .on_channel(@slack_channel) .sent_after_last_friday end end

Good points. I've also been thinking about the "layers" of the app. Something like this might make sense:

Client objects: SDK or wrappers around SDK code: Slack::Client, OpenAi::Client. We could think of them as part of the "infrastructure layer"

Query and Service objects: part of the "domain layer". They use Client objects to do their thing, and return raw data, or rich objects, that are part of our domain.

View objects: part of the "presentation layer". OpenAiWriter, SimpleWriter. They shouldn't know about Client objects. They would receive domain data and present it.

GoldMiner object, and any other object we need to coordinate execution: part of the "application layer", the way through which we interact with the application.

That's my Rails brain thinking :D

That's useful. I'd love to take some time to investigate this together and do some refactoring.

Sure, any time!

MatheusRich · 2023-08-24T20:36:27Z

lib/gold_miner/slack/client.rb

          permalink: match.permalink
        )
      end
    end

-    def fetch_author(message)
+    def build_author(message)
      Slack::User.new(


I'd love to call this class Author, instead of Slack::User. Maybe there are two objects here?

Slack::User: contains:

id (Slack id)

username (Slack username)

BlogPost::Author: contains:

name (which I'm planning to pull from a config instead of only using what's on Slack)

link: preferred link to use

And the BlogPost::Author might receive a Slack::User on its constructor.

Thoughts?

mike-burns · 2023-08-28T17:26:16Z

Is it possible to share that YAML with the one that Giant Robots uses?

MatheusRich · 2023-08-28T17:36:58Z

@mike-burns That would be cool, and I thought about it too. The challenge is that we need to map someone's Slack username to the name on that YAML file. We have no common "id" or "foreign key" on both systems.

Alternatively, both could use Hub as the source of truth and have it generate a YAML with all the info we might need.

elias19r · 2023-09-01T16:18:11Z

lib/gold_miner/author_config.rb

+    def self.default
+      YAML
+        .load_file(DEFAULT_CONFIG_PATH)
+        .transform_values { |author| author["link"] }
+        .then { |links| new(links) }
+    end
+
+    def initialize(author_config = {})
+      @author_config = author_config
+    end
+
+    def link_for(slack_username)
+      @author_config.fetch(slack_username, DEFAULT_AUTHOR_LINK)
+    end


minor: Just noticed some "name disparity": AuthorConfig cannot be initialized with the contents of author_config.yml because it actually expects "author links" in the initialization.

If we plan to support more config fields in the future, we could load the hash from the YAML file as is, not transforming it, and update the link_for method. Something along the lines:

# frozen_string_literal: true module GoldMiner class AuthorConfig DEFAULT_AUTHOR_LINK = "#to-do" DEFAULT_CONFIG_PATH = "lib/config/author_config.yml" def self.default YAML .load_file(DEFAULT_CONFIG_PATH) .then { |author_config| new(author_config) } end def initialize(author_config = {}) @author_config = author_config end def link_for(slack_username) @author_config.dig(slack_username, "link") || DEFAULT_AUTHOR_LINK end end end

Alternatively, we could rename the file and class like author_config.yml --> author_links.yml (and update the content) and AuthorConfig --> AuthorLinks

Alternatively, we could rename the file and class like author_config.yml --> author_links.yml (and update the content) and AuthorConfig --> AuthorLinks

That's how it started, but I figured we could add more fields (like name) later, so I changed the class to AuthorConfig.

AuthorConfig cannot be initialized with the contents of author_config.yml because it actually expects "author links" in the initialization.

You're absolutely right! This is reminiscent of when it was AuthorLinks instead. TY for catching this.

lib/config/author_config.yml

elias19r

Thank you for working on this!

I don't have many suggestions, I just left a minor comment about AuthorConfig

Co-authored-by: Elias Rodrigues <eliasrodrigues.info@gmail.com>

@elias19r

While coding #25, I noticed (as correctly pointed out by @elias19r) some weird dependencies in the app: The Slack Client knew too much about the business logic. This PR is a first move in a new direction. More changes will come in the future. Here are the major changes: - **Rewrite `Slack::Client`**: - This class used to know too much about the app-specific needs. It's now a thin wrapper around the Slack API client gem. - A major change though, is that it automatically fetches the message author real name, so it issues an additional API call for each message. To remedy the performance hits, all the author names are requested in parallel, with a cache to avoid unnecessary requests. - **Introduce `GoldMiner::SlackExplorer`** - This class is responsible for "exploring" Slack and "mining" interesting messages. Those messages, later become a blog post. - Now that we have a simple Slack client, we can put the business logic in the explorer. This also opens the door for more explorers in the future, like a Twitter explorer.

MatheusRich added 3 commits August 11, 2023 12:36

Fix specs

cdfe56a

Update ruby-openai gem and use ChatGPT 3.5 model

dbc05a7

Also add a small test factory module.

MatheusRich commented Aug 24, 2023

View reviewed changes

elias19r reviewed Sep 1, 2023

View reviewed changes

elias19r approved these changes Sep 1, 2023

View reviewed changes

MatheusRich and others added 2 commits September 1, 2023 13:41

Sort author config alphabetically

cea32e7

Fix author config initialization

b3ddc34

Co-authored-by: Elias Rodrigues <eliasrodrigues.info@gmail.com>

MatheusRich merged commit 37c33e7 into main Sep 1, 2023
3 checks passed

MatheusRich deleted the mr/author-links branch September 1, 2023 18:28

MatheusRich mentioned this pull request Sep 7, 2023

Introduce SlackExplorer #33

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add author links on articles #25

Add author links on articles #25

MatheusRich commented Aug 24, 2023 •

edited

Loading

MatheusRich Aug 24, 2023 •

edited

Loading

MatheusRich Aug 24, 2023

elias19r Sep 1, 2023

MatheusRich Sep 1, 2023

elias19r Sep 1, 2023

MatheusRich Sep 1, 2023

elias19r Sep 1, 2023

MatheusRich Aug 24, 2023

mike-burns commented Aug 28, 2023

MatheusRich commented Aug 28, 2023

elias19r Sep 1, 2023 •

edited

Loading

MatheusRich Sep 1, 2023

elias19r left a comment

Add author links on articles #25

Add author links on articles #25

Conversation

MatheusRich commented Aug 24, 2023 • edited Loading

MatheusRich Aug 24, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-burns commented Aug 28, 2023

MatheusRich commented Aug 28, 2023

elias19r Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elias19r left a comment

Choose a reason for hiding this comment

MatheusRich commented Aug 24, 2023 •

edited

Loading

MatheusRich Aug 24, 2023 •

edited

Loading

elias19r Sep 1, 2023 •

edited

Loading