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

New command: outlook mailbox settings set. Closes #6208 #6550

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

MartinM85
Copy link
Contributor

Closes #6208

@milanholemans
Copy link
Contributor

Thank you for the PR @MartinM85! I notice that the workflows are failing due to the issue you've raised in a discussion. We'll try to look into that ASAP. Until fixed, let's mark the PR as a draft until it's ready.

@milanholemans milanholemans marked this pull request as draft January 6, 2025 19:41
@MartinM85 MartinM85 marked this pull request as ready for review January 7, 2025 07:02
@martinlingstuyl martinlingstuyl self-assigned this Jan 10, 2025
Copy link
Contributor

@martinlingstuyl martinlingstuyl left a comment

Choose a reason for hiding this comment

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

Hi @MartinM85. Looks great! Could you look at my comments before we merge this?


```md definition-list
`-i, --userId [userId]`
: The ID of the Microsoft Entra user to update mailbox settings for. Specify either `userId` or `userName`, but not both. This option is required when using application permissions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Either userId or userName is required when using application permissions.
Can you also use the options when using delegated permissions?

If not I'd rewrite it slightly. If yes, I'd rewrite it slightly as well to: This is required when using application permissions. to avoid giving the impression that they're both required...

: The time format for the user's mailbox. Example: `H:mm`.

`--timeZone [timeZone]`
: The default time zone for the user's mailbox. Should follow [Windows time zone name](https://learn.microsoft.com/en-us/windows-hardware/manufacture/desktop/default-time-zones?view=windows-11#time-zones) or [IANA time zone identifier](https://en.wikipedia.org/wiki/List_of_tz_database_time_zones#List). Example: `Central Europe Standard Time`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
: The default time zone for the user's mailbox. Should follow [Windows time zone name](https://learn.microsoft.com/en-us/windows-hardware/manufacture/desktop/default-time-zones?view=windows-11#time-zones) or [IANA time zone identifier](https://en.wikipedia.org/wiki/List_of_tz_database_time_zones#List). Example: `Central Europe Standard Time`.
: The default time zone for the user's mailbox. Should follow [Windows time zone name](https://learn.microsoft.com/windows-hardware/manufacture/desktop/default-time-zones?view=windows-11#time-zones) or [IANA time zone identifier](https://en.wikipedia.org/wiki/List_of_tz_database_time_zones#List). Example: `Central Europe Standard Time`.

: The time of the day that the user stops working. Example: `17:00:00.000000`.

`--workingHoursTimeZone [workingHoursTimeZone]`
: The name of a time zone to which the working hours apply. Should follow [Windows time zone name](https://learn.microsoft.com/en-us/windows-hardware/manufacture/desktop/default-time-zones?view=windows-11#time-zones) or [IANA time zone identifier](https://en.wikipedia.org/wiki/List_of_tz_database_time_zones#List). Example: `Central Europe Standard Time`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
: The name of a time zone to which the working hours apply. Should follow [Windows time zone name](https://learn.microsoft.com/en-us/windows-hardware/manufacture/desktop/default-time-zones?view=windows-11#time-zones) or [IANA time zone identifier](https://en.wikipedia.org/wiki/List_of_tz_database_time_zones#List). Example: `Central Europe Standard Time`.
: The name of a time zone to which the working hours apply. Should follow [Windows time zone name](https://learn.microsoft.com/windows-hardware/manufacture/desktop/default-time-zones?view=windows-11#time-zones) or [IANA time zone identifier](https://en.wikipedia.org/wiki/List_of_tz_database_time_zones#List). Example: `Central Europe Standard Time`.

}

public async commandAction(logger: Logger, args: CommandArgs): Promise<void> {
const data: any = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move constructing the data object to a private function for readability...

: The days of the week on which the user works, separated by a comma. Allowed values are `monday`, `tuesday`, `wednesday`, `thursday`, `friday`, `saturday`, or `sunday`.

`--workingHoursStartTime [workingHoursStartTime]`
: The time of the day that the user starts working. Example: `09:00:00.000000`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Writing down the entire time string doesn't seem to me very user friendly.

Can we write it in multiple formats?

  • 09:00
  • 09:00:00
  • 09:00:00.0000000

If the API doesn't accept that we should make it work by:

  • allowing users to use all three time formats
  • validating the option input and throwing an error if the format is incorrect...

That would make for better usability I think.

@martinlingstuyl martinlingstuyl marked this pull request as draft January 10, 2025 14:50
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.

New command: outlook mailbox settings set
3 participants