Skip to content

reviewer badge for notification for chaoss slack - #508

Open
AJ-droi wants to merge 9 commits into
badging:mainfrom
AJ-droi:main
Open

reviewer badge for notification for chaoss slack#508
AJ-droi wants to merge 9 commits into
badging:mainfrom
AJ-droi:main

Conversation

@AJ-droi

@AJ-droi AJ-droi commented Apr 1, 2026

Copy link
Copy Markdown

Pull Request Template

Summary:

Changes:

  • scripts/reviewer_badge_notifier.py: verify Slack chat_postMessage responses, update state only when both public and DM posts succeed, and fix time.sleep import.

Testing:

  • Manual: python scripts/reviewer_badge_notifier.py (requires valid Slack scopes and COMMUNITY_MANAGER_ID).
  • Automated tests: none added or modified.

Additional Notes:

  • Slack bot needs chat:write (and im:write) scopes; app reinstall required after scope changes.

Signed-off-by: AJ-droi <ajiriosiobe@gmail.com>
Signed-off-by: AJ-droi <ajiriosiobe@gmail.com>
@AJ-droi

AJ-droi commented Apr 1, 2026

Copy link
Copy Markdown
Author

Hi @adeyinkaoresanya, Here is my PR for issue #482.
I look for forward to review and feedback

@adeyinkaoresanya

adeyinkaoresanya commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

