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

Jump to bottom

feat: add backwards compatibility for sheetId #318

Merged
merged 8 commits into from Jul 17, 2023
Merged

feat: add backwards compatibility for sheetId #318

merged 8 commits into from Jul 17, 2023

Conversation

setchy
Copy link
Contributor

@setchy setchy commented May 21, 2023 •
edited

Support both documentId with sheetId for backwards compatibility

@setchy setchy mentioned this pull request May 21, 2023
Closed
@marisahoenig
Copy link
Member

Hey @setchy — I'd love to get this merged in. Would you first be able to add a unit test to make sure the docId works? The ones testing the sheetId are in sheet-spec.js.

@setchy
Copy link
Contributor Author

setchy commented Jul 7, 2023

@marisahoenig - unit tests added 😄

marisahoenig
marisahoenig reviewed Jul 7, 2023
View reviewed changes
Copy link
Member

@marisahoenig marisahoenig left a comment

Choose a reason for hiding this comment

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

I think the changes are great. I sent a message to Devansh to just double check this — will get this merged in next week.

spec/util/urlUtils-spec.js
@@ -32,4 +32,40 @@ describe('Url Utils', () => {
expect(config).toHaveBeenCalledTimes(1)
expect(queryParams).toHaveBeenCalledTimes(1)
})

it('should prioritize documentId before legacy sheetId', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Great idea for this unit test!

src/util/factory.js Outdated
const queryParams = queryString ? QueryParams(queryString[0]) : {}
sheet = GoogleSheet(queryParams.sheetId, queryParams.sheetName)
}
const queryString = window.location.href.match(/sheetId(.*)/)
Copy link
Member

Choose a reason for hiding this comment

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

Love the simplification

@setchy setchy changed the title feat: add back backwards compatibility feat: add backwards compatibility for sheetId Jul 9, 2023
@devansh-sharma-tw
Copy link
Contributor

@setchy , can you pls add the regex check for documentId as well here in factory.js line 539 (similar to the change in urlUtils) ? Unless it was skipped for some specific case ?

Currently, this partially breaks the flow of Switching Accounts when accessing a private Google Sheet with the wrong user.

Thanks!

@setchy
Copy link
Contributor Author

setchy commented Jul 17, 2023

@setchy , can you pls add the regex check for documentId as well here in factory.js line 539 (similar to the change in urlUtils) ? Unless it was skipped for some specific case ?

Currently, this partially breaks the flow of Switching Accounts when accessing a private Google Sheet with the wrong user.

Thanks!

Good pick up @devansh-sharma-tw 👏. Not sure how best to validate locally, but I've pushed a change that will use the sheetName function now

@devansh-sharma-tw
Copy link
Contributor

@setchy getSheetName does work now.
Although I think you have pushed some other changes in src/graphing/components/quadrantTables.js and src/graphing/radar.js not related to this PR. If you can clean those up, we should be good to merge this :)

Not sure how best to validate locally

One way to validate this locally (which I use) with 2 Google accounts is like this:

  • Create a private Google Sheet with multiple sheets with Account A
  • Open it in BYOR with a specific sheet (so URL would be like localhost/?documentId=<Account A private sheet>&sheetName=<some sheet name>)
  • Login with Account B
  • It should take you to the "Switch Account" screen
  • Then logging in correctly with Account B should render the radar with the correct sheet (not the default sheet)

@setchy
Copy link
Contributor Author

setchy commented Jul 17, 2023

Oops 🙈. All cleaned up now 🧼

@devansh-sharma-tw
Copy link
Contributor

Looks good to merge now. Thanks!

@devansh-sharma-tw devansh-sharma-tw merged commit 3e35804 into thoughtworks:master Jul 17, 2023
2 checks passed
setchy added a commit to setchy/build-your-own-radar that referenced this pull request Jul 19, 2023
@setchy
Squashed commit of the following:
8a8d470 
commit 3e35804
Merge: 28bc969 2b19a19
Author: Devansh Sharma <81564342+devansh-sharma-tw@users.noreply.github.com>
Date:   Mon Jul 17 18:32:13 2023 +0530

    Merge pull request thoughtworks#318 from setchy/feature/doc-sheet-backwards-compatibility

    feat: add backwards compatibility for sheetId

commit 2b19a19
Author: Adam Setch <adam.setch@outlook.com>
Date:   Mon Jul 17 05:31:54 2023 -0700

    revert unrelated changes

