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

PB-871 : add debug UI and URL params to override URLs used by data integration team - #minor #1025

Merged
merged 7 commits into from
Aug 27, 2024
Merged

PB-871 : add debug UI and URL params to override URLs used by data integration team - #minor #1025

merged 7 commits into from
Aug 27, 2024

Conversation

pakb
Copy link
Contributor

@pakb pakb commented Aug 8, 2024 •
edited
Loading

@github-actions github-actions bot changed the title PB-871 : add debug UI and URL params to override URLs used by data integration team PB-871 : add debug UI and URL params to override URLs used by data integration team - #minor Aug 8, 2024
@cypress Cypress
Copy link

cypress bot commented Aug 8, 2024 •
edited
Loading

web-mapviewer    Run #3135

Run Properties:  status check passed Passed #3135  •  git commit 5061ab6bbf: PB-871 : keep base URL config objects private
Project web-mapviewer
Branch Review feat-PB-871-debug-tools-for-url
Run status status check passed Passed #3135
Run duration 05m 29s
Commit git commit 5061ab6bbf: PB-871 : keep base URL config objects private
Committer Pascal Barth
View all properties for this run ↗︎
Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 21
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 210
View all changes introduced in this branch ↗︎

@pakb pakb force-pushed the feat-PB-871-debug-tools-for-url branch 2 times, most recently from af6b508 to 4549f62 Compare August 13, 2024 12:47
@pakb pakb requested review from ltshb, rebert and ltkum August 13, 2024 13:31
ltshb
ltshb reviewed Aug 19, 2024
View reviewed changes
Copy link
Contributor

@ltshb ltshb left a comment

Choose a reason for hiding this comment

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

I like the idea of the debug modal to set the url backend.

However having the override check every where in code were we need the backend doesn't seem a good approach.

I would consider having a new approach, now the backend urls are hardcoded as constant in a javascript file, we should have a library that returns the backend urls for each backend, so instead of having API_BASE_URL we would have a function getApiBaseUrl() or even better having a singleton class Backends from which we could as for any backend base url. Then this class would be responsible to override any url if needed. In future we could also consider that the backend urls are not anymore hardcoded in the config but would be asked from the backend at map.geo.admin/api/backends

@ltclm
Copy link

ltclm commented Aug 22, 2024

great, thank you @pakb
one small question:
when setting the api_url to http://localhost:9176 as here:

https://sys-map.int.bgdi.ch/preview/feat-pb-871-debug-tools-for-url/index.html#/map?lang=de&center=2609348.52,1236799.55&z=4.585&bgLayer=ch.swisstopo.pixelkarte-farbe&topic=ech&swisssearch=SWISSIMAGE+Zeitreise&layers=ch.swisstopo.swissimage-product@year=2017;ch.swisstopo.swissimage-product.metadata@year=2017@features=206028&featureInfo=default&timeSlider=2017&api_url=http://localhost:9176/

the identify request is done with the localhost url but the htmlpopup is still done by sys-api3.int.bgdi.ch.

rebert
rebert approved these changes Aug 22, 2024
View reviewed changes
Copy link
Contributor

@rebert rebert left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This helps us a lot for the DI. Beside of the feedback from @ltclm about the popup, it seems to work fine. I'm looking forward to get it merged

@pakb pakb force-pushed the feat-PB-871-debug-tools-for-url branch from 4549f62 to 99dc934 Compare August 23, 2024 06:55
@pakb pakb requested a review from ltshb August 23, 2024 07:02
ltshb
ltshb reviewed Aug 26, 2024
View reviewed changes
Copy link
Contributor

@ltshb ltshb left a comment

Choose a reason for hiding this comment

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

I like your idea to split the config into several files 👍🏼

src/config/baseUrl.config.js Outdated Show resolved Hide resolved
src/config/baseUrl.config.js Outdated Show resolved Hide resolved
src/modules/menu/components/debug/BaseUrlOverrideModal.vue Outdated Show resolved Hide resolved
src/utils/layerUtils.js Outdated Show resolved Hide resolved
@pakb
PB-871 : add debug entries for URL override for main DI services
804ea1e 
supporting API3, WMS and WMTS base URL overrides.

Changing calls to functions using these base URLs from the config.js so that they now prioritize the debug value if defined.
@pakb
PB-871 : adding debug UI modal to change URL overrides
7430003
@pakb
PB-871 : adding debug URL params for URL override
780a727 
named like they were in the mf-geoadmin3's code
@pakb
PB-871 : moving function enforceEndingSlashInUrl back into config.js
c9ab30a 
it otherwise creates a circular dependency and some part of the tests can't load properly (LV95 is imported from config.js, which requires something in utils.js, which requires something in config.js, etc...)
@pakb
PB-871 : re-arranging app config into multiple files
8f5f880 
Trying to gather common config together in a meaningfully named config file.

Adding function to get base URLs, instead of direct access to constants. So that we have the opportunity to override base URLs. I didn't want to go through the store to do that. Doing so would create a lot of circular dependencies between the store, the config and components.
As it's a debug tool, that's "good enough" this way.
@pakb
PB-871 : code clean-up
e7746fc 
moving slash enforcing function back to where other base URLs are in the config
@pakb
PB-871 : keep base URL config objects private
5061ab6 
and only export/share function to access them.

Also clearing the layers config cache whenever a base URL changes. Changing the API3 URL would not trigger a new layer config load without that.
@pakb pakb force-pushed the feat-PB-871-debug-tools-for-url branch from 6e16b4f to 5061ab6 Compare August 27, 2024 09:12
@pakb pakb changed the base branch from master to develop August 27, 2024 09:12
@pakb pakb changed the title PB-871 : add debug UI and URL params to override URLs used by data integration team - #minor PB-871 : add debug UI and URL params to override URLs used by data integration team Aug 27, 2024
@github-actions github-actions bot changed the title PB-871 : add debug UI and URL params to override URLs used by data integration team PB-871 : add debug UI and URL params to override URLs used by data integration team - #minor Aug 27, 2024
@pakb pakb requested a review from ltshb August 27, 2024 09:29
ltshb
ltshb approved these changes Aug 27, 2024
View reviewed changes
Copy link
Contributor

@ltshb ltshb left a comment

Choose a reason for hiding this comment

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

Just change the one from my last comment and you good to merge

@pakb pakb merged commit a5e96ae into develop Aug 27, 2024
15 checks passed
@pakb pakb deleted the feat-PB-871-debug-tools-for-url branch August 27, 2024 10:08
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rebert rebert rebert approved these changes

@ltshb ltshb ltshb approved these changes

@ltkum ltkum Awaiting requested review from ltkum

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

Successfully merging this pull request may close these issues.
4 participants
@pakb @ltclm @rebert @ltshb