Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix insertion of access tokens, when swapping renderer #1021

Conversation

birkskyum
Copy link
Member

@birkskyum birkskyum commented Jan 21, 2025

Going from e.g. MapTiler to OpenLayers and back will lose the maptlier key.

This code finds the urls in the style that has "{key}" and insert the correct API keys

Fixes the error reported here, cc @nyurik

Related to:

Launch Checklist

  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Add an entry to CHANGELOG.md under the ## main section.

@codecov-commenter
Copy link

codecov-commenter commented Jan 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.37%. Comparing base (af01346) to head (42ba9dc).
Report is 57 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1021      +/-   ##
==========================================
+ Coverage   59.84%   64.37%   +4.53%     
==========================================
  Files         104      104              
  Lines        3011     5825    +2814     
  Branches      680     1724    +1044     
==========================================
+ Hits         1802     3750    +1948     
- Misses       1209     2073     +864     
- Partials        0        2       +2     

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

@HarelM
Copy link
Collaborator

HarelM commented Jan 22, 2025

Is there a test we can add to make sure this behavior is kept in the future?

@birkskyum birkskyum changed the title Fix key when going between maplbire and openlayers Fix insertion of access tokens, when swapping renderer Jan 22, 2025
@birkskyum birkskyum force-pushed the fix-api-key-when-going-between-openlayers-and-maplibre branch 2 times, most recently from ffce5cf to c612220 Compare January 22, 2025 11:47
@birkskyum
Copy link
Member Author

birkskyum commented Jan 22, 2025

I'm not sure how to test this through dom/cypress. It's quite different from the other tests, as it's about network calls and visual output on canvas.

Cypress clearly is capable of both (network and snapshots), I just have to study it.

@HarelM
Copy link
Collaborator

HarelM commented Jan 22, 2025

You can use Cypress to intercept network calls I believe.
Also you can check what is stored in the lical storage, or is the local storage content "correct"?

@birkskyum
Copy link
Member Author

@HarelM , figured out the test now. It checks the requests made after the render change has the access token instead of just {key}

@birkskyum birkskyum requested a review from HarelM January 23, 2025 12:40
@HarelM
Copy link
Collaborator

HarelM commented Jan 26, 2025

The test needs some cleaning up, otherwise this looks good, thanks!

@birkskyum birkskyum requested a review from HarelM January 27, 2025 14:14
@HarelM
Copy link
Collaborator

HarelM commented Jan 27, 2025

Firefox and cypress do not play together well, this is not the fire time the tests are passing with chrome but not with firefox, maybe we should drop support for firefox and hope it works well, IDK...

@birkskyum birkskyum force-pushed the fix-api-key-when-going-between-openlayers-and-maplibre branch from d53e162 to 4039ef0 Compare January 28, 2025 12:24
@birkskyum birkskyum force-pushed the fix-api-key-when-going-between-openlayers-and-maplibre branch from 4039ef0 to 42ba9dc Compare January 28, 2025 12:25
@birkskyum
Copy link
Member Author

@HarelM , I've removed firefox in this pr too.

@HarelM
Copy link
Collaborator

HarelM commented Jan 28, 2025

Great! Thanks!

@HarelM HarelM enabled auto-merge (squash) January 28, 2025 12:28
@birkskyum birkskyum disabled auto-merge January 28, 2025 12:49
@birkskyum
Copy link
Member Author

Not sure why it's still showing an "E2E tests using firefox"

@birkskyum
Copy link
Member Author

@HarelM , is this PR approved?

@HarelM
Copy link
Collaborator

HarelM commented Jan 28, 2025

It was required in the setting of the branch protection, I've removed it and fixed it now.

@birkskyum birkskyum enabled auto-merge (squash) January 28, 2025 12:54
@birkskyum birkskyum merged commit abf3bd1 into maplibre:main Jan 28, 2025
9 checks passed
@HarelM
Copy link
Collaborator

HarelM commented Feb 15, 2025

@birkskyum can you take a look at the added cypress test here?
I think it causes some CI instability and prevents automatic dependencies updates...

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switching to OpenLayers and back to MapLibre hides all styles
3 participants