Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[MM-64] Update plugin with respect to phase 1 upgrades #190

Merged
merged 17 commits into from
Nov 29, 2024
Merged

[MM-64] Update plugin with respect to phase 1 upgrades #190

merged 17 commits into from
Nov 29, 2024

Conversation

ayusht2810
Copy link
Contributor

Summary

Updated plugin with respect to phase 1 upgrades

Ticket Link

@ayusht2810 ayusht2810 self-assigned this Jan 2, 2024
@ayusht2810 ayusht2810 added the 2: Dev Review Requires review by a core committer label Jan 2, 2024
mickmister
mickmister reviewed Feb 15, 2024
View reviewed changes
webapp/src/types/mattermost-webapp/index.d.ts Show resolved Hide resolved
webapp/src/websocket/index.ts Outdated Show resolved Hide resolved
webapp/src/websocket/index.ts Outdated Show resolved Hide resolved
lieut-data and others added 2 commits February 16, 2024 18:06
@lieut-data @mickmister @ayusht2810
Sync with playbooks: install-go-tools, gotestsum, and dynamic versions (
93c0b37 
#192)

* Revert "Update main.go (#154)"

This reverts commit be4a281.

* Revert "[MM-33506] Use embed package to include plugin manifest (#145)"

This reverts commit ca9ee3c.

* Revert "Don't generate manifest.ts (#127)"

This reverts commit 18d30b5.

* install-go-tools target, adopt gotestsum

* bring back make apply + automatic versioning

* Update build/manifest/main.go

Co-authored-by: Michael Kochell <[email protected]>

* suppress git describe error when no tags match

* make version/release notes opt-in

* fix whitespace in Makefile

* document version management options

---------

Co-authored-by: Michael Kochell <[email protected]>
@hanzei @lieut-data @raghavaggarwal2308
Fetch plugin logs from server (#193)
83f03ba 
Co-authored-by: Jesse Hallam <[email protected]>
@ayusht2810
Copy link
Contributor Author

@mickmister Fixed the review comments. Please re-review.

@ayusht2810 ayusht2810 requested a review from mickmister June 20, 2024 08:06
mickmister
mickmister reviewed Jun 24, 2024
View reviewed changes
webapp/src/websocket/index.ts
Comment on lines +14 to 21
const globalState = (store.getState() as GlobalState);
if (globalState.views?.rhs?.rhsState === 'plugin') {
if (globalState.views.rhs.pluggableId === rhsComponentId) {
store.dispatch(refetch() as Action);
}
} else {
store.dispatch(refetch() as Action);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make it so this else essentially "never" happens. Meaning it should only happen if the redux store has changed its structure from what we currently assume. The first if should just be "does the redux structure match our expectations", and not make any assertions about the state of the store, as described in this comment #190 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mickmister Not sure whether I got your point or not. If the condition if (globalState.views?.rhs?.rhsState === 'plugin') is false, how will we refetch if we remove the else condition?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ayusht2810 As far as I understand, we only want to refetch if the plugin's RHS is currently showing. The else was added here in the case of "the redux store's structure itself changes in future iterations of the webapp codebase". That's a corner case that will essentially "never" be used, because the redux store structure will likely not change.

I'm not saying we should remove the else condition. I'm saying that the branch that runs that will/should not happen unless the redux store structure changes.

This is how I think it should be implemented (verbosely):

const doesReduxStoreStructureMatchOurAssumptions = Boolean(globalState.views?.rhs && 'rhsState' in globalState.views?.rhs);
if (doesReduxStoreStructureMatchOurAssumptions) {
    if (globalState.views?.rhs?.rhsState === 'plugin' && globalState.views?.rhs?.pluggableId === rhsComponentId) {
        store.dispatch(refetch() as Action);
    }
} else {
    store.dispatch(refetch() as Action);
}

Personally I think we can get rid of the else altogether, but the undocumented redux store structure could in theory change in the future, so it's not a bad idea to have code that defends against it. I'm 0/5 on if we need it, but we should only refetch the data when necessary given what the code has to work with. @ayusht2810 Does this all make sense?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mickmister Just for clarification:

  • The if block present here for handling the older redux structure (i.e. for order MM versions), where we had access to globalState.views.
  • The else block is for handling the current redux structure with no access to globalState.views(i.e. latest MM version). So, if we remove the else block the RHS will not update until the user re-open it.

So, I do't think we should remove the else block as it will break the functionality on latest servers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc: @wiggin77

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The else block is for handling the current redux structure with no access to globalState.views(i.e. latest MM version). So, if we remove the else block the RHS will not update until the user re-open it.

@raghavaggarwal2308 Are we sure about this? I thought it was just that the exported GlobalStore type doesn't expose it. But I could have that incorrect

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mickmister Yes we are sure, the field does not exist in redux in the latest version.

@wiggin77 wiggin77 requested a review from hmhealey July 26, 2024 22:04
@wiggin77
Copy link
Member

@ayusht2810 has QA done any testing on this PR? It is quite large to merge without some testing.

@raghavaggarwal2308
Copy link
Contributor

@wiggin77 We generally assign a PR for QA once the PR is dev approved. If the changes looks good to you we can assign it to @AayushChaudhary0001

@hmhealey hmhealey removed their request for review July 31, 2024 20:49
@hmhealey
Copy link
Member

Dismissing my request for review since I think I was just added to this PR to answer a question, and there's enough other devs assigned at the moment. I've been a bit swamped with big PRs to review recently, but feel free to re-add me if needed

@raghavaggarwal2308 raghavaggarwal2308 added the 3: QA Review Requires review by a QA tester label Sep 12, 2024
AayushChaudhary0001
AayushChaudhary0001 approved these changes Oct 7, 2024
View reviewed changes
Copy link

@AayushChaudhary0001 AayushChaudhary0001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested this PR and all the basic functionalities are working fine, LGTM. Approved.

@raghavaggarwal2308
Copy link
Contributor

raghavaggarwal2308 commented Oct 8, 2024 •
edited
Loading

@wiggin77 Can you give this PR a review? It is requred for the secret config PR #201

@mattermost-build
Copy link

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!

@raghavaggarwal2308 raghavaggarwal2308 added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Nov 6, 2024
@raghavaggarwal2308
Copy link
Contributor

raghavaggarwal2308 commented Nov 6, 2024 •
edited
Loading

@wiggin77 The CI is not running on this PR. I think this might be due to an outdated branch protection rule. Can you please check the same?

@mattermost-build
Copy link

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!

@wiggin77 wiggin77 closed this Nov 28, 2024
@wiggin77 wiggin77 reopened this Nov 28, 2024
@wiggin77
Copy link
Member

/update-branch

@mattermost-build
Copy link

Error trying to update the PR.
Please do it manually.

@raghavaggarwal2308 raghavaggarwal2308 merged commit a1f3aec into master Nov 29, 2024
4 checks passed
@raghavaggarwal2308 raghavaggarwal2308 deleted the MM-64 branch November 29, 2024 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kshitij-Katiyar Kshitij-Katiyar Kshitij-Katiyar left review comments

@mickmister mickmister mickmister left review comments

@hmhealey hmhealey hmhealey left review comments

@AayushChaudhary0001 AayushChaudhary0001 AayushChaudhary0001 approved these changes

@raghavaggarwal2308 raghavaggarwal2308 raghavaggarwal2308 approved these changes

@wiggin77 wiggin77 wiggin77 approved these changes

Assignees

@ayusht2810 ayusht2810

Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.