-
Notifications
You must be signed in to change notification settings - Fork 318
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 a new command: 'spo site admin remove'. Closes #5884 #6062
base: main
Are you sure you want to change the base?
Conversation
Thank you, we'll try to review it ASAP! |
Here, the same change mentioned in this code review will be applied: using spo.getSiteId() to get the site ID in admin mode. |
@mkm17, a few conflicts arose with the latest changes. Could you take a look at them? |
677cf0f
to
566f0cc
Compare
@Jwaegebaert thank you for the reminder, I have corrected it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mkm17 awesome work so far 👏
I say this is in very good shape and I only added some small comments regarding code style and refactoring to remove duplicated logic.
Other than that seems like it is ready to be 🚢ed
const groupName = 'TestGroup'; | ||
|
||
|
||
before(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const groupName = 'TestGroup'; | |
before(() => { | |
const groupName = 'TestGroup'; | |
before(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
this.#initTelemetry(); | ||
this.#initOptions(); | ||
this.#initValidators(); | ||
this.#initOptionSets(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should also add this.#initTypes();
to initialize most of the options as string and the force
and asAdmin
as bool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
force: typeof args.options.force !== 'undefined', | ||
asAdmin: typeof args.options.asAdmin !== 'undefined' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for bools this approach may be good enough
force: typeof args.options.force !== 'undefined', | |
asAdmin: typeof args.options.asAdmin !== 'undefined' | |
force: !!args.options.force, | |
asAdmin: !!args.options.asAdmin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
public async commandAction(logger: Logger, args: CommandArgs): Promise<void> { | ||
try { | ||
if (!args.options.force) { | ||
const result = await cli.promptForConfirmation({ message: `Are you sure you want to remove specified user from the site administrators list ${args.options.siteUrl}?` }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this question may be misleading if we are removing a group 🤔.
Maybe what we could do is adapt the question and prompt for user when one of the user...
options was passed and ask for group when one of the group...
options was used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
|
||
private async callActionAsAdmin(logger: Logger, args: CommandArgs, loginNameToRemove: string): Promise<void> { | ||
await logger.logToStderr('Removing site administrator as an administrator...'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets log this kind of additional info when verbose
options was used. Otherwise lets keep the console output clean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
private async getPrimaryAdminLoginNameFromAdmin(adminUrl: string, siteId: string): Promise<string> { | ||
const requestOptions: CliRequestOptions = { | ||
url: `${adminUrl}/_api/SPO.Tenant/sites(%27${siteId}%27)?$select=OwnerLoginName`, | ||
headers: { | ||
accept: 'application/json;odata=nometadata', | ||
'content-type': 'application/json;charset=utf-8' | ||
} | ||
}; | ||
|
||
const response: string = await request.get<string>(requestOptions); | ||
const responseContent = JSON.parse(response); | ||
return responseContent.OwnerLoginName; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already have same code in site admin list
.
Lets try to remove duplicated logic and instead lets extract this to one of the utils
we have like spo
and use this in both commands from the util method 👍
This is a huge improvement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, I have changed it in both commands. So let's wait with reviewing of next commands in this scope after publishing this PR
|
||
private async getSiteAdmins(adminUrl: string, siteId: string): Promise<AdminUserResult[]> { | ||
const requestOptions: CliRequestOptions = { | ||
url: `${adminUrl}/_api/SPO.Tenant/GetSiteAdministrators?siteId=%27${siteId}%27`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do not have to encode single quotes or any other symbols when writing the url sorrounded by we may just write
siteId='${siteId}'` and it should work automagically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
let requestUrl: string = `https://graph.microsoft.com/v1.0/users/${options.userId}`; | ||
|
||
if (options.userName) { | ||
requestUrl = `https://graph.microsoft.com/v1.0/users('${formatting.encodeQueryParameter(options.userName)}')`; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a bit strange. Usually we extract common part and add to string rest of the url based on the options passed.
so we could do something like:
let requestUrl: string = `https://graph.microsoft.com/v1.0/users/${options.userId}`; | |
if (options.userName) { | |
requestUrl = `https://graph.microsoft.com/v1.0/users('${formatting.encodeQueryParameter(options.userName)}')`; | |
} | |
let requestUrl: string = `https://graph.microsoft.com/v1.0/users/`; | |
if (options.userName) { | |
requestUrl += formatting.encodeQueryParameter(options.userName); | |
} | |
else { | |
reuqestUrl += options.userId | |
} |
or to an inline if and have this in a single line as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it a little bit , in one case there is / and in second one is ( . Nice idea.
await request.post(requestOptions); | ||
} | ||
|
||
private async getPrimaryOwnerLoginFromSite(siteUrl: string): Promise<string> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also this we already have in the site admin list
.
Lets extract this to a new util
in spo
and resue it in both places 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
}); | ||
|
||
|
||
await assert.rejects(command.action(logger, { options: { siteUrl: siteUrl, userId: adminToRemoveId, asAdmin: true, force: true } }), new CommandError(`User not found.`)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}); | |
await assert.rejects(command.action(logger, { options: { siteUrl: siteUrl, userId: adminToRemoveId, asAdmin: true, force: true } }), new CommandError(`User not found.`)); | |
}); | |
await assert.rejects(command.action(logger, { options: { siteUrl: siteUrl, userId: adminToRemoveId, asAdmin: true, force: true } }), new CommandError(`User not found.`)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
566f0cc
to
c354530
Compare
c354530
to
752f3d9
Compare
Adds a new command: 'm365 spo site admin remove'.
Closes #5884
Related tasks #5874.
Similar PR #5926