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.

Sign up for GitHub

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

Follow up to add nav parameter #23845

Merged
merged 13 commits into from
Feb 18, 2025
Merged

Follow up to add nav parameter #23845

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
4524e3a
refactor: updated navParameter to function properly
MarioJGMsoft Feb 12, 2025
8bd939d
refactor: Changed storage format for containerPackageName
MarioJGMsoft Feb 12, 2025
690ccbe
refactor: Applied changes based on comments
MarioJGMsoft Feb 12, 2025
231644b
refactor: added changes based on feedback
MarioJGMsoft Feb 12, 2025
894fb33
docs: updated params comment
MarioJGMsoft Feb 12, 2025
3449e65
refactor: updated appendLocator params based on feedback
MarioJGMsoft Feb 12, 2025
ad2a948
Merge branch 'main' into navParamUpdate
MarioJGMsoft Feb 12, 2025
7633994
refactor: added changes based on comments
MarioJGMsoft Feb 12, 2025
522665b
docs: updated comment
MarioJGMsoft Feb 12, 2025
972520f
test: updated test flow so that it works.
MarioJGMsoft Feb 14, 2025
aca8cc3
test: removed containerPackage name test since we no longer use that …
MarioJGMsoft Feb 18, 2025
25e6edb
test: updated test name and added cleanup line
MarioJGMsoft Feb 18, 2025
cf33cdd
Merge branch 'main' into navParamUpdate
jatgarg Feb 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/drivers/odsp-driver/api-report/odsp-driver.legacy.alpha.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ export class OdspDriverUrlResolver implements IUrlResolver {

// @alpha
export class OdspDriverUrlResolverForShareLink implements IUrlResolver {
constructor(shareLinkFetcherProps?: ShareLinkFetcherProps | undefined, logger?: ITelemetryBaseLogger, appName?: string | undefined, getContext?: ((resolvedUrl: IOdspResolvedUrl, dataStorePath: string) => Promise<string | undefined>) | undefined);
constructor(shareLinkFetcherProps?: ShareLinkFetcherProps | undefined, logger?: ITelemetryBaseLogger, appName?: string | undefined, getContext?: ((resolvedUrl: IOdspResolvedUrl, dataStorePath: string) => Promise<string | undefined>) | undefined, containerPackageInfo?: IContainerPackageInfo | undefined);
appendDataStorePath(requestUrl: URL, pathToAppend: string): string | undefined;
appendLocatorParams(baseUrl: string, resolvedUrl: IResolvedUrl, dataStorePath: string, packageInfoSource?: IContainerPackageInfo): Promise<string>;
static createDocumentUrl(baseUrl: string, driverInfo: OdspFluidDataStoreLocator): string;
Expand Down
2 changes: 1 addition & 1 deletion packages/drivers/odsp-driver/src/createFile/createFile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ export async function createNewFluidFile(

odspResolvedUrl.context = resolvedUrl?.context;
odspResolvedUrl.appName = resolvedUrl?.appName;
odspResolvedUrl.codeHint = resolvedUrl?.codeHint;
odspResolvedUrl.codeHint = odspResolvedUrl.codeHint ?? resolvedUrl?.codeHint;

if (shareLinkInfo?.createLink?.link) {
let newWebUrl = shareLinkInfo.createLink.link.webUrl;
Expand Down
4 changes: 2 additions & 2 deletions packages/drivers/odsp-driver/src/createOdspCreateContainerRequest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
} from "@fluidframework/driver-definitions/internal";
import { ISharingLinkKind } from "@fluidframework/odsp-driver-definitions/internal";

import { buildOdspShareLinkReqParams, getContainerPackageName } from "./odspUtils.js";
import { buildOdspShareLinkReqParams } from "./odspUtils.js";

/**
* Create the request object with url and headers for creating a new file on OneDrive Sharepoint
Expand All @@ -37,7 +37,7 @@ export function createOdspCreateContainerRequest(
const createNewRequest: IRequest = {
url: `${siteUrl}?driveId=${encodeURIComponent(driveId)}&path=${encodeURIComponent(
filePath,
)}${containerPackageInfo ? `&containerPackageName=${getContainerPackageName(containerPackageInfo)}` : ""}${shareLinkRequestParams ? `&${shareLinkRequestParams}` : ""}`,
)}${shareLinkRequestParams ? `&${shareLinkRequestParams}` : ""}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

@MarioJGMsoft Linter might be unhappy about the unused containerPackageInfo - you can rename the parameter starting with an _ to avoid the issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied change

headers: {
[DriverHeader.createNew]: {
fileName,
Expand Down
1 change: 1 addition & 0 deletions packages/drivers/odsp-driver/src/odspDocumentServiceFactoryCore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ export class OdspDocumentServiceFactoryCore
!!this.hostPolicy.sessionOptions?.forceAccessTokenViaAuthorizationHeader,
odspResolvedUrl.isClpCompliantApp,
this.hostPolicy.enableSingleRequestForShareLinkWithCreate,
odspResolvedUrl,
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though it is not related to this PR, I really would like us to change the name of this variable from odspResolveUrl to something else that represents the original name. Something like resolvedUrlForCreateNew. Let me know if that doesn't sound right to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that the name can and should be changed to something more related, but I also think that it might be best to leave that change to another PR. Will create the follow up task for it.

)
: module.createNewContainerOnExistingFile(
getAuthHeader,
Expand Down
5 changes: 4 additions & 1 deletion packages/drivers/odsp-driver/src/odspDriverUrlResolverForShareLink.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ export class OdspDriverUrlResolverForShareLink implements IUrlResolver {
* navigates directly to the link.
* @param getContext - callback function which is used to get context for given resolved url. If context
* is returned then it will be embedded into url returned by getAbsoluteUrl() method.
* @param containerPackageInfo - container package information which will be used to extract the container package name.
*/
public constructor(
shareLinkFetcherProps?: ShareLinkFetcherProps | undefined,
Expand All @@ -82,6 +83,7 @@ export class OdspDriverUrlResolverForShareLink implements IUrlResolver {
resolvedUrl: IOdspResolvedUrl,
dataStorePath: string,
) => Promise<string | undefined>,
private readonly containerPackageInfo?: IContainerPackageInfo | undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

| undefined type shouldn't be needed here since the property is already optional right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm guessing it's there because the call to get context might return undefined, but the function can still exist.

) {
this.logger = createOdspLogger(logger);
if (shareLinkFetcherProps) {
Expand Down Expand Up @@ -257,7 +259,8 @@ export class OdspDriverUrlResolverForShareLink implements IUrlResolver {

const containerPackageName: string | undefined =
getContainerPackageName(packageInfoSource) ??
odspResolvedUrl.codeHint?.containerPackageName;
odspResolvedUrl.codeHint?.containerPackageName ??
getContainerPackageName(this.containerPackageInfo);
Copy link
Contributor

Choose a reason for hiding this comment

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

Add comment explaining why the chaining here is done.

Something like, "containerPackage information can be either provided via the constructor or to the method's params. We give priority to the locally provided params, and fallback to the constructor provided value" (re-phrase maybe to make it more understandable).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added change


odspResolvedUrl.appName = this.appName;

Expand Down
13 changes: 0 additions & 13 deletions packages/drivers/odsp-driver/src/test/createNewUtilsTests.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,18 +472,6 @@ describe("Create New Utils Tests", () => {
await epochTracker.removeEntries().catch(() => {});
});
it("Should set the appropriate nav param info when a resolved url is sent", async () => {
const mockOdspResolvedUrl: IOdspResolvedUrl = {
...resolvedUrl,
odspResolvedUrl: true,
summarizer: true,
dataStorePath: "/dataStorePath",
codeHint: {
containerPackageName: "mockContainerPackageName",
},
fileVersion: "mockFileVersion",
context: "mockContext",
appName: "mockAppName",
};
const odspResolvedUrl = await useCreateNewModule(createChildLogger(), async (module) =>
mockFetchOk(
async () =>
Expand All @@ -498,7 +486,6 @@ describe("Create New Utils Tests", () => {
false /* forceAccessTokenViaAuthorizationHeader */,
undefined /* isClpCompliantApp */,
true /* enableSingleRequestForShareLinkWithCreate */,
mockOdspResolvedUrl,
),
{
itemId: "mockItemId",
Copy link
Contributor

Choose a reason for hiding this comment

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

After you have made the review changes above, let's add a test with the following flow:

  • initialize OdspDriverUrlResolverForShareLink() and use it to create a resolve the request generated via createOdspCreateContainerRequest
  • mock attach() api response
  • then in the container.resolvedUrl - check if we have all the desired properties.

Checkout examples in odspCreateContainer.spec.ts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since in the unit test, there is no container, you can alternatively check service. resolvedUrl. Service is returned by the createContainer method in the serviceFactory.

Expand Down
Loading