Follow up to add nav parameter #23845

MarioJGMsoft · 2025-02-12T18:53:03Z

Description

This task is a follow up to #20323 .

When loop tried to implement the code some errors came up, the expected information was not being added.

Acceptance criteria
The link works properly when loop tries to implement and use it.

Execution plan
Update the methods we are using to receive the appName and the containerPackageName. Also update IODSPResolvedURL to be able to have the containerPackageName.

Reviewer Guidance

Please let me know if there's something that is missing or that I should keep in mind. Thanks!

Fixes: AB#29956

packages/drivers/odsp-driver/src/createFile/createFile.ts

packages/drivers/odsp-driver/src/createOdspCreateContainerRequest.ts

packages/drivers/odsp-driver/src/odspUtils.ts

packages/drivers/odsp-driver/src/odspDocumentServiceFactoryCore.ts

pragya91 · 2025-02-12T21:09:06Z

packages/drivers/odsp-driver/src/test/createNewUtilsTests.spec.ts

@@ -498,7 +486,6 @@ describe("Create New Utils Tests", () => {
 						false /* forceAccessTokenViaAuthorizationHeader */,
 						undefined /* isClpCompliantApp */,
 						true /* enableSingleRequestForShareLinkWithCreate */,
-						mockOdspResolvedUrl,
 					),
 				{
 					itemId: "mockItemId",


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.

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.

…functionality.

github-actions · 2025-02-18T22:38:47Z

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> [email protected] ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> [email protected] serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> [email protected] check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  160781 links
    1452 destination URLs
    1683 URLs ignored
       0 warnings
       0 errors

pragya91 · 2025-02-13T05:23:02Z

packages/drivers/odsp-driver/src/odspDriverUrlResolverForShareLink.ts

@@ -82,6 +83,7 @@ export class OdspDriverUrlResolverForShareLink implements IUrlResolver {
 			resolvedUrl: IOdspResolvedUrl,
 			dataStorePath: string,
 		) => Promise<string | undefined>,
+		private readonly containerPackageInfo?: IContainerPackageInfo | undefined,


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

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

pragya91 · 2025-02-13T05:30:07Z

packages/drivers/odsp-driver/src/odspDriverUrlResolverForShareLink.ts

@@ -257,7 +259,8 @@ export class OdspDriverUrlResolverForShareLink implements IUrlResolver {

 		const containerPackageName: string | undefined =
 			getContainerPackageName(packageInfoSource) ??
-			odspResolvedUrl.codeHint?.containerPackageName;
+			odspResolvedUrl.codeHint?.containerPackageName ??
+			getContainerPackageName(this.containerPackageInfo);


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).

Added change

pragya91 · 2025-02-19T00:00:16Z

packages/drivers/odsp-driver/src/createOdspCreateContainerRequest.ts

@@ -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}` : ""}`,


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

Applied change

pragya91 · 2025-02-19T00:02:16Z

packages/drivers/odsp-driver/src/test/createNewUtilsTests.spec.ts


 		// Compare the values to make sure that the nav parameter was added correctly
 		assert.equal(
 			decodedValue,
-			"s=%2FsiteUrl&d=driveId&f=mockItemId&c=%2F&fluid=1&a=mockAppName&p=mockContainerPackageName&x=mockContext",
+			"s=%2FsiteUrl&d=driveId&f=itemId&c=%2F&fluid=1&a=appName&p=containerPackageName&x=context",


@MarioJGMsoft , Let us also test for scenarios where appName is not provided, but all other params are. Same with containerPackageName. Just to ensure the nav param is not misconstructed.

Applied change

pragya91 · 2025-02-19T00:03:22Z

packages/drivers/odsp-driver/src/test/createNewUtilsTests.spec.ts

+			/nav=([^&]*)/,
+		)?.[1] as string;
+		// Decode the Base64 value to UTF-8, \r�� is being stored at the end of the string so we slice it off
+		const decodedValue = fromBase64ToUtf8(base64Value).slice(0, -3);


Where is \r�� coming from?

I'm not sure, it's being added when I decode the value of navParam, so I'm not sure at what point of the code flow it's being added to the encoded nav param. I don't think it can become an issue but let me know if you think otherwise.

pragya91 · 2025-02-19T00:06:12Z

packages/drivers/odsp-driver/src/odspDocumentServiceFactoryCore.ts

@@ -187,6 +187,7 @@ export class OdspDocumentServiceFactoryCore
 								!!this.hostPolicy.sessionOptions?.forceAccessTokenViaAuthorizationHeader,
 								odspResolvedUrl.isClpCompliantApp,
 								this.hostPolicy.enableSingleRequestForShareLinkWithCreate,
+								odspResolvedUrl,


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.

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.

pragya91 · 2025-02-19T00:13:03Z

packages/drivers/odsp-driver/src/test/createNewUtilsTests.spec.ts

+		);
+
+		const expectedResponse = {
+			context: "http://sp.devinstall/_api/v2.1/$metadata#",


Any reason for using this exact URL? Could we use a dummy one.

Yes, we could use a dummy one and I added the change.

pragya91 · 2025-02-19T00:14:15Z

packages/drivers/odsp-driver/src/test/createNewUtilsTests.spec.ts

+			identityType: "Enterprise",
+		};
+
+		const getContext = async (


The function is defined as async, but I don't see any promises used

In the constructor of OdspDriverUrlResolverForShareLink, getContext is a function call that returns a Promise of <strin | undefined>, so the getContext function has to be async.

pragya91 · 2025-02-19T00:16:06Z

packages/drivers/odsp-driver/src/test/createNewUtilsTests.spec.ts

+			logger,
+			"appName",
+			getContext,
+			{ name: "containerPackageName" },


{ name: "containerPackageName" } /*IContainerPackageInfo*/,

applied change

pragya91 · 2025-02-19T00:19:35Z

packages/drivers/odsp-driver/src/test/createNewUtilsTests.spec.ts

-		// Update the webUrl to the version that has the nav parameter that was supposed to be added
-		mockSharingLinkData.webUrl =
-			"https://mock.url/?nav=cz0lMkZzaXRlVXJsJmQ9ZHJpdmVJZCZmPW1vY2tJdGVtSWQmYz0lMkYmZmx1aWQ9MSZhPW1vY2tBcHBOYW1lJnA9bW9ja0NvbnRhaW5lclBhY2thZ2VOYW1lJng9bW9ja0NvbnRleHQ%3D";
+		const resolved = await shareLinkResolver.resolve(request);


Add a comment above this like explaining that we are trying to emulate the 'container' behavior where we first create a resolved object for the provided request to invoke driver's createContainer request.

Applied change

This reverts commit 3c2d546.

## Description This task is a follow up to [microsoft#20323](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/20323) . When loop tried to implement the code some errors came up, the expected information was not being added. Acceptance criteria The link works properly when loop tries to implement and use it. Execution plan Update the methods we are using to receive the appName and the containerPackageName. Also update IODSPResolvedURL to be able to have the containerPackageName. ## Reviewer Guidance Please let me know if there's something that is missing or that I should keep in mind. Thanks! Fixes: [AB#29956](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/29956) --------- Co-authored-by: Jatin Garg <[email protected]>

refactor: updated navParameter to function properly

4524e3a

github-actions bot added base: main PRs targeted against main branch area: driver Driver related issues area: odsp-driver public api change Changes to a public API labels Feb 12, 2025

refactor: Changed storage format for containerPackageName

8bd939d

MarioJGMsoft requested review from pragya91 and jatgarg February 12, 2025 19:20

jatgarg reviewed Feb 12, 2025

View reviewed changes

packages/drivers/odsp-driver/src/createFile/createFile.ts Show resolved Hide resolved

jatgarg reviewed Feb 12, 2025

View reviewed changes

packages/drivers/odsp-driver/src/createOdspCreateContainerRequest.ts Outdated Show resolved Hide resolved

jatgarg reviewed Feb 12, 2025

View reviewed changes

packages/drivers/odsp-driver/src/createOdspCreateContainerRequest.ts Outdated Show resolved Hide resolved

jatgarg reviewed Feb 12, 2025

View reviewed changes

packages/drivers/odsp-driver/src/odspUtils.ts Outdated Show resolved Hide resolved

jatgarg reviewed Feb 12, 2025

View reviewed changes

packages/drivers/odsp-driver/src/odspDocumentServiceFactoryCore.ts Outdated Show resolved Hide resolved

pragya91 reviewed Feb 12, 2025

View reviewed changes

refactor: Applied changes based on comments

690ccbe

github-actions bot removed the public api change Changes to a public API label Feb 12, 2025

refactor: added changes based on feedback

231644b

github-actions bot added the public api change Changes to a public API label Feb 12, 2025

MarioJGMsoft added 7 commits February 12, 2025 22:37

docs: updated params comment

894fb33

refactor: updated appendLocator params based on feedback

3449e65

Merge branch 'main' into navParamUpdate

ad2a948

refactor: added changes based on comments

7633994

docs: updated comment

522665b

test: updated test flow so that it works.

972520f

test: removed containerPackage name test since we no longer use that …

aca8cc3

…functionality.

jatgarg approved these changes Feb 18, 2025

View reviewed changes

jatgarg marked this pull request as ready for review February 18, 2025 22:09

jatgarg requested a review from a team as a code owner February 18, 2025 22:09

test: updated test name and added cleanup line

25e6edb

MarioJGMsoft requested a review from a team February 18, 2025 22:26

MarioJGMsoft requested review from pragya91, markfields, jason-ha, kian-thompson, WillieHabi and rajatch-ff and removed request for a team February 18, 2025 22:26

Merge branch 'main' into navParamUpdate

cf33cdd

MarioJGMsoft merged commit 3c2d546 into microsoft:main Feb 18, 2025
35 checks passed

MarioJGMsoft deleted the navParamUpdate branch February 18, 2025 23:20

pragya91 reviewed Feb 18, 2025

View reviewed changes

pragya91 reviewed Feb 19, 2025

View reviewed changes

MarioJGMsoft added a commit to MarioJGMsoft/FluidFramework that referenced this pull request Feb 19, 2025

Revert "Follow up to add nav parameter (microsoft#23845)"

75ce7a9

This reverts commit 3c2d546.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Follow up to add nav parameter #23845

Follow up to add nav parameter #23845

MarioJGMsoft commented Feb 12, 2025 •

edited by azure-boards bot

Loading

pragya91 Feb 12, 2025

jatgarg Feb 12, 2025

github-actions bot commented Feb 18, 2025

pragya91 Feb 13, 2025

MarioJGMsoft Feb 19, 2025

pragya91 Feb 13, 2025

MarioJGMsoft Feb 19, 2025

pragya91 Feb 19, 2025

MarioJGMsoft Feb 19, 2025

pragya91 Feb 19, 2025

MarioJGMsoft Feb 19, 2025

pragya91 Feb 19, 2025

MarioJGMsoft Feb 19, 2025

pragya91 Feb 19, 2025

MarioJGMsoft Feb 19, 2025

pragya91 Feb 19, 2025 •

edited

Loading

MarioJGMsoft Feb 19, 2025

pragya91 Feb 19, 2025

MarioJGMsoft Feb 19, 2025

pragya91 Feb 19, 2025

MarioJGMsoft Feb 19, 2025

pragya91 Feb 19, 2025

MarioJGMsoft Feb 19, 2025

Follow up to add nav parameter #23845

Follow up to add nav parameter #23845

Conversation

MarioJGMsoft commented Feb 12, 2025 • edited by azure-boards bot Loading

Description

Reviewer Guidance

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 18, 2025

linkcheck output

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pragya91 Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarioJGMsoft commented Feb 12, 2025 •

edited by azure-boards bot

Loading

pragya91 Feb 19, 2025 •

edited

Loading