Cc/api3 #129
Cc/api3 #129
Conversation
Fix for detecting if workspace is orphaned.
…only O365 Group based workspaces)
| $logFileName = "$($_.BaseName)_$(Get-Date -Format 'yyyyMMdd_HHmmss').trx" | ||
| # Due to this issue, timestamp is being auto added by dotnet test which breaks AppVeyor from finding tests - https://github.com/Microsoft/vstest/issues/1951 | ||
| $logBaseName = $_.BaseName | ||
| $logFileName = "$($logBaseName).trx" # Depending how above issue is fixed, may need to add this back in _$(Get-Date -Format 'yyyyMMdd_HHmmss') |
There was a problem hiding this comment.
Inconsistent indentation.
| if(Test-Path -Path $resultsLog) { | ||
| Write-Host "Publishing test results..." | ||
| #Write-Host "ResultsDirectory: $ResultsDirectory" | ||
| #Write-Host "LogFileName: $logFileName" |
There was a problem hiding this comment.
This section of code is doubly indented. I would recommend removing these commented out console logs if you feel they aren't needed anymore.
| # https://www.appveyor.com/docs/running-tests/ | ||
| # https://www.appveyor.com/docs/running-tests/#pushing-real-time-test-results-to-build-console |
There was a problem hiding this comment.
Inconsistent indentation.
|
|
||
| if (result != null) | ||
| { | ||
| return (Table)result; |
There was a problem hiding this comment.
Could cast using as instead. I think that would help you avoid the if-else return null block.
| } | ||
|
|
||
| public enum CrossFilteringBehaviorEnum | ||
| { | ||
| OneDirection, | ||
| BothDirections, | ||
| Automatic, | ||
| NotAvailable |
There was a problem hiding this comment.
Is it intentional to remove NotAvailable from here?
| { | ||
| return this.Client.Groups.GetGroupsAsAdmin(expand, filter, top, skip).Value.Select(x => (Workspace)x); | ||
| return this.Client.Groups.GetGroupsAsAdmin(top, expand, filter, skip).Value.Select(x => (Workspace)x); |
There was a problem hiding this comment.
These parameters aren't named like other wrapped calls to the SDK so I'm not sure if the reordering is intentional or not.
| @@ -130,15 +130,15 @@ public void CopyPowerBIReportObjectTest() | |||
| var expectedResponse = new Report(); | |||
| var client = new Mock<IPowerBIApiClient>(); | |||
| client.Setup(x => x.Reports | |||
| .CopyReport(reportName, Guid.Empty.ToString(), reportId.ToString(), Guid.Empty.ToString(), Guid.Empty.ToString())) | |||
| .CopyReport(reportName, Guid.Empty, reportId, Guid.Empty, Guid.Empty)) | |||
There was a problem hiding this comment.
I know the original code did this, but we should avoid using blank/ambiguous values like this for our tests.
| @@ -154,7 +154,7 @@ private void GetWorkspaces() | |||
|
|
|||
| var workspaces = this.Scope == PowerBIUserScope.Individual ? | |||
| client.Workspaces.GetWorkspaces(filter: this.Filter, top: this.First, skip: this.Skip) : | |||
| client.Workspaces.GetWorkspacesAsAdmin(expand: "users", filter: this.Filter, top: this.First, skip: this.Skip); | |||
| client.Workspaces.GetWorkspacesAsAdmin(top: this.First.GetValueOrDefault(), expand: "users", filter: this.Filter, skip: this.Skip); | |||
There was a problem hiding this comment.
We should consider updating first to be a mandatory parameter so we don't have to do GetValueOrDefault() now that it's required by the SDK. We might be able to remove some of the logic that sets a default value since this is a new version.
| @@ -60,7 +60,7 @@ protected override void BeginProcessing() | |||
|
|
|||
| if (this.Scope == PowerBIUserScope.Organization) | |||
| { | |||
| this.Logger.WriteWarning($"Only preview workspaces are supported when -{nameof(this.Scope)} {nameof(PowerBIUserScope.Organization)} is specified"); | |||
| this.Logger.WriteWarning($"Only workspaces created in the new workspace experience are supported when -{nameof(this.Scope)} {nameof(PowerBIUserScope.Organization)} is specified"); | |||
There was a problem hiding this comment.
You might want to spot check that the other warning messages about v2 workspaces are consistent with this wording.
| public enum NewWorkspaceType | ||
| { | ||
| Workspace, | ||
| Group |
There was a problem hiding this comment.
Should we consider calling this Office365Group since it will be user facing?
Updating codebase to use PowerBI SDK v3 plus some new parameters to the cmdlets