commit 93528e6
Author: Adam Setch <adam.setch@outlook.com>
Date:   Mon Jul 17 05:31:05 2023 -0700

    revert unrelated changes

commit 28bc969
Merge: ee16efd 1b5bc31
Author: Devansh Sharma <81564342+devansh-sharma-tw@users.noreply.github.com>
Date:   Mon Jul 17 11:54:25 2023 +0530

    Merge pull request thoughtworks#324 from setchy/feature/vscode-extension-recommendations

    feat: update vscode extension recommendations

commit ee16efd
Merge: af85830 7a6c915
Author: Devansh Sharma <81564342+devansh-sharma-tw@users.noreply.github.com>
Date:   Mon Jul 17 11:51:58 2023 +0530

    Merge pull request thoughtworks#323 from setchy/fix/vscode-formatting-setup

    fix: vscode ide setup

commit 0ea3dcc
Author: Adam Setch <adam.setch@outlook.com>
Date:   Sun Jul 16 22:02:49 2023 -0700

    refactor

commit af85830
Merge: f6637ac a1ef1bf
Author: Devansh Sharma <81564342+devansh-sharma-tw@users.noreply.github.com>
Date:   Mon Jul 17 10:19:53 2023 +0530

    Merge pull request thoughtworks#334 from setchy/docs/remove-closed-survey

    docs: remove survey now that its closed

commit c1987b8
Author: Adam Setch <adam.setch@outlook.com>
Date:   Sun Jul 16 14:27:42 2023 -0700

    refactor: simplify

commit bbd2f9a
Author: Adam Setch <adam.setch@outlook.com>
Date:   Fri Jul 14 16:36:43 2023 -0700

    fix linting

commit 0954c6b
Author: Adam Setch <adam.setch@outlook.com>
Date:   Fri Jul 14 16:34:40 2023 -0700

    feat: support backwards compatibility

commit a1ef1bf
Author: Adam Setch <adam.setch@outlook.com>
Date:   Fri Jul 14 06:52:33 2023 -0700

    docs: remove survey now that its closed

commit 7a6c915
Author: Adam Setch <adam.setch@outlook.com>
Date:   Tue Jul 11 07:40:33 2023 -0400

    add prettier enabled

commit 1b5bc31
Author: Adam Setch <adam.setch@outlook.com>
Date:   Sat Jul 8 15:24:01 2023 -0400

    feat: update vscode extension recommendations

commit df07ccc
Author: Adam Setch <adam.setch@outlook.com>
Date:   Sat Jul 8 15:23:14 2023 -0400

    revert extensions

commit e54f290
Author: Adam Setch <adam.setch@outlook.com>
Date:   Sat Jul 8 15:21:06 2023 -0400

    fix: vscode ide setup

commit f22216c
Author: Adam Setch <adam.setch@outlook.com>
Date:   Fri Jul 7 18:22:16 2023 -0400

    tests: add unit tests

commit fd81095
Author: Adam Setch <adam.setch@outlook.com>
Date:   Sun May 21 16:30:44 2023 -0400

    feat: add back backwards compatibility
