Mattermost-server: Removal of unused status flags in Admin requests state object

Created on 1 Oct 2019  路  15Comments  路  Source: mattermost/mattermost-server

Remove unused status flags from https://github.com/mattermost/mattermost-redux/blob/master/src/reducers/requests/admin.js

We have a pattern to track API requests success, failure and progress states by making respective redux actions like


export function getRolesByNames(rolesNames: Array<string>) {
    return bindClientFunc({
        clientFunc: Client4.getRolesByNames,
        onRequest: RoleTypes.ROLES*BY_NAMES*REQUEST,
        onSuccess: <RoleTypes.RECEIVED*ROLES, RoleTypes.ROLES_BY_NAMES*SUCCESS],
        onFailure: RoleTypes.ROLES*BY_NAMES*FAILURE,
        params: [
            rolesNames,
        >,
    });
}

which modify the redux state of requests object


export default (combineReducers({
    getRolesByNames,
    getRoleByName,
    getRole,
    editRole,
}): (RolesRequestsStatuses, GenericAction) => RolesRequestsStatuses);

Most of these state objects are not needed because we don't consume them either in webapp or in mobile app repo.

The proposed solution is to remove any dispatches with success, failure or progress along with the reducer and initial state entity by checking their dependency in both webapp and mobile app repos.

If the redux state for the request is used only in one component we can use react state instead of redux so, we can remove the entity

PR's for reference:
Redux PR: https://github.com/mattermost/mattermost-redux/pull/748
Mobile PR: https://github.com/mattermost/mattermost-mobile/pull/2490
Web PR: https://github.com/mattermost/mattermost-mobile/pull/2496


If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.

New contributors please see our Developer's Guide.

JIRA: https://mattermost.atlassian.net/browse/MM-18904

AreTechnical Debt Easy Help Wanted TecRedux Up For Grabs

All 15 comments

Taking a stab at this.

@mbluemer Hey, How is this going? Is there anything i can help you with? Fell free to mention me if you need any help with the issue.

Hey @sudheerDev and @amyblais , sorry about the delay but it seems that life right now has gotten ahead of ambition to contribute, I haven't had the spare time to get to this. Opening it up for someone else to pick up.

I'll give it a whirl @hanzei

Awesome, thanks @Mycobee :+1:

Just a heads up...I am busy through the end of the week, and plan to have this PR open by the end of the weekend.

Thanks for your patience!

Thanks for the update @Mycobee

This is still in progress, I started moving forward with it today, and plan to work on it more tomorrow. Hopefully I will have a PR by the evening. I planned to have one earlier, but I ended up getting caught up with problems on another ticket I am working on.

So after grepping for 120 _REQUEST, _SUCCESS, and _FAILURE actions, I didn't have hits on any of them. Does that sound correct? I was ignoring node_modules/ and dist/ directories because I didn't think that should have any impact.

If this is correct, I may be able to pull out everything??

Below is my grep shell script.

