Skip to content

Migrate talentpool to new API schema#1445

Merged
mbaruh merged 11 commits into
masterfrom
ks123/talentpool/migrate
Mar 8, 2021
Merged

Migrate talentpool to new API schema#1445
mbaruh merged 11 commits into
masterfrom
ks123/talentpool/migrate

Conversation

@ks129
Copy link
Copy Markdown
Contributor

@ks129 ks129 commented Mar 3, 2021

Note: This has to be merged exactly same time than python-discord/site#447

Migrates talent pool to the new system. More exact changes are in the commit message body.

Closes #1425

ks129 added 2 commits March 3, 2021 15:58
We need to disable this, because new format of nominations
don't match with it.
- Add disable_header to watchchannel initialization.
We don't have root actor field anymore, so headers give error
and there is no point to rewrite this, because this will be
removed soon.

- Removed duplicates check of nominations of one user.
Now as API allows this, multiple actors can nomination one
user.

- Add special error message if same actor have already nominated user
Every actor can only have 1 nomination entry.

- Remove previous reason from watch command
We don't store reason that way anymore, and we don't want
that this message spam whole channel.

- Split end reason and reason editing commands.
API PATCH request buildup have been changed, so changing both
of them in one command don't make sense anymore.

- Migrate nomination string generation
@ks129 ks129 added a: API Related to or causes API changes a: moderation Related to community moderation functionality: (moderation, defcon, verification) labels Mar 3, 2021
Copy link
Copy Markdown
Member

@mbaruh mbaruh left a comment

Choose a reason for hiding this comment

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

Only a code review, haven't tested yet. Mostly grammer

Comment thread bot/exts/moderation/watchchannels/talentpool.py Outdated
Comment thread bot/exts/moderation/watchchannels/talentpool.py Outdated
Comment thread bot/exts/moderation/watchchannels/talentpool.py Outdated
Comment thread bot/exts/moderation/watchchannels/talentpool.py Outdated
Comment thread bot/exts/moderation/watchchannels/talentpool.py Outdated
Comment thread bot/exts/moderation/watchchannels/talentpool.py Outdated
Comment thread bot/exts/moderation/watchchannels/talentpool.py Outdated
Comment thread bot/exts/moderation/watchchannels/talentpool.py Outdated
Comment thread bot/exts/moderation/watchchannels/talentpool.py
Comment thread bot/exts/moderation/watchchannels/talentpool.py Outdated
@mbaruh mbaruh requested a review from wookie184 March 5, 2021 20:35
@ks129 ks129 requested a review from mbaruh March 6, 2021 12:36
@Xithrius Xithrius added the t: enhancement Changes or improvements to existing features label Mar 6, 2021
Copy link
Copy Markdown
Contributor

@wookie184 wookie184 left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! Have tested this and it seems to work as expected, nice job.

Only thing I would say is that the result from tp search is a bit messy/hard to read at the moment, here's an example
image
I'm not sure if changing this much is within the scope of this PR, if you think not I can add them in a separate PR, but here are some thoughts:

  • The "Nomination ID" for inactive nominations should be after status and date like with active infractions.
  • "Actor", "Created", "Reason" would be a better order than "Actor", "Reason", "Created".

@ks129
Copy link
Copy Markdown
Contributor Author

ks129 commented Mar 7, 2021

@wookie184 I made these changes.

@mbaruh
Copy link
Copy Markdown
Member

mbaruh commented Mar 8, 2021

image
This doesn't paginate well, gets cut off. Doesn't seem to happen with the infractions paginator.

@Xithrius Xithrius added the p: 1 - high High Priority label Mar 8, 2021
Copy link
Copy Markdown
Member

@mbaruh mbaruh left a comment

Choose a reason for hiding this comment

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

After testing it seems to be working okay for many, a bit shorter nominations. So I guess limiting it to 1000 characters really is enough like you said.

I also think you should change the approval message in line 112 to ":white_check_mark: The nomination for {user} has been added to the talent pool"

@ks129 ks129 requested a review from mbaruh March 8, 2021 17:21
@mbaruh mbaruh merged commit 01f011d into master Mar 8, 2021
@mbaruh mbaruh deleted the ks123/talentpool/migrate branch March 8, 2021 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: API Related to or causes API changes a: moderation Related to community moderation functionality: (moderation, defcon, verification) p: 1 - high High Priority t: enhancement Changes or improvements to existing features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow multiple nominations on a member to be active at once

4 participants