-
Notifications
You must be signed in to change notification settings - Fork 23
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
Use SiteURL for all calls from the webapp #43
Comments
@mickmister can you simply state for beginners what domain to be replaced with what domain as this looks like domain string replacement issue, thanks |
@duke7able By default, the domain is blank in the request. For instance, an example might be: fetch('/api/users'); But we want const siteURL = getSiteURL(state);
fetch(siteURL + '/api/users'); There is no hardcoded domain to put here because each server is self-hosted, and thus will have a different domain. The main reason why this is needed is for deployments that host their server on a subpath. The site URL would be something like Have I made the task more clear? |
I can work on it if a PR for this does not already exist. |
Thanks @sudiptog81 ! All yours 😄 |
Opening back up for grabs. You can find a PR that started work on this ticket at #58 |
@jasonblais / @DHaussermann - Looking into this one a bit it looks like it's related to the version of Mattermost Reduce in the package. The version that is included right now (5.26) does not accept the searchTeams parameter of I updated the version locally and tested and it seems to be working fine. Do we know if there is an overall impact in bumping the version up? |
Thanks! Not sure, will defer to @mickmister and @DHaussermann who may know more. |
@coltoneshaw Is this the ticket you meant to comment on? Where does the |
Sorry, the original solution - #58 that is above appears to be a functioning solution to this issue. However, the issue isn't with that PR but with the version of So, my curiosity is what impact would increasing the redux package version have on this plugin? I have a functioning version of this with the fix for this issue on an updated redux package. |
Okay I understand the Should/does the |
@coltoneshaw Sorry I forgot to add the your mention above ^ |
It looks like the discussion ended up here because when @DHaussermann was testing a fix for this issue it was throwing an error on the If you download the plugin from the repo in its unpublished state now you'll also see the |
@coltoneshaw Okay now I think I actually understand 😅 There was an issue discussed in the PR and you tracked down the root cause of that issue 🎉 Yes I think this should have its own issue for tracking. Updating the mm-redux repo should do the trick as you have noted and confirmed works
Thank you for pointing this out 👍 |
@mickmister - Sorry for that! I originally thought it was just a fix for that PR but the more I tested the more it just seemed outdated on redux. I put in a PR / issue for it - #68 I do think the original code for this specific issue can be resubmitted as it does work with subpaths. Not sure what you think the best process for that is. |
@coltoneshaw It seems fine to have both issues solved in one PR, since one of the fixes is just a dependency update |
If the server's SiteURL is configured with a subpath, the webapp's API calls do not work. The task here is to make this plugin's client to prefix its URLs with the SiteURL.
Here's an example of how the SiteURL may be computed:
https://github.com/mattermost/mattermost-plugin-jira/blob/19a9c2442817132b4eee5c77e259b80a40188a6a/webapp/src/selectors/index.js#L13-L26
The text was updated successfully, but these errors were encountered: