Skip to content
This repository was archived by the owner on Jul 9, 2025. It is now read-only.

Implemented All Requests For Voice Messaging Endpoints#223

Open
KiPageFault wants to merge 16 commits intomainfrom
215-voicemail-utility-scripts
Open

Implemented All Requests For Voice Messaging Endpoints#223
KiPageFault wants to merge 16 commits intomainfrom
215-voicemail-utility-scripts

Conversation

@KiPageFault
Copy link
Collaborator

@KiPageFault KiPageFault commented Mar 19, 2025

Checklist

  • Reference a related issue in the repository.
  • Provide a description of the proposed changes.
  • Ensure that code follows the project's style guidelines. Github linter checks should pass.
  • Update documentation as necessary.
  • Verify that all tests pass.

Reviewer Only

  • Code Quality

    • Are variable and function names descriptive and follow naming conventions?
    • Is the code free of redundant or duplicate segments?
    • Does the code adhere to the project's style guidelines? Github linter checks shold pass.
  • Functionality

    • Does the code perform the intended functions correctly?
    • Are all possible edge cases handled appropriately?
  • Testing

    • Are there unit tests covering new or modified functionalities?
    • Do all existing and new tests pass successfully?
    • Are Github checks passing?
  • Documentation

    • Is the code adequately commented to explain complex sections?
    • Have docstrings been used in functions/ methods?
    • Do docstrings follow convention throughout solution?
    • Has relevant documentation been updated or created?
  • Dependencies

    • Are new dependencies justified and documented?
    • Is the impact of these dependencies on the overall project considered?
  • Performance

    • Is the code optimised for performance without compromising readability?
  • Security

    • Are there any security implications introduced by the changes?
    • Have security best practices been followed?
  • Merge Preparations

    • Is the branch up-to-date with the base branch?
    • Are there any merge conflicts that need resolution?

@KiPageFault KiPageFault linked an issue Mar 19, 2025 that may be closed by this pull request
@KiPageFault KiPageFault reopened this Mar 19, 2025
Billy and others added 6 commits March 25, 2025 10:27
Created File Checking And Loading CSV's Via Pandas In Utility

Added Additional File Exceptions

Polished Voice Messaging Endpoints
Fixed None Pythonic Evaluation To Satisfy Lint Error
Removed Log Formatting When Specifying No Raw Type To Satisfy Lint Error
Removed Invalid Log Formatting To Satisfy Lint Error
@KiPageFault
Copy link
Collaborator Author

Added All Necessary Documentation

Copy link
Owner

@Jordan-Prescott Jordan-Prescott left a comment

Choose a reason for hiding this comment

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

Some adjustments

self,
service_provider_id: str,
group_id: str,
service_pack_id: str = None
Copy link
Owner

Choose a reason for hiding this comment

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

Although 14 are good at service packs for specific users we don't know if other companies are. Can you remove this param and update the method accordingly. I would change this to a list of users and set to None, if the user passes us the list of users we will do for those only if not we will apply to all. - What do you think?

self,
service_provider_id: str,
group_id: str,
new_password: str
Copy link
Owner

Choose a reason for hiding this comment

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

Same here as above should we add a list of users? Additionally we could have new_password as optional and if they do not specify one we could use the api.generate_password to get one/ a unique for each. Final thought we would need to pass these details back to the user.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The optional password is a great idea! However for the list of users. The alteration of vm_portal_bulk_password_set in the powered by odins spear docs will allow users to perform this action themselves

)

logger.info("Successfully Set Group User Portal Passcodes")

Copy link
Owner

Choose a reason for hiding this comment

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

Same as above is there anything we would return to the user?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I dont believe there is anything to tangibly return bar a list of affected users. Due to the scripts nature it is impossible to return the users with their new password as it is it obfuscated in the request

Copy link
Owner

@Jordan-Prescott Jordan-Prescott left a comment

Choose a reason for hiding this comment

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

Still some parts to iron these out


def main(
api, service_provider_id: str, group_id: str, users: list, password_type: str
api, service_provider_id: str, group_id: str, users: list, password_type: str, static_password: str
Copy link
Owner

Choose a reason for hiding this comment

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

I like this static password param nice work - have you updated docs to reflect this both in codebase and the docs online?

@@ -0,0 +1,54 @@
def main(api, service_provider_id: str, group_id: str, service_pack_id: str = None):
Copy link
Owner

Choose a reason for hiding this comment

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

Commented on this above, the service pack id here I think we should remove.

Copy link
Owner

@Jordan-Prescott Jordan-Prescott left a comment

Choose a reason for hiding this comment

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

Few bits but there are a lot of changes here so it may take us a couple of times to get it there.

Copy link
Owner

Choose a reason for hiding this comment

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

This needs removing - be careful about what you commit and push.

What's useful is having another environment on your computer where you have the latest v of OS for these kind of things.

@@ -0,0 +1,574 @@
from .base_endpoint import BaseEndpoint

class VoiceMessaging(
Copy link
Owner

Choose a reason for hiding this comment

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

is there no POST methods in this endpoint?

group_id: str,
users: list,
password_type: str,
static_password: Optional[str] = None
Copy link
Owner

Choose a reason for hiding this comment

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

Have you tested this with each password type? This script updates 3 types of passwords


def vm_auto_login(
self,
service_provider_id: str,
Copy link
Owner

Choose a reason for hiding this comment

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

Can we add a param here to specify users and if no users specified we auto login all?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Voicemail Utility Scripts

2 participants