Skip to content

Conversation

@jake-low
Copy link

@jake-low jake-low commented Jul 8, 2025

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation
  • πŸ§‘β€πŸ’» Refactor
  • βœ… Test
  • πŸ€– Build or CI
  • ❓ Other (please specify)

Describe this PR

This is a tweak to the changes made in #6933. That PR works correctly AFAIK for tasks.hotosm.org's use case, but I discovered when backporting the fix to osmus/tasking-manager (which powers tasks.openstreetmap.us and tasks.teachosm.org) that it does not work if the backend is deployed to a nonstandard URL path.

This change makes the callback URL string a relative path (removing the hardcoded /api/v2 prefix). The appropriate prefix will then be prepended in fetchLocalJSONAPI() when the string is converted to a URL object. This prefix may be /api/v2 or it may be another value depending on the configuration environment variables provided at runtime.

This change is running in production on tasks.openstreetmap.us and tasks.teachosm.org already, so I am fairly confident that it works.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 8, 2025

@ramyaragupathy
Copy link
Member

@suzit-10 can you please review this PR?

@suzit-10
Copy link
Contributor

suzit-10 commented Sep 4, 2025

Thank you @jake-low for raising this πŸ™
Removing the hardcoded /api/v2 is definitely a good improvement, and it’s very helpful for instances with non-standard backend URL (e.g., without /api/v2).
Besides the authorization URL, there are a few other places with the same prefix β€” could you update those too? I think it will also need a small tweak to fetchLocalJSONAPI(), since it’s used across the codebase, and simply prepending may not work in all cases (e.g., where api/v2 isn’t expected).

Really appreciate you taking the time to look into this πŸ™Œ

@jake-low
Copy link
Author

Hi @suzit-10, thanks for reviewing this. I unfortunately don't have time to revisit this PR and modify or retest it. I believe it fixes a genuine bug, although it may not fix all occurrences of that bug since I was focused only on fixing the code introduced in #6933.

Feel free to either merge this PR or close it at your discretion.

@suzit-10
Copy link
Contributor

suzit-10 commented Sep 18, 2025

Hi @jake-low, thanks for the update! I appreciate you taking the time to address this. I'll proceed with the required changes across the code and merge the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants