Fix/role bot2

[goldstarhigher]

16 fix code.

[DzhideX]

Thoughts:

• Clean up the codebase a little:

  • Can we remove any mention of database and any unnecessary "old" packages (e.g., mongoose)
  • In the same vein, having control.ts under "database/" might be confusing, can we change that?
  • Is query/members.gql being used for anything?
  • Remove unused variables (e.g. blockNumber in control.ts)

• Clean up control.ts:

  • Most of the file looks fine but the main part (the part with nested loops/if statements) I think could be a little better in terms of understandability
  • Use the correct loop in the right spots (e.g., a map for qn_recive_data being moved into a non-used var needs to be changed)
  • Better variable names than: "d", "r" and "k"
  • Remove nesting as much as possible (by using for example early returns/guard clauses and other techniques)
    • Examples: https://testing.googleblog.com/2017/06/code-health-reduce-nesting-reduce.html

• who_is:

  • Doesn't list member id
  • If no mapping for user: reply explaining that and link to a external resource to establish it
  • "who_is" (based on the spec) shouldn't necessarily list all discord roles, just the relevant (on-chain) ones

• list_role_members:

  • My interpretation of this is that it should go through all the members with this role and display "who_is data" for all of them
  • "list_role_members" (based on the spec) shouldn't necessarily list all discord roles, just the relevant (on-chain) ones

• status:

  • Status indicator missing: you could potentially indicate whether or not the infra used for the bot (e.g., query node and other) is online/working.

• I think we need better response messages for the bot (both grammatically but also content-wise). I'll check with some DAO members if they can help provide some.

  If anything is unclear please consult the original specifications first: https://github.com/Joystream/discord-bot/issues/5

[DzhideX]

Okay, after another quick review, a lot of it has been implemented but not all of it (to the extent that I'd like it to).

Namely, the following still need work:

• who_is:

  • if no mapping for user: reply explaining that and link to a external resource to establish it (missing external resource)

• list_role_members:

  • my interpretation of this is that it should go through all the members with this role and display "who_is data" for all of them
  • "list_role_members" (based on the spec) shouldn't necessarily list all discord roles, just the relevant (on-chain) ones

That being said, since these are all related to the new messages being written by HR Lead Mariia I think it'd be acceptable to consider this done (functionality-wise) ✅

[chrlschwb]

Okay, after another quick review, a lot of it has been implemented but not all of it (to the extent that I'd like it to).

Namely, the following still need work:

• who_is:

  • if no mapping for user: reply explaining that and link to a external resource to establish it (missing external resource)

• list_role_members:

  • my interpretation of this is that it should go through all the members with this role and display "who_is data" for all of them
  • "list_role_members" (based on the spec) shouldn't necessarily list all discord roles, just the relevant (on-chain) ones

That being said, since these are all related to the new messages being written by HR Lead Mariia I think it'd be acceptable to consider this done (functionality-wise) ✅

I think "list_role_members" should not display all the "who_is data" for all members. That would be repetition.

[DzhideX]

Here's what is written in the original spec for that specific point:

same as doing who_is on all discord handles in discord role, and include discord handles that hve no mapping, just as in who_is.

I disagree that it would be repetition. The reasoning is that you're saying give me all the roles of these specific members. So, you want more information than just who the members are who hold that one role. I don't see how it's repetition, it's just a different way to do it and if you have problems with it, then you have problems with the spec and you should raise that with Bedeho as I can't make any executive decision on that front.

[bedeho]

I strongly recommend Gitflow branch management is used from now on, not merging from some remote fork directly into main.