-
Notifications
You must be signed in to change notification settings - Fork 333
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
Command update - spo tenant settings set #5352
Conversation
Amazing work @ganesh-sanap 👏 Looking forward in using these new options (especially |
Thank you @ganesh-sanap, we'll try to review it ASAP! |
@milanholemans Any update on this PR? |
@ganesh-sanap not yet no. As you can see, we have quite some PRs to process at the moment and we only do this in our free time. Also, I haven't really had time for the past few days. We will try to process it ASAP. Thank you for being so patient. |
Hi @milanholemans, Any updates on this PR? Thank you! |
Hi @ganesh-sanap we had a busy month to release the new major version of CLI. Because this PR is quite large I wasn't able to include it in that release. Will try my best in the coming weeks to review it. Sorry for the delay. |
Hi, @ganesh-sanap it's been a while since this PR was raised. The last couple of months were quite busy for me. During this time, some PRs were merged and changed stuff to CLI. Could you rebase with the latest main version and fix the few errors? Thanks! |
@milanholemans Can you help with the steps or commands for rebasing with latest main branch to my local branch ( |
Hi @ganesh-sanap definitely! The best way to do this is by syncing your fork on GitHub with the latest changes and pulling these changes locally on your Then you switch back to your feature branch and: # Rebase to main
git rebase main
# Fix merge conflicts in VS Code, when ready click 'Continue' in VS Code.
# When done, push to GitHub, force flag is important! Don't push using VS Code.
git push --force You can add commits that are added later as usual. |
a9a8098
to
6a878e8
Compare
Thank you @milanholemans! I have done all the changes and marked this PR for your review. |
Thank you @ganesh-sanap! Will start reviewing in the coming days (looking at the size of the PR, it will take awhile). |
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.
Fantastic work so far @ganesh-sanap! Love to see all the new options. Because this PR will take some time to complete, I will review it in pieces.
Right now I've only reviewed the docs. I made a few comments along the way. Could you take a look at them, please?
One small point of attention, let's try to end every description with a period (.
), because they are all sentences.
Will try to review the code part of the PR ASAP in the coming weeks.
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 did a second review of the docs. I feel we're almost there, could you make these few adjustments before we proceed to the code?
Thank you for the awesome work! It's much appreciated.
: Allowed values `Edit`, `View`, `LimitedEdit`, `LimitedView`, `ManageList`, `None`, `Owner`, `RestrictedView`, `Review`, `Submit` | ||
|
||
`--ContainerLoopDefaultShareLinkRole [ContainerLoopDefaultShareLinkRole]` | ||
: Allowed values `Edit`, `View`, `LimitedEdit`, `LimitedView`, `ManageList`, `None`, `Owner`, `RestrictedView`, `Review`, `Submit` |
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.
There's indeed not much information. But since we have the description for ContainerDefaultShareLinkRole
I think this setting is the same but for Loop components.
: Allowed values `Anyone`, `Organization`, `SpecificPeople`, `Uninitialized` | ||
|
||
`--ContainerLoopDefaultShareLinkScope [ContainerLoopDefaultShareLinkScope]` | ||
: Allowed values `Anyone`, `Organization`, `SpecificPeople`, `Uninitialized` |
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.
Since we have the description for ContainerDefaultShareLinkScope
, we can use that one, only this setting is about Loop components.
@milanholemans Pushed commit with required changes, thanks for review! |
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.
Hi @ganesh-sanap review number 3 is here 😊
Please could you check the comments I made so far? Haven't been able to test all options yet. Tested the first 20 options and encountered 10 options with issues. Seems like we have the most issues with ...Role
options. So it could be a good idea to recheck all these options.
Thanks again for the great work so far!
@ganesh-sanap don't want to put any stress on you at all, but did you find some time to look at the comments I made? Maybe you have lost sight of this? I wouldn't want this valuable work to be lost. |
I will have a look and work on it @milanholemans. |
Hi @ganesh-sanap did you lose track of this? Don't want to put any pressure just double-checking. |
Hi @ganesh-sanap do you still want to work on this? No problem if you don't have enough time, we can always open it up for someone else. |
@milanholemans sorry for delay. I am working on it. I have few questions about changes requested, will ask separately. Thanks! |
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.
Hi @ganesh-sanap, this is a bit annoying, but I would really like to include a few useful options:
ExpireVersionsAfterDays
(number)MajorVersionLimit
(number)EnableAutoExpirationVersionTrim
(boolean)
You can find more info about them here: https://learn.microsoft.com/en-us/powershell/module/sharepoint-online/set-spotenant?view=sharepoint-ps
@milanholemans sure, I will look into those options. |
@ganesh-sanap are you still willing to work on this? How do you see this issue? Is it too big/too much work? Is it unclear? Are you still interested in it? You are welcome to answer honestly so we can find a way out. |
Hi @milanholemans, I have completed all PR review changes except restricting enum values for Regarding adding new options ( |
Is it that complicated? Aren't these just number and boolean fields? |
@milanholemans while passing the options ( I think we will have to do the |
Technically speaking, we should indeed. But is this really limited by the API itself? Does it throw an error if you don't? |
Hi @ganesh-sanap. I know this PR is taking a while, but I haven't forgotten about it. We're currently preparing a V8 release with CLI which consumed quite some time from my side. I will try to find some time to do another review for this big PR. Maybe in the meantime you could rebase with the latest main and resolve the merge conflict to make it go a little bit faster? |
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 did another partial review of the command logic @ganesh-sanap. Could you take a look at the comments I made, please?
Pardon me for the long silent period.
Can you also make sure that the code is rebased with the latest main, please?
SharingCapability: string; // <SharingCapabilities> | ||
NoAccessRedirectUrl?: string; | ||
ArchiveRedirectUrl?: string; | ||
ConditionalAccessPolicyErrorHelpLink?: 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.
NoAccessRedirectUrl?: string; | ||
ArchiveRedirectUrl?: string; | ||
ConditionalAccessPolicyErrorHelpLink?: string; | ||
CustomizedExternalSharingServiceUrl?: 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.
Can we set this value to null
when an empty string is passed?
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 comment is for which option/parameter exactly?
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.
Comments always apply to the last line displayed, so in this case, it should be CustomizedExternalSharingServiceUrl
.
ArchiveRedirectUrl?: string; | ||
ConditionalAccessPolicyErrorHelpLink?: string; | ||
CustomizedExternalSharingServiceUrl?: string; | ||
LabelMismatchEmailHelpLink?: 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.
Can we set this value to null
when an empty string is passed?
@@ -179,36 +326,70 @@ class SpoTenantSettingsSetCommand extends SpoCommand { | |||
#initTelemetry(): void { | |||
this.telemetry.push((args: CommandArgs) => { | |||
const telemetryProps: any = { |
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.
From what I can see, we're missing some telemetry options:
CoreRequestFilesLinkEnabled
OneDriveRequestFilesLinkEnabled
DisableDocumentLibraryDefaultLabeling
DisableVivaConnectionsAnalytics
DisplayNamesOfFileViewersInSpo
EnableAIPIntegration
EnableRestrictedAccessControl
ExternalUserExpirationRequired
HideSyncButtonOnDocLib
IncludeAtAGlanceInShareEmails
InformationBarriersSuspension
IsFluidEnabled
IsWBFluidEnabled
IsCollabMeetingNotesFluidEnabled
IsEnableAppAuthPopUpEnabled
IsLoopEnabled
OneDriveDefaultLinkToExistingAccess
ContainerDefaultLinkToExistingAccess
ReduceTempTokenLifetimeEnabled
ShowOpenInDesktopOptionForSyncedFiles
ShowPeoplePickerGroupSuggestionsForIB
SiteOwnerManageLegacyServicePrincipalEnabled
StopNew2010Workflows
StopNew2013Workflows
ViewersCanCommentOnMediaDisabled
AllowEveryoneExceptExternalUsersClaimInPrivateSite
AnyoneLinkTrackUsers
HasAdminCompletedCUConfiguration
HasIntelligentContentServicesCapability
HasTopicExperiencesCapability
MachineLearningCaptureEnabled
MassDeleteNotificationDisabled
MobileFriendlyUrlEnabledInTenant
EnableAutoNewsDigest
AllowCommentsTextOnEmailEnabled
CommentsOnFilesDisabled
DisableAddToOneDrive
DisableBackToClassic
DisablePersonalListCreation
ViewInFileExplorerEnabled
AllowGuestUserShareToUsersNotInSiteCollection
BlockSendLabelMismatchEmail
CoreDefaultLinkToExistingAccess
public getAllEnumOptions(): string[] { | ||
return ['SharingCapability', 'SharingDomainRestrictionMode', 'DefaultSharingLinkType', 'ODBMembersCanShare', 'ODBAccessRequests', 'FileAnonymousLinkType', 'FolderAnonymousLinkType', 'DefaultLinkPermission', 'ConditionalAccessPolicy', 'LimitedAccessFileType', 'SpecialCharactersStateInFileFolderNames']; | ||
return ['SharingCapability', 'CoreSharingCapability', 'ODBSharingCapability', 'ContainerSharingCapability', 'CoreDefaultShareLinkRole', 'CoreLoopDefaultSharingLinkRole', 'ContainerDefaultShareLinkRole', 'ContainerLoopDefaultShareLinkRole', 'OneDriveDefaultShareLinkRole', 'OneDriveLoopDefaultSharingLinkRole', 'CoreDefaultShareLinkScope', 'CoreLoopDefaultSharingLinkScope', 'ContainerDefaultShareLinkScope', 'ContainerLoopDefaultShareLinkScope', 'OneDriveDefaultShareLinkScope', 'OneDriveLoopDefaultSharingLinkScope', 'SharingDomainRestrictionMode', 'DefaultSharingLinkType', 'ODBMembersCanShare', 'ODBAccessRequests', 'AllowAnonymousMeetingParticipantsToAccessWhiteboards', 'FileAnonymousLinkType', 'FolderAnonymousLinkType', 'DefaultLinkPermission', 'ConditionalAccessPolicy', 'LimitedAccessFileType', 'MediaTranscription', 'MediaTranscriptionAutomaticFeatures', 'ImageTaggingOption', 'MarkNewFilesSensitiveByDefault', 'OCRModeForAdminSites', 'OCRModeForComplianceODBs', 'OCRModeForComplianceSites', 'SpecialCharactersStateInFileFolderNames']; | ||
} | ||
|
||
public getAllRoleOptions(): string[] { | ||
return ['CoreDefaultShareLinkRole', 'CoreLoopDefaultSharingLinkRole', 'ContainerDefaultShareLinkRole', 'ContainerLoopDefaultShareLinkRole', 'OneDriveDefaultShareLinkRole', 'OneDriveLoopDefaultSharingLinkRole']; | ||
} |
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.
Instead of functions, can't we use just an array like we do with boolean options?
private getMediaTranscriptionPolicyType(): string[] { return ['Enabled', 'Disabled']; } | ||
private getMediaTranscriptionAutomaticFeaturesPolicyType(): string[] { return ['Enabled', 'Disabled']; } | ||
private getImageTaggingChoice(): string[] { return ['Disabled', 'Basic', 'Enhanced']; } | ||
private getSensitiveByDefaultState(): string[] { return ['AllowExternalSharing', 'BlockExternalSharing']; } | ||
private getObjectCharacterRecognitionMode(): string[] { return ['Disabled', 'InclusionList', 'ExclusionList']; } | ||
private getSharingScope(): string[] { return ['Uninitialized', 'Anyone', 'Organization', 'SpecificPeople']; } |
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.
Instead of functions, let's use private variables.
this.getAllRoleOptions().indexOf(propertyKey) > -1) { | ||
const role: Role = Role[(propertyValue.trim() as keyof typeof Role)]; | ||
if (role === undefined || role === null) { | ||
return `${propertyKey} option has invalid value of "${propertyValue}". Allowed values are: ${this.RoleMap.join(", ")}.`; | ||
} | ||
} |
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.
Why do we need this? Normally all values listed in the autocomplete property should be correct input.
if (this.getAllRoleOptions().indexOf(optionKey) > -1) { | ||
// map enum values to int | ||
optionValue = Role[(args.options[optionKey].trim() as keyof typeof Role)].valueOf(); | ||
} |
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.
Can't we just treat these values as enums? This logic is already present.
propsXml += `<SetProperty Id="${id++}" ObjectPathId="7" Name="${optionKey}"><Parameter Type="String">${optionValue}</Parameter></SetProperty>`; | ||
} | ||
else { | ||
propsXml += `<SetProperty Id="${id++}" ObjectPathId="7" Name="${optionKey}"><Parameter Type="Null">${null}</Parameter></SetProperty>`; |
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.
Can't we simplify this?
propsXml += `<SetProperty Id="${id++}" ObjectPathId="7" Name="${optionKey}"><Parameter Type="Null">${null}</Parameter></SetProperty>`; | |
propsXml += `<SetProperty Id="${id++}" ObjectPathId="7" Name="${optionKey}"><Parameter Type="Null" /></SetProperty>`; |
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.
@milanholemans I am unable to find above code in my forked repository. I think I have lost the code changes from last review during rebase. Is there any way to bring it back, can you please help?
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.
What did you do exactly if you lost the code?
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.
@milanholemans Here are the steps I followed:
- Pulled all changes from
pnp:main
tomain/master
branch of my fork. - Cloned forked repository to my new laptop.
- Used
git rebase
command as you suggested above. - There were no merge conflicts shown, so I used
git push --force
.
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.
On your local machine, you can use git reflog
to see what actions you did on your repo. It will contain the git commands you did and all the git commit hashes you can restore. Unfortunately, that's something I cannot view for your repo.
If you are struggling with it, I think I'm able to restore the last 2 commits you made. I'm not sure how many commits this PR contained originally (probably more than 2).
@@ -637,6 +1137,15 @@ class SpoTenantSettingsSetCommand extends SpoCommand { | |||
}); | |||
propsXml += `<SetProperty Id="${id++}" ObjectPathId="7" Name="${optionKey}"><Parameter Type="Array">${valuesXml}</Parameter></SetProperty><Method Name="Update" Id="${id++}" ObjectPathId="7" />`; | |||
} | |||
else if (['LabelMismatchEmailHelpLink'].indexOf(optionKey) > -1) { |
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.
Is this something we could apply to all options instead of just this one?
Hi @ganesh-sanap, are you still willing to continue this PR? |
Yes, I will look into review comments. |
9cd907c
to
fb20cac
Compare
fb20cac
to
a1baff4
Compare
@ganesh-sanap With this in mind, would you be open to closing this PR for now? If you still wish to proceed, perhaps we could revisit this work in smaller, more manageable pieces, which might also speed up the review process. Let me know what you think |
Thank you so much for the time and effort you’ve put into this PR. Your contribution and dedication are greatly appreciated, and we truly value the work you’ve done. Unfortunately, as the PR has been inactive for a while and some progress was lost during the rebase, we’ve decided to close it for now. If you’d like to revisit this in the future or contribute in other ways, we’d be happy to collaborate and support you. Thanks again for your contribution to the project! |
Type
Related Issues?
Closes #5034
What is in this Pull Request ?
Added around 33 new options/parameters to
spo tenant settings set
cmdlet as discussed in issue #5034.