declare -a ACTIONS=(
    "GET_LOGS_REQUEST"
    "GET_LOGS_SUCCESS"
    "GET_LOGS_FAILURE"
    "GET_AUDITS_REQUEST"
    "GET_AUDITS_SUCCESS"
    "GET_AUDITS_FAILURE"
    "GET_CONFIG_REQUEST"
    "GET_CONFIG_SUCCESS"
    "GET_CONFIG_FAILURE"
    "UPDATE_CONFIG_REQUEST"
    "UPDATE_CONFIG_SUCCESS"
    "UPDATE_CONFIG_FAILURE"
    "RELOAD_CONFIG_REQUEST"
    "RELOAD_CONFIG_SUCCESS"
    "RELOAD_CONFIG_FAILURE"
    "GET_ENVIRONMENT_CONFIG_REQUEST"
    "GET_ENVIRONMENT_CONFIG_SUCCESS"
    "GET_ENVIRONMENT_CONFIG_FAILURE"
    "TEST_EMAIL_REQUEST"
    "TEST_EMAIL_SUCCESS"
    "TEST_EMAIL_FAILURE"
    "TEST_SITE_URL_REQUEST"
    "TEST_SITE_URL_SUCCESS"
    "TEST_SITE_URL_FAILURE"
    "TEST_S3_REQUEST"
    "TEST_S3_SUCCESS"
    "TEST_S3_FAILURE"
    "INVALIDATE_CACHES_REQUEST"
    "INVALIDATE_CACHES_SUCCESS"
    "INVALIDATE_CACHES_FAILURE"
    "RECYCLE_DATABASE_REQUEST"
    "RECYCLE_DATABASE_SUCCESS"
    "RECYCLE_DATABASE_FAILURE"
    "CREATE_COMPLIANCE_REQUEST"
    "CREATE_COMPLIANCE_SUCCESS"
    "CREATE_COMPLIANCE_FAILURE"
    "GET_COMPLIANCE_REQUEST"
    "GET_COMPLIANCE_SUCCESS"
    "GET_COMPLIANCE_FAILURE"
    "UPLOAD_BRAND_IMAGE_REQUEST"
    "UPLOAD_BRAND_IMAGE_SUCCESS"
    "UPLOAD_BRAND_IMAGE_FAILURE"
    "DELETE_BRAND_IMAGE_REQUEST"
    "DELETE_BRAND_IMAGE_SUCCESS"
    "DELETE_BRAND_IMAGE_FAILURE"
    "GET_CLUSTER_STATUS_REQUEST"
    "GET_CLUSTER_STATUS_SUCCESS"
    "GET_CLUSTER_STATUS_FAILURE"
    "TEST_LDAP_REQUEST"
    "TEST_LDAP_SUCCESS"
    "TEST_LDAP_FAILURE"
    "SYNC_LDAP_REQUEST"
    "SYNC_LDAP_SUCCESS"
    "SYNC_LDAP_FAILURE"
    "GET_LDAP_GROUPS_REQUEST"
    "GET_LDAP_GROUPS_SUCCESS"
    "GET_LDAP_GROUPS_FAILURE"
    "LINK_LDAP_GROUP_REQUEST"
    "LINK_LDAP_GROUP_SUCCESS"
    "LINK_LDAP_GROUP_FAILURE"
    "UNLINK_LDAP_GROUP_REQUEST"
    "UNLINK_LDAP_GROUP_SUCCESS"
    "UNLINK_LDAP_GROUP_FAILURE"
    "SAML_CERT_STATUS_REQUEST"
    "SAML_CERT_STATUS_SUCCESS"
    "SAML_CERT_STATUS_FAILURE"
    "UPLOAD_SAML_PUBLIC_REQUEST"
    "UPLOAD_SAML_PUBLIC_SUCCESS"
    "UPLOAD_SAML_PUBLIC_FAILURE"
    "UPLOAD_SAML_PRIVATE_REQUEST"
    "UPLOAD_SAML_PRIVATE_SUCCESS"
    "UPLOAD_SAML_PRIVATE_FAILURE"
    "UPLOAD_SAML_IDP_REQUEST"
    "UPLOAD_SAML_IDP_SUCCESS"
    "UPLOAD_SAML_IDP_FAILURE"
    "DELETE_SAML_PUBLIC_REQUEST"
    "DELETE_SAML_PUBLIC_SUCCESS"
    "DELETE_SAML_PUBLIC_FAILURE"
    "DELETE_SAML_PRIVATE_REQUEST"
    "DELETE_SAML_PRIVATE_SUCCESS"
    "DELETE_SAML_PRIVATE_FAILURE"
    "DELETE_SAML_IDP_REQUEST"
    "DELETE_SAML_IDP_SUCCESS"
    "DELETE_SAML_IDP_FAILURE"
    "TEST_ELASTICSEARCH_REQUEST"
    "TEST_ELASTICSEARCH_SUCCESS"
    "TEST_ELASTICSEARCH_FAILURE"
    "PURGE_ELASTICSEARCH_INDEXES_REQUEST"
    "PURGE_ELASTICSEARCH_INDEXES_SUCCESS"
    "PURGE_ELASTICSEARCH_INDEXES_FAILURE"
    "UPLOAD_LICENSE_REQUEST"
    "UPLOAD_LICENSE_SUCCESS"
    "UPLOAD_LICENSE_FAILURE"
    "REMOVE_LICENSE_REQUEST"
    "REMOVE_LICENSE_SUCCESS"
    "REMOVE_LICENSE_FAILURE"
    "GET_ANALYTICS_REQUEST"
    "GET_ANALYTICS_SUCCESS"
    "GET_ANALYTICS_FAILURE"
    "UPLOAD_PLUGIN_REQUEST"
    "UPLOAD_PLUGIN_SUCCESS"
    "UPLOAD_PLUGIN_FAILURE"
    "INSTALL_PLUGIN_FROM_URL_REQUEST"
    "INSTALL_PLUGIN_FROM_URL_SUCCESS"
    "INSTALL_PLUGIN_FROM_URL_FAILURE"
    "GET_PLUGIN_REQUEST"
    "GET_PLUGIN_SUCCESS"
    "GET_PLUGIN_FAILURE"
    "GET_PLUGIN_STATUSES_REQUEST"
    "GET_PLUGIN_STATUSES_SUCCESS"
    "GET_PLUGIN_STATUSES_FAILURE"
    "REMOVE_PLUGIN_REQUEST"
    "REMOVE_PLUGIN_SUCCESS"
    "REMOVE_PLUGIN_FAILURE"
    "ENABLE_PLUGIN_REQUEST"
    "ENABLE_PLUGIN_SUCCESS"
    "ENABLE_PLUGIN_FAILURE"
    "DISABLE_PLUGIN_REQUEST"
    "DISABLE_PLUGIN_SUCCESS"
    "DISABLE_PLUGIN_FAILURE"
    )
for ACTION in ${ACTIONS[@]}
do
  echo $ACTION
  grep -rnw --exclude-dir={node_modules,dist} mattermost-webapp/ -e $ACTION
  grep -rnw --exclude-dir=node_modules mattermost-mobile/ -e $ACTION
done

Hi @hanzei , I don't mean to bother, but can you confirm that this looks correct? If so I am wondering if it is possible to remove the admin.ts file.

Thank you

@sudheerDev Can you help with this question?

@Mycobee Unfortunately that does not work all that well. I cross checked for all values to be sure and i think in this case we do use createCompliance in webapp at https://github.com/mattermost/mattermost-webapp/blob/dc33cec5eeb04cf721efd498a5a1ce998b3cc179/components/admin_console/compliance_reports/index.js#L40. For this entity we can get the error value in the component from the action https://github.com/mattermost/mattermost-webapp/blob/dc33cec5eeb04cf721efd498a5a1ce998b3cc179/components/admin_console/compliance_reports/compliance_reports.jsx#L90 eliminating the need to store error in redux state.

so after making the webapp change we can remove the reducer file and make other changes to actions to not dispatch the request flags.

let me know if you have any questions about this.

Hey @Mycobee, let us know if the above was helpful? If not, we'd be happy to clarify it :)

Hi,

Sorry for the delays...between work and life, I do not have the time to finish this pull request. Whoever picks it up, if they want to tag me, I have a lot of helpful PMs from @sudheerDev on the issue.

Hey @Mycobee totally understand, thanks for the heads up. Appreciate looking into it and hope to see you around in the community as time permits :)

Was this page helpful?
0 / 5 - 0 ratings