@AJ-droi
I just realised that I placed my comment in the wrong place (under issue #482 )! 😭
Apologies!
Bringing the comment here for documentation purposes:

Dated 05/05/26
@AJ-droi Thank you for your PR. While your solution works, what we are expecting is that it would be integrated into the main script so we don't have to run two different scripts.

Currently, I am working on refactoring the update_reviewers.py for efficiency as the current implementation is a bit expensive. Once that’s done, I will reach out so we can integrate your changes. Please reach out if you have any questions.

Thank you once again!

@adeyinkaoresanya

Copy link
Copy Markdown
Contributor

Hello @AJ-droi

As promised, I'm reaching out with an update. I have completed the refactor, and we are excited about the results. The script is now approximately 95% more efficient and significantly easier to maintain.

Because of these changes, your current PR no longer aligns with the updated code structure. I really appreciate the work you have already put into it, and I would still love to see your contribution included if you are interested in updating it.

If you'd like to continue, I would ask that the implementation follow the new design pattern, integrate cleanly with the refactored codebase, and run through scripts/main.py.

Please let me know whether you'd like to take this on, and I'd be happy to answer any questions or provide guidance on the new structure.

Thank you again for your patience and for your contribution to the project.

@AJ-droi

AJ-droi commented Jun 15, 2026

Copy link
Copy Markdown
Author

Hi @adeyinkaoresanya, I have integrated the badge reviewer to scripts/main.py.
I look forward to your review and feedback

@adeyinkaoresanya

Copy link
Copy Markdown
Contributor

Hi @adeyinkaoresanya, I have integrated the badge reviewer to scripts/main.py.
I look forward to your review and feedback

Acknowledged! I will look into it soon. Thank you very much!

@AJ-droi

AJ-droi commented Jun 18, 2026

Copy link
Copy Markdown
Author

Hi @adeyinkaoresanya, I have integrated the badge reviewer to scripts/main.py.
I look forward to your review and feedback

Acknowledged! I will look into it soon. Thank you very much!

Thank you, I await your review

@adeyinkaoresanya
adeyinkaoresanya self-requested a review June 25, 2026 14:27

@adeyinkaoresanya adeyinkaoresanya left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@AJ-droi Thanks for the contribution. The overall implementation looks good. I reviewed it mainly for correctness and runtime safety. There are a few changes that should be addressed before merge.

Required changes

1. Validate required environment variables at start up

The module initializes Slack using environment variables without checking if they exist:

SLACK_BOT_TOKEN = os.getenv("SLACK_BOT_TOKEN")
SLACK_CHANNEL_ID = os.getenv("SLACK_CHANNEL_ID")
COMMUNITY_MANAGER_ID = os.getenv("COMMUNITY_MANAGER_ID")

If any of these is missing, failures will occur later and be harder to debug.

Recommendation:
Explicitly validate them:

Example:

if not SLACK_BOT_TOKEN:
    raise RuntimeError("Missing SLACK_BOT_TOKEN")

2. Guard against unknown badge values in is_upgrade() for future badge changes and to prevent runtime crashes:

Current implementation:

return BADGE_ORDER.index(new) > BADGE_ORDER.index(old)

This can raise a ValueError if old or new contains a badge not present in BADGE_ORDER.

Recommendation:
Validate both values before indexing:

if old not in BADGE_ORDER or new not in BADGE_ORDER:
    return False

Suggested improvements (non-blocking)

3. Consider using conversations_open() before sending DMs

Current implementation sends directly to:

channel=COMMUNITY_MANAGER_ID

While this may work, it can fail depending on workspace configuration or if the target account becomes inactive.

A more reliable approach would be:

dm = client.conversations_open(users=[COMMUNITY_MANAGER_ID])
channel_id = dm["channel"]["id"]

client.chat_postMessage(channel=channel_id, text=message)

This ensures a valid DM channel exists before sending.

4. Clarify the state source

This implementation currently derives badge state from the README. If badge_state.json is no longer part of the intended architecture, please remove it to avoid redundant state sources.

5. Improve the messages

Let's make the milestone more concrete by including the reviewer’s total completed reviews and the message to the community manager more actionable

For the public channel:

message = f"""
@{username}, congratulations on earning the {badge} badge after completing {data.total} reviews! 🎉

Thank you for your continued contributions to Event DEI Badging 👏
"""

For the community manager's DM:

message = f"""
Hi <@{COMMUNITY_MANAGER_ID}> 👋

Badger @{username} has unlocked the {badge} badge with {data.total} completed reviews!🎉

This could be a great opportunity to spotlight their contributions on our socials or in the next community update.
"""

6. Please resolve the outstanding conflicts, especially around README.md, before merge.

Once the required changes are addressed, this should be good to go!

Thanks again!


load_dotenv()

SLACK_BOT_TOKEN = os.getenv("SLACK_BOT_TOKEN")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Validate required environment variables at start up


SLACK_BOT_TOKEN = os.getenv("SLACK_BOT_TOKEN")
SLACK_CHANNEL_ID = os.getenv("SLACK_CHANNEL_ID")
COMMUNITY_MANAGER_ID = os.getenv("COMMUNITY_MANAGER_ID")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Validate required environment variables at start up

load_dotenv()

SLACK_BOT_TOKEN = os.getenv("SLACK_BOT_TOKEN")
SLACK_CHANNEL_ID = os.getenv("SLACK_CHANNEL_ID")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Validate required environment variables at start up

AJ-droi added 2 commits June 26, 2026 14:36
Signed-off-by: AJ <ajiriosiobe@gmail.com>
Signed-off-by: Ajiri Osiobe <ajiriosiobe@gmail.com>
@AJ-droi

AJ-droi commented Jun 29, 2026

Copy link
Copy Markdown
Author

@AJ-droi Thanks for the contribution. The overall implementation looks good. I reviewed it mainly for correctness and runtime safety. There are a few changes that should be addressed before merge.

Required changes

1. Validate required environment variables at start up

The module initializes Slack using environment variables without checking if they exist:

SLACK_BOT_TOKEN = os.getenv("SLACK_BOT_TOKEN")
SLACK_CHANNEL_ID = os.getenv("SLACK_CHANNEL_ID")
COMMUNITY_MANAGER_ID = os.getenv("COMMUNITY_MANAGER_ID")

If any of these is missing, failures will occur later and be harder to debug.

Recommendation: Explicitly validate them:

Example:

if not SLACK_BOT_TOKEN:
    raise RuntimeError("Missing SLACK_BOT_TOKEN")

2. Guard against unknown badge values in is_upgrade() for future badge changes and to prevent runtime crashes:

Current implementation:

return BADGE_ORDER.index(new) > BADGE_ORDER.index(old)

This can raise a ValueError if old or new contains a badge not present in BADGE_ORDER.

Recommendation: Validate both values before indexing:

if old not in BADGE_ORDER or new not in BADGE_ORDER:
    return False

Suggested improvements (non-blocking)

3. Consider using conversations_open() before sending DMs

Current implementation sends directly to:

channel=COMMUNITY_MANAGER_ID

While this may work, it can fail depending on workspace configuration or if the target account becomes inactive.

A more reliable approach would be:

dm = client.conversations_open(users=[COMMUNITY_MANAGER_ID])
channel_id = dm["channel"]["id"]

client.chat_postMessage(channel=channel_id, text=message)

This ensures a valid DM channel exists before sending.

4. Clarify the state source

This implementation currently derives badge state from the README. If badge_state.json is no longer part of the intended architecture, please remove it to avoid redundant state sources.

5. Improve the messages

Let's make the milestone more concrete by including the reviewer’s total completed reviews and the message to the community manager more actionable

For the public channel:

message = f"""
@{username}, congratulations on earning the {badge} badge after completing {data.total} reviews! 🎉

Thank you for your continued contributions to Event DEI Badging 👏
"""

For the community manager's DM:

message = f"""
Hi <@{COMMUNITY_MANAGER_ID}> 👋

Badger @{username} has unlocked the {badge} badge with {data.total} completed reviews!🎉

This could be a great opportunity to spotlight their contributions on our socials or in the next community update.
"""

6. Please resolve the outstanding conflicts, especially around README.md, before merge.

Once the required changes are addressed, this should be good to go!

Thanks again!

Hello @adeyinkaoresanya

I have made a commit for the fix to this changes you requested for.
I look forward to your feedback.

@adeyinkaoresanya

Copy link
Copy Markdown
Contributor

@AJ-droi Thank you for your prompt response on this.

This looks good to go. I will merge as soon as I lay hold of the necessary credentials.

One more thing though, is there a reason the badge_state.json file still exists? I haven't seen it used anywhere else in your code.

@AJ-droi

AJ-droi commented Jun 30, 2026

Copy link
Copy Markdown
Author

@AJ-droi Thank you for your prompt response on this.

This looks good to go. I will merge as soon as I lay hold of the necessary credentials.

One more thing though, is there a reason the badge_state.json file still exists? I haven't seen it used anywhere else in your code.

I used it for my local test, let me remove it

@AJ-droi

AJ-droi commented Jun 30, 2026

Copy link
Copy Markdown
Author

@AJ-droi Thank you for your prompt response on this.
This looks good to go. I will merge as soon as I lay hold of the necessary credentials.
One more thing though, is there a reason the badge_state.json file still exists? I haven't seen it used anywhere else in your code.

I used it for my local test, let me remove it

I have removed the badge_state.json

@adeyinkaoresanya

Copy link
Copy Markdown
Contributor

@AJ-droi Thank you for your prompt response on this.
This looks good to go. I will merge as soon as I lay hold of the necessary credentials.
One more thing though, is there a reason the badge_state.json file still exists? I haven't seen it used anywhere else in your code.

I used it for my local test, let me remove it

I have removed the badge_state.json

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants