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 endpoint to delete timezones #1343

Merged

Conversation

Poeticrow
Copy link
Contributor

@Poeticrow Poeticrow commented Mar 1, 2025

Pull Request

Description

This PR implements the timezone deletion feature in the TimezonesService and improves error handling.
Key changes:

  • Added a deleteTimezone method to remove a timezone by ID.
  • Ensured proper error handling:
    • Returns NotFoundException if the timezone does not exist.
    • Returns InternalServerErrorException only for unexpected errors.
  • Improved logging with Logger.error for better debugging.
  • Updated test cases:
    • Added unit tests for successful deletion.
    • Added test for NotFoundException when an invalid ID is provided.
    • Mocked repository methods correctly for expected behavior.
  • Fixed a bug where the catch block was overriding NotFoundException with InternalServerErrorException.

Related Issue

Fixes #1336

Type of Change

  • feat: New feature
  • fix: Bug fix
  • docs: Documentation updates
  • style: Code style/formatting changes
  • refactor: Code refactoring
  • perf: Performance improvements
  • test: Test additions/updates
  • chore: Build process or tooling changes
  • ci: CI configuration changes
  • other:

How Has This Been Tested?

  • Unit tests
  • Integration tests
  • Manual tests

Test Evidence

image

All unit tests are passing
Verified NotFoundException is thrown correctly
Verified correct success response for valid deletions

Screenshots (if applicable)

Documentation Screenshots (if applicable)

N/A (No UI changes)

Checklist

  • My code follows the project's coding style
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published
  • I have included a screenshot showing all tests passing
  • I have included documentation screenshots (if applicable)

Additional Notes

  • This change ensures that API consumers receive a clear error message when trying to delete a non-existent timezone.
  • No breaking changes expected, but API consumers should handle the NotFoundException properly.

…vements

- Added a  method in  to allow timezone deletion by ID.
- Implemented proper error handling:
  - Throws  if the timezone does not exist.
  - Ensures  is only thrown for unexpected errors.
- Updated test cases in  and :
  - Added unit tests for successful deletion.
  - Added test for  when an invalid ID is provided.
  - Mocked repository methods correctly to reflect expected behavior.
- Fixed a bug where the  block was overriding  with .
- Improved logging with  to track service-level errors efficiently.

BREAKING CHANGE: Ensure all consumers handle  correctly when deleting non-existent timezones.
@theblvckdev
Copy link
Contributor

Update your branch

@Poeticrow
Copy link
Contributor Author

Update your branch

It's done.

@theblvckdev
Copy link
Contributor

This PR title isn't following best practices for several reasons:

  1. Format inconsistency: While "Feat/" is attempting to follow conventional commits format, the standard would be "feat:" (with a colon, not a slash).

  2. Lack of clarity: The title is vague about whether you're adding or deleting a timezone endpoint.

A better PR title would be:

feat: add endpoint to delete timezones

This improved title:

  • Uses the proper conventional commit format with lowercase "feat:" and a colon
  • Clearly states the action (adding an endpoint) and its purpose (to delete timezones)
  • Removes uncertainty
  • Follows the imperative mood ("add" rather than "adding")

Following consistent PR naming conventions helps with release notes generation, makes the git history more readable, and helps team members quickly understand the purpose of changes.

@Poeticrow Poeticrow changed the title Feat/add delete timezone endpoint feat: add endpoint to delete timezones Mar 1, 2025
@Poeticrow
Copy link
Contributor Author

This PR title isn't following best practices for several reasons:

  1. Format inconsistency: While "Feat/" is attempting to follow conventional commits format, the standard would be "feat:" (with a colon, not a slash).
  2. Lack of clarity: The title is vague about whether you're adding or deleting a timezone endpoint.

A better PR title would be:

feat: add endpoint to delete timezones

This improved title:

  • Uses the proper conventional commit format with lowercase "feat:" and a colon
  • Clearly states the action (adding an endpoint) and its purpose (to delete timezones)
  • Removes uncertainty
  • Follows the imperative mood ("add" rather than "adding")

Following consistent PR naming conventions helps with release notes generation, makes the git history more readable, and helps team members quickly understand the purpose of changes.

Thank you. I've corrected it

@theblvckdev theblvckdev merged commit 05b3be2 into hngprojects:dev Mar 1, 2025
1 check passed
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.

[FEAT] Implement Timezone Deletion Functionality in the Timezone Module
2 participants