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

Add check if group is M365 group to the Get-PnPMicrosoft365Group command #2258

Merged
merged 3 commits into from
Aug 24, 2022

Conversation

reusto
Copy link
Contributor

@reusto reusto commented Aug 21, 2022

Type

  • Bug Fix
  • New Feature
  • Sample

What is in this Pull Request ?

Added check if the group is a Microsoft 365 group when getting a group by ID with the Get-PnPMicrosoft365Group command.
This is to align the return of the command with Get-PnPMicrosoft365Group | Where-Object {$_.Id -eq '{guid}'

When a group with the specified ID exists that is not a Microsoft 365 group, then an error will be thrown including a hint to use the Get-PnPAzureADGroup command.

Update 24.08.2022:

Hint regarding usage of Get-PnPAzureADGroup command dropped due to different implementation.

Added fixes for the following bugs:

  • Get-PnPMicrosoft365Group returns a group if the mailNickname of the group matches the provided Identity even if the group is not a Microsoft 365 group
  • Get-PnPMicrosoft365Group doesn't return a group if the format of the provided identity matches the GUID format but is meant to be the display name or mail nickname

@gautamdsheth
Copy link
Collaborator

gautamdsheth commented Aug 23, 2022

hi @reusto , can we do something like this at API level itself instead of the results ?

.../v1.0/groups?$filter=id eq '02eea2f2-9cf9-4cac-80fb-07be37177f72' and groupTypes/any(c:c+eq+'Unified')

We can then check if its null or not.
If null, we can add a warning/error then ?
Would love to hear your thoughts on this 😊

@reusto
Copy link
Contributor Author

reusto commented Aug 24, 2022

hi @gautamdsheth , technicall it is possible to do this at the API level as you suggest.

I like your suggestion due to multiple reasons:

  • Performance seems to be similar to my original implementation.
  • It allows for code consolidation with the GetGroupAsync function when using display name.
  • It allows to have edge cases in mind like a user creating a group with a display name or mailnickname in the format of a GUID and then trying to get the group by display name or mail nickname, which is why I extended the filter query to include display name and mail nickname even when providing an input that seems like a GUID to
    .../v1.0/groups?$filter=groupTypes/any(c:c+eq+'Unified') and (id eq '{groupId}' or displayName eq '{groupId}' or mailNickName eq '{groupId}')

I already implemented this and created a new commit.

The commit also includes fixes for the following bugs that I found while looking into your suggestion:

  • Get-PnPMicrosoft365Group returns a group if the mailNickname of the group matches the provided Identity even if the group is not a Microsoft 365 group (this was due to missing brackets in the filter query when getting the group by displayname or mailnickname)
  • Get-PnPMicrosoft365Group doesn't return a group if the format of the provided identity matches the GUID format but is meant to be the display name or mail nickname

I used the implementation of the previous GetGroupAsync (by displayName) function with additions from the GetGroupAsync (by groupId) function, but also changed the behavior when no group is returned to throw an error. This is to match with the previous behavior of the GetGroupAsync (by groupId) function and to be consistent with the behavior of other commands in this module which also throw an error when an item with the provided identity can't be found. (for example Get-PnPListItem or Get-PnPList).

Due to the new implementation the hint regarding the usage of Get-PnPAzureAdGroup was dropped as we now can't differentiate wether a group is not returned because it doesn't exist or because it is not a Microsoft 365 group. But this shouldn't be a problem as it isn't relevant for the command and was previously just implemented as a nice to have.

@gautamdsheth gautamdsheth merged commit e81bb74 into pnp:dev Aug 24, 2022
@gautamdsheth
Copy link
Collaborator

Thank you so much @reusto !
Much appreciated 😊🙏

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.

2 participants