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.

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

#8791: Migrate /api/me/ from version 1.0 to 1.1 #8793

Closed
wants to merge 7 commits into from

Conversation

johnnymetz
Copy link
Collaborator

@johnnymetz johnnymetz commented Jul 10, 2024

What does this PR do?

  • Closes Slice 2: Remove telemetry organization from Extension #8791
  • Replace telemetry organization with the new primary organization's isEnterprise field
  • Use an axios interceptor to inject the API version header into the request, instead of injecting it at all the calls sites which is messy and error-prone
  • Update swagger

Demo

https://www.loom.com/share/390fccf83dc34902a26490107d28f80c

@johnnymetz johnnymetz self-assigned this Jul 10, 2024
Copy link

codecov bot commented Jul 10, 2024

Codecov Report

Attention: Patch coverage is 40.00000% with 15 lines in your changes missing coverage. Please review.

Project coverage is 74.55%. Comparing base (8318d74) to head (65ae845).
Report is 243 commits behind head on main.

Files with missing lines Patch % Lines
.../components/floatingActions/initFloatingActions.ts 0.00% 5 Missing ⚠️
src/data/service/apiClient.ts 16.66% 5 Missing ⚠️
src/data/service/apiVersioning.ts 42.85% 4 Missing ⚠️
src/data/service/errorService.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8793      +/-   ##
==========================================
+ Coverage   74.24%   74.55%   +0.30%     
==========================================
  Files        1332     1338       +6     
  Lines       40817    41290     +473     
  Branches     7634     7730      +96     
==========================================
+ Hits        30306    30782     +476     
+ Misses      10511    10508       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Jul 10, 2024

Playwright test results

failed  8 failed
passed  105 passed
flaky  1 flaky
skipped  6 skipped

Details

report  Open report ↗︎
stats  120 tests across 40 suites
duration  10 minutes, 52 seconds
commit  65ae845
info  For more information on how to debug and view this report, see our readme

Failed tests

chrome › tests/extensionConsole/activation.spec.ts › can activate a mod with built-in integration
edge › tests/extensionConsole/activation.spec.ts › can activate a mod with built-in integration
chrome › tests/pageEditor/copyMod.spec.ts › run a copied mod with a built-in integration
edge › tests/pageEditor/copyMod.spec.ts › run a copied mod with a built-in integration
chrome › tests/smoke/floatingActionButton.spec.ts › sidebar page smoke test › can toggle the sidebar from the floating action button and view the related mod's sidebar panel
chrome › tests/smoke/floatingActionButton.spec.ts › sidebar page smoke test › can hide the floating action button
edge › tests/smoke/floatingActionButton.spec.ts › sidebar page smoke test › can toggle the sidebar from the floating action button and view the related mod's sidebar panel
edge › tests/smoke/floatingActionButton.spec.ts › sidebar page smoke test › can hide the floating action button

Flaky tests

edge › tests/regressions/welcomeStarterBricks.spec.ts › #8740: can view the starter mods on the pixiebrix.com/welcome page

Skipped tests

chrome › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor
edge › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor
chrome › tests/runtime/googleSheetsIntegration.spec.ts › can activate a google spreadsheet mod with config options
edge › tests/runtime/googleSheetsIntegration.spec.ts › can activate a google spreadsheet mod with config options
chrome › tests/runtime/insertAtCursor.spec.ts › Insert at Cursor › 8154: can insert at cursor after opening sidebar from selection menu
edge › tests/runtime/insertAtCursor.spec.ts › Insert at Cursor › 8154: can insert at cursor after opening sidebar from selection menu

Copy link
Contributor

@BLoe BLoe left a comment

Choose a reason for hiding this comment

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

It looks like all the places where you are adding the me version header are using getApiClient(), and also this is going to get messy if we start versioning every call site of every various endpoint like this. Should we start setting up an "API versioning" architecture within the api client where it attaches versioning headers based on match patterns for the request urls, or something like that that's more maintainable/extensible than manually adding the headers everywhere?

@twschiller twschiller added this to the 2.0.6 milestone Jul 10, 2024
@johnnymetz johnnymetz force-pushed the feature/8791-remove-telemetry-organization branch from 8f1588f to 37e3e9a Compare July 11, 2024 18:13
@johnnymetz johnnymetz force-pushed the feature/8791-remove-telemetry-organization branch from 2cdfdc0 to 36b1f6b Compare July 11, 2024 22:47
@johnnymetz johnnymetz changed the title #8791: Remove telemetry organization #8791: Migrate /api/me/ from version 1.0 to 1.1 Jul 11, 2024
@johnnymetz johnnymetz force-pushed the feature/8791-remove-telemetry-organization branch from 5e26621 to a5f905b Compare July 15, 2024 15:13
@twschiller twschiller removed this from the 2.0.6 milestone Jul 16, 2024
src/data/service/apiClient.ts Outdated Show resolved Hide resolved
@johnnymetz
Copy link
Collaborator Author

Closing in favor of #9074 b/c it's easier to start from scratch than resolve the merge conflicts

@johnnymetz johnnymetz closed this Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slice 2: Remove telemetry organization from Extension
4 participants