Clarify inputs (arguments) to the list_user_domains routine

[yevgenko]

Relevant issue(s)

N/A

What does this do?

This tidying makes easier to read and understand the inputs of list_user_domains routine.

Why was this needed?

Making code easier to understand makes it easier to work with.

---

[skip changelog]

[garethrees]

Thanks so much! Little code cleanup commits like this are always appreciated 💖 .

I'm wondering whether we could make a slight tweak? I'm not sure we need both list_user_domains and list_user_domains_since, since the former now doesn't really do anything.

I think the key improvement here is moving to keyword arguments, which I don't see a reason not to do in the existing list_user_domains method. Essentially, ending up with:

def self.list_user_domains(start_date: nil, limit: nil)
  sql = if start_date
    <<~SQL
    SELECT lower(substring(email, position('@' in email)+1)) AS domain,
    COUNT(id) AS count
    FROM users
    WHERE created_at >= '#{start_date}'
    GROUP BY domain
    ORDER BY count DESC
    SQL
  else
    <<~SQL
    SELECT lower(substring(email, position('@' in email)+1)) AS domain,
    COUNT(id) AS count
    FROM users
    GROUP BY domain
    ORDER BY count DESC
    SQL
  end
  sql = "#{sql} LIMIT #{limit}" if limit

  User.connection.select_all(sql).to_a
end

That looks like it should work for existing uses of list_user_domains. What do you think?

[yevgenko]

I'm wondering whether we could make a slight tweak? I'm not sure we need both list_user_domains and list_user_domains_since, since the former now doesn't really do anything.

I do agree, I see the method is used in rake task and it relies on the "ruby magic" to interpret keyword arguments, so, the client won't be affected by the change. I'll push update in a minute.

[garethrees]

Looks great, thanks so much!