Skip to content

URL construction for member list access using base URL and org ID #467

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

anagha-cmd
Copy link

@anagha-cmd anagha-cmd commented Apr 16, 2025

This PR modifies how member list URLs are constructed by using a configurable base URL from environment variables instead of hardcoded values. This change improves flexibility and allows the bot to work with different membership systems. Closes #455
Changes made:

  • Added MEMBERS_LIST_BASE_URL as a required environment variable
  • Added validation for the new environment variable in config.py
  • Updated URL construction in cogs/make_member.py to use the base URL from settings
  • Updated documentation in README.md and .env.example to reflect these changes

Testing:

  • Tested URL construction with different base URLs
  • Verified error handling for invalid or missing base URLs
  • Confirmed backward compatibility with Guild of Students URL structure

@CarrotManMatt CarrotManMatt added documentation Improvements or additions to documentation good first issue Good for newcomers refactor Improvements to the codebase that do not directly affect users labels Apr 16, 2025
@MattyTheHacker MattyTheHacker self-requested a review April 16, 2025 11:42
@CarrotManMatt
Copy link
Member

Using the base URL in this way is a good idea! One issue I could forsee is if an alternative membership system didn't have a concept of organisation IDs.

Copy link
Member

@MattyTheHacker MattyTheHacker left a comment

Choose a reason for hiding this comment

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

The base url stuff here isn't necessary as it's unlikely that any organisation other than the guild will have the same url format so that can be removed. If we're looking at supporting other SUs I suspect that'll be a larger issue with significant refactoring needed to support the different member list formats

@CarrotManMatt
Copy link
Member

supporting other SUs I suspect that'll be a larger issue with significant refactoring needed to support the different member list formats

See #253

@MattyTheHacker
Copy link
Member

Yeah I think until we're ready to start actioning the config stuff we assume we're just working with the guild. We can start implementing compatibility with other SUs when that's done

@CarrotManMatt
Copy link
Member

other SUs

I'd say we can be even more generic than that and just say "membership system"! /lh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers refactor Improvements to the codebase that do not directly affect users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update example env
3 participants