Skip to content
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

Feat: Add is admin utility #567

Merged
merged 9 commits into from
Aug 6, 2024

Conversation

Asartea
Copy link
Contributor

@Asartea Asartea commented Jul 14, 2024

Because

  • We currently duplicate this code

This PR

  • Adds an isAdmin(member) utility
  • Replace all code that did this previously with usages of the new function
  • Fixes using Map() for mocking in points.test.js to use Collection() instead
  • Runs prettier over all affected files

Issue

Closes #566

Pull Request Requirements

  • I have thoroughly read and understand The Odin Project Contributing Guide
    - [] The title of this PR follows the location of change: brief description of change format, e.g. Callbacks command: Update verbiage
  • The Because section summarizes the reason for this PR
  • The This PR section has a bullet point list describing the changes in this PR
  • If this PR addresses an open issue, it is linked in the Issue section
  • If this PR adds new features or functionality, I have added new tests
  • If applicable, I have ensured all tests related to any command files included in this PR pass, and/or all snapshots are up to date

Copy link
Contributor

@MaoShizhong MaoShizhong left a comment

Choose a reason for hiding this comment

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

Awesome work!

Further to the two points below, we also don't actually need the @discordjs/collection package. discord.js already exports Collection (which comes from their utility package), so we can just destructure Collection from the import, then when mocking 'discord.js', just have ...jest.requireActual('discord.js'), before our mocked properties.

e.g.

jest.mock('discord.js', () => ({
  ...jest.requireActual('discord.js'),
  Member: jest.fn().mockImplementation((roles) => ({
    roles: {
      cache: roles,
    },
  })),
}));

And the equivalent for points.test.js

services/getting-hired-message.service.js Outdated Show resolved Hide resolved
events/message-create.js Outdated Show resolved Hide resolved
@Asartea
Copy link
Contributor Author

Asartea commented Aug 6, 2024

✅ Done

Copy link
Contributor

@MaoShizhong MaoShizhong left a comment

Choose a reason for hiding this comment

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

Very much a nit, but mind renaming any memberMap variables in both test files to memberCollection?
Once that's done, the rest looks good to me! 🙏

@Asartea
Copy link
Contributor Author

Asartea commented Aug 6, 2024

@MaoShizhong MaoShizhong merged commit e9fd590 into TheOdinProject:main Aug 6, 2024
2 checks passed
@MaoShizhong
Copy link
Contributor

I didn't even realise I didn't formally approve. Well that's something to address.

@Asartea Asartea deleted the feat/add-is-admin-utility branch August 6, 2024 15:19
@Asartea Asartea mentioned this pull request Aug 6, 2024
7 tasks
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.

Abstract out isAdmin check to new utility in /utils
2 participants