Use SiteURL for all calls from the webapp

[mickmister]

Use SiteURL for all calls from the webapp

[duke7able]

@mickmister can you simply state for beginners what domain to be replaced with what domain as this looks like domain string replacement issue, thanks

[mickmister]

@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 https://company.com/mattermost and so https://company.com/mattermost/api/users would be the resulting URL for example.

Have I made the task more clear?

[sudiptog81]

I can work on it if a PR for this does not already exist.

[larkox]

Thanks @sudiptog81 ! All yours 😄

[jasonblais]

Opening back up for grabs. You can find a PR that started work on this ticket at #58

[coltoneshaw]

@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 (term, options), but it seems in later versions this is changed - mattermost/mattermost-redux#1200.

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?

[jasonblais]

Thanks! Not sure, will defer to @mickmister and @DHaussermann who may know more.

[mickmister]

@coltoneshaw Is this the ticket you meant to comment on? Where does the searchTeams action come into play with this ticket?

[coltoneshaw]

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 mattermost-redux contained in the plugin. The link https://github.com/mattermost/mattermost-plugin-custom-attributes/blob/2800ede9ab3d5be1eaefb0b6701369c2d1a88737/webapp/src/components/admin_settings/teams_input/index.js#L12 does not accept the parameters that are passed to it based on the version of redux is what I'm seeing.

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.

[mickmister]

Okay I understand the searchTeams issue now. The redux function had changed its arguments, and this plugin's code assumed the new changes, but is still importing the old version of the function. It's possible updating the redux package would have similar side effects with other functions that are in the opposite situation of expecting the old version of functions. I can take a look at all the imports if you'd like or I can do that during PR review if you are submitting a PR.

Should/does the searchTeams issue have its own ticket? I'm still curious how the discussion ended up on this one

[mickmister]

@coltoneshaw Sorry I forgot to add the your mention above ^

[coltoneshaw]

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 searchTeam but not searchUsers. Probably my fault for moving the conversation here, happy to open a new issue. Interestingly enough it looks like it may have been introduced here - #50 when swapping to a versioned release.

If you download the plugin from the repo in its unpublished state now you'll also see the searchTeams error.

[mickmister]

@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

If you download the plugin from the repo in its unpublished state now you'll also see the searchTeams error.

Thank you for pointing this out 👍

[coltoneshaw]

@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.

[mickmister]

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