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

Adds engage community user add. Closes #6293 #6388

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

Conversation

MathijsVerbeeck
Copy link
Contributor

Closes #6293

@MathijsVerbeeck MathijsVerbeeck linked an issue Sep 26, 2024 that may be closed by this pull request
@MathijsVerbeeck MathijsVerbeeck added the pr-major PR for the next major release label Sep 26, 2024
@Adam-it Adam-it changed the base branch from v10 to main September 30, 2024 13:28
@MathijsVerbeeck MathijsVerbeeck force-pushed the add-viva-engage-community-user-add branch from fb971cd to b4b41b5 Compare October 2, 2024 08:56
@martinlingstuyl martinlingstuyl self-assigned this Oct 4, 2024
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 @MathijsVerbeeck, got some comments for you. ⚡

`--userNames [userNames]`
: The user principal names of users. You can also pass a comma-separated list of UPNs. Specify either `ids` or `userNames` but not both.

`--entraGroupId [entraGroupId]`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say place this one right below communityDisplayName, as it's an optionset

async getCommunityIdByEntraGroupId(entraGroupId: string): Promise<string> {
const communities = await odata.getAllItems<Community>('https://graph.microsoft.com/v1.0/employeeExperience/communities?$select=id,groupId');

const filtereCommunities = communities.filter(c => c.groupId === entraGroupId);
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 use find() instead of filter() (and fix the typo in the const name)

Suggested change
const filtereCommunities = communities.filter(c => c.groupId === entraGroupId);
const filteredCommunities = communities.find(c => c.groupId === entraGroupId);

* @returns The ID of the Viva Engage group.
*/
async getEntraGroupIdByCommunityDisplayName(displayName: string): Promise<string> {
const communityId = await this.getCommunityIdByDisplayName(displayName);
Copy link
Contributor

Choose a reason for hiding this comment

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

You're doing in two requests what can be done in one. Or so it seems.

Also, I dont think there's real value in a helper method that only retrieves the entra group Id for a community. Let's rewrite getCommunityIdByDisplayName to getCommunityByDisplayName and use that to get the group and its Id.

* @param displayName Community display name.
* @returns The ID of the Viva Engage community.
*/
async getCommunityIdByDisplayName(displayName: string): Promise<string> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's more logical to just have a function that returns the community. So let's rename this one to getCommunityByDisplayName.
Of course it's important to only $select what we need. What you could do is add an optional selectProperties: string[] parameter on this function, so that it's possible to optimize the request (if necessary).

In this case we'd be able to call the function as follows: getCommunityByDisplayName("Some community", ["id"]).
In the case of retrieving the entra group id, we could run getCommunityByDisplayName("Some community", ["groupId"]).

* @param entraGroupId The ID of the Microsoft Entra group.
* @returns The ID of the Viva Engage community.
*/
async getCommunityIdByEntraGroupId(entraGroupId: string): Promise<string> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, let's return the entire community object by default, and let's allow to select specific properties using a selectProperties string[] parameter.

* @param communityId The ID of the Viva Engage community.
* @returns The ID of the Viva Engage group.
*/
async getEntraGroupIdByCommunityId(communityId: string): Promise<string> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets ditch this method in favor of reusing getCommunityByDisplayName('communityName', ['groupId'])

Copy link
Contributor

Choose a reason for hiding this comment

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

We can't drop this method since we use communityId to locate the correct community. Instead, I suggest aligning it with the other methods by making it more dynamic, like getCommunityById(communityId: string, selectProperties: string[]), and returning the community object. This will make it more future-proof for other scenarios. Thoughts?

@martinlingstuyl
Copy link
Contributor

Hi @MathijsVerbeeck, do you have the time to conclude the work on these two viva PR's soon? I'm in the presupposition they need to be included in v10. So I'd hope to merge them sometime soon.

@Jwaegebaert
Copy link
Contributor

@MathijsVerbeeck, since you're using the same util, I’ve already applied the changes @martinlingstuyl recommended. You can check it out here: #6371.

@martinlingstuyl
Copy link
Contributor

Whats the status here @MathijsVerbeeck? It's marked PR-major. But we'll need to do some work yet to include it.

@Adam-it Adam-it removed the pr-major PR for the next major release label Oct 30, 2024
@Adam-it
Copy link
Member

Adam-it commented Oct 30, 2024

I am removing the pr-major label from this PR as we agreed to proceed with this change the 'standard' way and the removal of the group commands as breaking change that will target v11

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: viva engage community user add
4 participants