setchy added a commit to setchy/build-your-own-radar that referenced this pull request Jul 28, 2023
@setchy
Squashed commit of the following:
430b343 
commit a9eee15
Author: Adam Setch <adam.setch@outlook.com>
Date:   Tue Jul 25 17:14:40 2023 -0400

    build: add codeowners (thoughtworks#327)

commit 67b359e
Author: Adam Setch <adam.setch@outlook.com>
Date:   Tue Jul 25 17:08:52 2023 -0400

    docs: add @setchy to contributors and sort alphabetically (thoughtworks#329)

commit a99ff7a
Author: Adam Setch <adam.setch@outlook.com>
Date:   Fri Jul 21 18:33:29 2023 -0400

    fix: correct case for function name (thoughtworks#332)

commit 67dd1b6
Merge: 3e35804 3f21854
Author: Devansh Sharma <81564342+devansh-sharma-tw@users.noreply.github.com>
Date:   Thu Jul 20 09:49:49 2023 +0530

    Merge pull request thoughtworks#335 from thoughtworks/dependabot/npm_and_yarn/word-wrap-1.2.4

    Bump word-wrap from 1.2.3 to 1.2.4

commit 3f21854
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Wed Jul 19 06:43:41 2023 +0000

    Bump word-wrap from 1.2.3 to 1.2.4

    Bumps [word-wrap](https://github.com/jonschlinkert/word-wrap) from 1.2.3 to 1.2.4.
    - [Release notes](https://github.com/jonschlinkert/word-wrap/releases)
    - [Commits](jonschlinkert/word-wrap@1.2.3...1.2.4)

    ---
    updated-dependencies:
    - dependency-name: word-wrap
      dependency-type: indirect
    ...

    Signed-off-by: dependabot[bot] <support@github.com>

commit 3e35804
Merge: 28bc969 2b19a19
Author: Devansh Sharma <81564342+devansh-sharma-tw@users.noreply.github.com>
Date:   Mon Jul 17 18:32:13 2023 +0530

    Merge pull request thoughtworks#318 from setchy/feature/doc-sheet-backwards-compatibility

    feat: add backwards compatibility for sheetId

commit 2b19a19
Author: Adam Setch <adam.setch@outlook.com>
Date:   Mon Jul 17 05:31:54 2023 -0700

    revert unrelated changes

commit 93528e6
Author: Adam Setch <adam.setch@outlook.com>
Date:   Mon Jul 17 05:31:05 2023 -0700

    revert unrelated changes

commit 28bc969
Merge: ee16efd 1b5bc31
Author: Devansh Sharma <81564342+devansh-sharma-tw@users.noreply.github.com>
Date:   Mon Jul 17 11:54:25 2023 +0530

    Merge pull request thoughtworks#324 from setchy/feature/vscode-extension-recommendations

    feat: update vscode extension recommendations

commit ee16efd
Merge: af85830 7a6c915
Author: Devansh Sharma <81564342+devansh-sharma-tw@users.noreply.github.com>
Date:   Mon Jul 17 11:51:58 2023 +0530

    Merge pull request thoughtworks#323 from setchy/fix/vscode-formatting-setup

    fix: vscode ide setup

commit 0ea3dcc
Author: Adam Setch <adam.setch@outlook.com>
Date:   Sun Jul 16 22:02:49 2023 -0700

    refactor

commit af85830
Merge: f6637ac a1ef1bf
Author: Devansh Sharma <81564342+devansh-sharma-tw@users.noreply.github.com>
Date:   Mon Jul 17 10:19:53 2023 +0530

    Merge pull request thoughtworks#334 from setchy/docs/remove-closed-survey

    docs: remove survey now that its closed

commit c1987b8
Author: Adam Setch <adam.setch@outlook.com>
Date:   Sun Jul 16 14:27:42 2023 -0700

    refactor: simplify

commit bbd2f9a
Author: Adam Setch <adam.setch@outlook.com>
Date:   Fri Jul 14 16:36:43 2023 -0700

    fix linting

commit 0954c6b
Author: Adam Setch <adam.setch@outlook.com>
Date:   Fri Jul 14 16:34:40 2023 -0700

    feat: support backwards compatibility

commit a1ef1bf
Author: Adam Setch <adam.setch@outlook.com>
Date:   Fri Jul 14 06:52:33 2023 -0700

    docs: remove survey now that its closed

commit 7a6c915
Author: Adam Setch <adam.setch@outlook.com>
Date:   Tue Jul 11 07:40:33 2023 -0400

    add prettier enabled

commit 1b5bc31
Author: Adam Setch <adam.setch@outlook.com>
Date:   Sat Jul 8 15:24:01 2023 -0400

    feat: update vscode extension recommendations

commit df07ccc
Author: Adam Setch <adam.setch@outlook.com>
Date:   Sat Jul 8 15:23:14 2023 -0400

    revert extensions

commit e54f290
Author: Adam Setch <adam.setch@outlook.com>
Date:   Sat Jul 8 15:21:06 2023 -0400

    fix: vscode ide setup

commit f22216c
Author: Adam Setch <adam.setch@outlook.com>
Date:   Fri Jul 7 18:22:16 2023 -0400

    tests: add unit tests

commit fd81095
Author: Adam Setch <adam.setch@outlook.com>
Date:   Sun May 21 16:30:44 2023 -0400

    feat: add back backwards compatibility
@setchy setchy deleted the feature/doc-sheet-backwards-compatibility branch March 31, 2024 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marisahoenig marisahoenig marisahoenig left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@setchy @marisahoenig @devansh-sharma-tw