search

mattermost / mattermost-redux

Redux for Mattermost
Apache License 2.0
200 stars 386 forks source link

[Server #16802]: Update string values to boolean in "client config". #1348

Closed haardikdharma10 closed 3 years ago

haardikdharma10 commented 3 years ago

Summary

Updated string values to boolean in ClientConfig

Ticket Link

Fixes : https://github.com/mattermost/mattermost-server/issues/16802

mattermod commented 3 years ago

Hello @haardikdharma10,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

devinbinnie commented 3 years ago

Thanks for the PR @haardikdharma10! Looks like some of the unit tests are failing, please run make test on your local machine and have a look at which tests are failing. Once they're fixed, we can review your PR.

hmhealey commented 3 years ago

I've added the "Awaiting PR" label to this since it won't work without https://github.com/mattermost/mattermost-server/issues/16839 being implemented. Right now, the server can only send us the client config values as strings, but once that's fixed with a new API (or a tweak to the existing one), then this change can be done.

At that point, it'll also require accompanying webapp changes as well.

mattermod commented 3 years ago

This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

mattermod commented 3 years ago

This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

haardikdharma10 commented 3 years ago

@devinbinnie, gentle ping for review:)

hmhealey commented 3 years ago

@mkraft I just had a thought about this. We're changing boolean fields to be their correct type, but what about numeric fields? It's also possible that we have arrays and other non-string fields we might want to fix the type for as well when doing this. Since we're changing the format coming from the server, we might have to do these all at once.

mkraft commented 3 years ago

@mkraft I just had a thought about this. We're changing boolean fields to be their correct type, but what about numeric fields? It's also possible that we have arrays and other non-string fields we might want to fix the type for as well when doing this. Since we're changing the format coming from the server, we might have to do these all at once.

Yikes, I didn't realize we were doing numbers and arrays as string! Good callout. Would we be adding a bunch of overhead to do those as one separate PR? I just don't want to bait-and-switch the work for @haardikdharma10.

hmhealey commented 3 years ago

It all needs to be done at once since it requires a breaking API change on the server to return the correct types. These redux changes would have to use the new API, so it'd need to include all the new types.

hmhealey commented 3 years ago

@haardikdharma10 We're going to be shortly deprecating mattermost-redux as a standalone repository and moving it into mattermost-webapp as part of an effort to make development of both codebases easier by using a monorepo. This is going to require either merging or closing all outstanding pull requests in this repository in preparation for the migration planned for tomorrow.

For this PR, I know it's waiting on server changes in a separate ticket, and there's still some outstanding issues regarding changing other field types as well, so we'll have to close and reopen it later. I'd be glad to help getting this reopened in the webapp repo after our migration is done tomorrow though. Let me know what you think.

haardikdharma10 commented 3 years ago

Hi @hmhealey, sounds good to me. I'll close this for now.

hmhealey commented 3 years ago

Thanks! I'll let you know when it can be reopened against the webapp repo.

hmhealey commented 3 years ago

@haardikdharma10 Thanks for waiting on that. mattermost-redux now lives here, so future mattermost-redux changes can be submitted there.

For this PR, I still think we want to do this, but I think we need to figure out what we want to do with the client config as a whole since there's more fields that need their types changes as I mentioned before. I'd be glad to work with you on that since it's something that would be really beneficial, but I think the existing tickets need to be discussed and updated since they only cover one part of the structure.

haardikdharma10 commented 3 years ago

Hi @hmhealey, thanks for the info. I'd be happy to open up this PR again in the webapp repo in a while. I'd also be happy to discuss with you regarding other changes. Please let me know when is a good time to ping you on the community server :)

hmhealey commented 3 years ago

Hey. We discussed this a bit more internally, and there's some bigger issues we discovered since changing the type in the redux state will break all plugins relying on the config only containing strings. We still need to figure out how what parts of mattermost-redux we want accessible by plugins and how we can do that to best ensure backwards compatibility, so we're going to close these tickets for the time being while we sort that all out. We don't have a specific timeline for that yet, but that will likely involve breaking changes, so it would be a good time to break this for as well.

Still, we appreciate your work on this, and we can definitely let you know when we start trying to fix these types later if you're still interested in working on it then.