Skip to content
This repository has been archived by the owner on Oct 10, 2024. It is now read-only.

Add /claimed command #44

Closed
wants to merge 5 commits into from
Closed

Add /claimed command #44

wants to merge 5 commits into from

Conversation

TimJentzsch
Copy link
Contributor

Relevant issue: Closes #43.

Description:

Adds a /claimed command to view the commands claimed by the given user.

Ideally we merge #34 before this one and then add the user extraction from that PR to the /claimed command as well.

Screenshots:

Multiple claimed post

One claimed post

No claimed posts

Checklist:

  • Code Quality
  • Pep-8
  • Tests (if applicable)
  • Success Criteria Met
  • Inline Documentation
  • Wiki Documentation (if applicable)

@TimJentzsch TimJentzsch requested a review from a team as a code owner June 29, 2021 10:49
# Instead, we ask that the post isn't archived yet.
# For a short amount of time this might return completed posts.
submission_response = self.blossom_api.get(
"submission/", params={"claimed_by": volunteer["id"], "archived": False}
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to update Blossom with having completed_by as a parameter? This method would be reduced in size quite drastically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does have completed_by, but I can't filter for the value to be NULL, at least AFAIK

Copy link
Member

Choose a reason for hiding this comment

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

That does make sense... We could make a calculated boolean property which resembles the current state of the post? e.g. unclaimed, in progress, and done, such as the subreddit

tr_link = f"[Link]({data['transcription']['url']})"
def to_embed(data: Dict) -> Embed: # noqa: C901
"""Convert the submission to a Discord embed."""
color = Color.from_rgb(255, 176, 0) # Orange
Copy link
Member

Choose a reason for hiding this comment

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

I must say that I find this method a bit long and convoluted, since it is doing a lot, from extracting data from a dictionary to picking colors specific to Discord. Would we be able to split these two responsibilities?

post_list = "\n".join(
[
i18n["claimed"]["post_list_item"].format(
i + 1, post["tor_url"], post["url"]
Copy link
Member

Choose a reason for hiding this comment

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

Do we want some pagination/limit here? I can already see a problem with 100 claimed messages flashing before my eyes :P

@itsthejoker
Copy link
Member

@TimJentzsch is this still needed?

@TimJentzsch
Copy link
Contributor Author

Yes, I am currently working on a PR to Blossom that simplifies the /find logic and wanted to implement that before adding this command

@TimJentzsch
Copy link
Contributor Author

The find command will get reworked, and this probably too. I will implement that later, probably in a new PR and maybe after we release Buttercup.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add /claimed command
3 participants