Skip to content

Conversation

@VaibhavSingh8
Copy link
Contributor

@VaibhavSingh8 VaibhavSingh8 commented Nov 4, 2024

Date: 04 Nov 2024

Developer Name: @VaibhavSingh8


Issue Ticket Number

Description

This PR implements the ability for super users to delete Discord groups through the /groups endpoint on the dashboard site. This feature allows for better group management.

  • Added the deleteGroupRole model function to handle the soft deletion of group roles in the Firestore database.
  • The deletion is implemented as a soft delete, setting isDeleted: true along with deletion metadata.
  • Implemented controller logic to handle deletion requests, verify role existence, log deletion events for audit, and manage responses with appropriate status codes.
  • Added comprehensive test coverage for both success and error scenarios.

Documentation Updated?

  • Yes
  • No

Under Feature Flag

  • Yes
  • No

Database Changes

  • Yes
  • No

Breaking Changes

  • Yes
  • No

Development Tested?

  • Yes
  • No

Screenshots

Screenshot 1
test-1.mp4
test-2.mp4

Test Coverage

Screenshot 1 Screenshot 2024-11-20 at 2 46 54 AM Screenshot 2024-11-20 at 2 48 24 AM Screenshot 2024-11-20 at 2 48 48 AM

Additional Notes

@VaibhavSingh8 VaibhavSingh8 changed the title Feat/delete group role Feat: Super users can delete discord group roles Nov 5, 2024
@VaibhavSingh8
Copy link
Contributor Author

Hi @yesyash @vinit717 ,

Please review my PR.

Thank you!

@vinit717
Copy link
Member

vinit717 commented Nov 6, 2024

Hi @yesyash @vinit717 ,

Please review my PR.

Thank you!

Try to be in Discord so we can track this, ask @ankushdharkar for further assistance

@pankajjs
Copy link
Contributor

pankajjs commented Nov 9, 2024

Screenshot 2024-11-09 at 12 54 36 PM

This is failing.

@pankajjs
Copy link
Contributor

pankajjs commented Nov 9, 2024

Test is missing for controller.

Copy link
Member

@vinit717 vinit717 left a comment

Choose a reason for hiding this comment

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

Add a proper test coverage and tests of the code you made

@VaibhavSingh8
Copy link
Contributor Author

Add a proper test coverage and tests of the code you made

@vinit717 Can you please elaborate more on this as in what to add?

@vinit717
Copy link
Member

Add a proper test coverage and tests of the code you made

@vinit717 Can you please elaborate more on this as in what to add?

Proper test coverage report of the code/file you changed

@VaibhavSingh8
Copy link
Contributor Author

Add a proper test coverage and tests of the code you made

@vinit717 Can you please elaborate more on this as in what to add?

Proper test coverage report of the code/file you changed

@vinit717 updated the reports in the PR description. Do I need to add tests for the controller too, because I can't find any written earlier for any controller?

@vinit717
Copy link
Member

updated the reports in the PR description. Do I need to add tests for the controller too, because I can't find any written earlier for any controller?

Integration test itself is controller tests make logic parts in models so you can test it

@VaibhavSingh8
Copy link
Contributor Author

updated the reports in the PR description. Do I need to add tests for the controller too, because I can't find any written earlier for any controller?

Integration test itself is controller tests make logic parts in models so you can test it

Hi @vinit717

I've implemented the requested changes. Please review the PR.

Thank you!

@pankajjs
Copy link
Contributor

Are we not integrating the Discord API as we discussed? @VaibhavSingh8

@VaibhavSingh8
Copy link
Contributor Author

VaibhavSingh8 commented Nov 13, 2024

Are we not integrating the Discord API as we discussed? @VaibhavSingh8

The timeline for this PR is concise, so as I requested yesterday, let's proceed with the original advice by Ankush to do it in a separate PR.

@pankajjs
Copy link
Contributor

Okay, I will pick in new task.

@pankajjs
Copy link
Contributor

Left few comments @VaibhavSingh8

@pankajjs
Copy link
Contributor

pankajjs commented Nov 16, 2024

-In the working video, include the discord part as well.
Left few comments @VaibhavSingh8

@VaibhavSingh8
Copy link
Contributor Author

-In the working video, include the discord part as well. Left few comments @VaibhavSingh8

Fixed

Copy link
Contributor

@pankajjs pankajjs left a comment

Choose a reason for hiding this comment

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

LGTM

@iamitprakash iamitprakash merged commit 24d91f5 into RealDevSquad:develop Nov 21, 2024
3 checks passed
byt3quester pushed a commit to byt3quester/website-backend that referenced this pull request Dec 2, 2024
* added route, controller and model for deleting the group role

* added route, controller and model for deleting the group role

* unit tests for deleteGroupRole

* updated unit tests for deleteGroupRole

* integrated feature flag

* chore: roleid fetch from firestore and removed console logs

* integration tests for deleteGroupRole

* fix: reduced db calls from 2 to 1 for roleId

* feat: integrated discord service to delete role from discord

* test: unit and integration tests for discord service integration

* chore: refactored controller and tests to use res.boom and routes to use devFlag middleware

* fix: error code
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.

Feature: Super user can delete Discord groups

4 participants