Conversation
…or module release
rdonovan92
requested review from
Dr-North,
apetsche1,
asvalent,
baristaTam,
ddriddle,
edthedev,
mabaumgartner,
mpitcel,
spaldr,
tzturner,
wennebo1 and
zdc217
baristaTam
left a comment
baristaTam
left a comment
There was a problem hiding this comment.
This is awesome, Richard! I just have some easy cleanup items I commented on below. Let me know when these are taken care of and I'll approve!
| Authorization = "Bearer $($Script:BoxSession.AccessToken)" | ||
| } | ||
|
|
||
| $Form = @{ |
There was a problem hiding this comment.
Any reason not to just make Form an optional param on invoke-boxrestcall and simplify this?
There was a problem hiding this comment.
No preferences here, just something I didn't think about doing and would be happy to look at this as an improvement in the near future
| Returns a link to the folder. | ||
|
|
||
| .EXAMPLE | ||
| New-BoxFolderCollaboration -FolderName "Finance" -Collaborators $Users |
There was a problem hiding this comment.
This example seems to include a parameter that isn't valid, and seems to omit several parameters that are marked mandatory.
There was a problem hiding this comment.
Just updated this, nice catch!
| if ($Login.Count -ne $Role.Count) { | ||
| throw "Login and Role counts must match." | ||
| } |
There was a problem hiding this comment.
Making two arrays that match exactly to set permissions seems like it could be error prone if there are very many collaborators? Getting off by one might set a lot of incorrect permissions. You could consider for a future improvement taking an powershell object with key value pairs for the user and role such as:
$obj1 = {
user = "user@example.com"
role = "reader"
}Or alternately, parameters Login and Role could be set to only accept a single string instead of an array and then use ValueFromPipelineByPropertyName so that objects could be sent from the pipeline.
There was a problem hiding this comment.
I initially had tried the powershell object, but for some reason, i couldn't get it to send the login and role properly, this is something I would definitely like to improve on.
| Body = $FolderBody | ||
| } | ||
|
|
||
| $FolderResponse = Invoke-BoxRestCall @CreateFolder |
There was a problem hiding this comment.
Would this be more modular if it was a separate function? i.e. Have a function for New-BoxFolder and then turn this function into something like New-Collaborator (or whatever the proper pwsh verb might be)?
There was a problem hiding this comment.
This is probably the way, this was created initially with a very specific need in mind, but breaking is probably the way to go.
There was a problem hiding this comment.
That would also allow us to in the future add collaborators to an existing folder.
There was a problem hiding this comment.
Splitting into two functions and then using ValueFromPipelineByPropertyName for the add collaborator piece would keep the function login much simpler as well.
|
|
||
| $Target = "$($Login[$i]) as $($Role[$i])" | ||
|
|
||
| if ($PSCmdlet.ShouldProcess("Box Folder $FolderId", "Add collaborator $Target")) { |
There was a problem hiding this comment.
Just curious, Does this work if using the WhatIf flag? $FolderId wouldn't exist at this point, right? Or am I thinking about this wrong?
There was a problem hiding this comment.
This does indeed for example, it would look like:
What if: Performing the operation "Create folder 'BoxAPITest1'" on target "Box".
4 participants
Initial push, added first batch of functions and pester template for module release