Skip to content
This repository has been archived by the owner on Sep 13, 2022. It is now read-only.

MM-18796 - Add POST /plugins/marketplace API to redux #938

Merged
merged 3 commits into from
Oct 9, 2019
Merged

MM-18796 - Add POST /plugins/marketplace API to redux #938

merged 3 commits into from
Oct 9, 2019

Conversation

alifarooq0
Copy link
Contributor

@alifarooq0 alifarooq0 commented Oct 4, 2019 •
edited
Loading

Summary

Add POST /plugins/marketplace API to redux

Ticket Link

Fixes: https://mattermost.atlassian.net/browse/MM-18796

@alifarooq0 alifarooq0 changed the title MM-18796 - Add POST API to redux MM-18796 - Add POST /plugins/marketplace API to redux Oct 4, 2019
@alifarooq0 alifarooq0 mentioned this pull request Oct 4, 2019
Merged
@alifarooq0 alifarooq0 added the 2: Dev Review Requires review by a core commiter label Oct 4, 2019
@alifarooq0 alifarooq0 added this to the v5.18.0 milestone Oct 4, 2019
marianunez
marianunez suggested changes Oct 4, 2019
View reviewed changes
Copy link
Member

@marianunez marianunez left a comment

Choose a reason for hiding this comment

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

Some comments below on the purpose of using the store and possibly simplifying most of this PR

src/client/client4.js
@@ -2661,6 +2661,15 @@ export default class Client4 {
);
}

installMarketplacePlugin = async (id, version) => {
this.trackEvent('api', 'api_install_marketplace_plugin');
Copy link
Member

Choose a reason for hiding this comment

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

I believe we are already tracking this on the webapp here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true. However if someone uses this directly by passing the webapp we'll be missing this event. But don't mind removing it :P

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. We should remove the one in the webapp then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marianunez
I'm personally OK keeping both, because once 5.16 is released, our analytics tool will contain this event. However >= 5.18 it'll be the other redux event, it might be hard to put together that data if you need to see how many people installed a marketplace plugin. My suggestion would to keep both now. If this change was going into 5.16 then it would've been fine removing that event.

Copy link
Member

Choose a reason for hiding this comment

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

There still needs to be a change to add the event to the whitelist here so that it can actually get logged - given that most events right now are bypassed to reduce usage in segment.

@aaronrothschild could you confirm if we should include this event to the whitelist to get tracked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah. forgot about that as well. Good catch! Maybe i should revert my other webapp change to remove the tracking there? or leave it the way it is right now?

Copy link
Member

Choose a reason for hiding this comment

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

0/5 either way. We just need to make sure that whichever we keep we add to the whitelist so that it actually gets to segment:

ui_marketplace_download and/or
api_install_marketplace_plugin

@aaronrothschild thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marianunez shall we unblock this PR and I can update the tracking events once I get the confirmation from Aaron.

I've created a jira to keep track and I can follow up with Aaron later, does that work?
https://mattermost.atlassian.net/browse/MM-19318

Copy link
Member

@marianunez marianunez Oct 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, we just would need to add the new event in the whitelist then because now with ui_marketplace_download removed from the webapp, we are not tracking either at this point🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah i didn't realize that ui event was whitelisted :(. I've reverted my change in webapp. mattermost/mattermost-webapp@0a5ec12

src/reducers/requests/plugins.js Outdated Show resolved Hide resolved
marianunez
marianunez suggested changes Oct 7, 2019
View reviewed changes
Copy link
Member

@marianunez marianunez left a comment

Choose a reason for hiding this comment

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

Changes needed if we want to actually track the event in segment

src/client/client4.js
@@ -2661,6 +2661,15 @@ export default class Client4 {
);
}

installMarketplacePlugin = async (id, version) => {
this.trackEvent('api', 'api_install_marketplace_plugin');
Copy link
Member

Choose a reason for hiding this comment

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

There still needs to be a change to add the event to the whitelist here so that it can actually get logged - given that most events right now are bypassed to reduce usage in segment.

@aaronrothschild could you confirm if we should include this event to the whitelist to get tracked?

@marianunez marianunez requested a review from prapti October 9, 2019 14:36
@alifarooq0
Copy link
Contributor Author

@prapti just a reminder that this will be tested as part of the final testing for the signed plugins epic

@alifarooq0
Copy link
Contributor Author

@iomodo * poke *

@alifarooq0
Copy link
Contributor Author

oo so fast :P

@alifarooq0 alifarooq0 added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Oct 9, 2019
@iomodo
Copy link
Contributor

iomodo commented Oct 9, 2019

too slow :D

prapti
prapti approved these changes Oct 9, 2019
View reviewed changes
Copy link

@prapti prapti left a comment

Choose a reason for hiding this comment

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

Approving to merge into plugin-signing feature branch. Will be tested once ready for master.

@alifarooq0 alifarooq0 merged commit 6977d2e into plugin_signing Oct 9, 2019
@alifarooq0 alifarooq0 deleted the MM-18796 branch October 9, 2019 15:33
alifarooq0 added a commit that referenced this pull request Nov 19, 2019
@alifarooq0
MM-16368 - Plugin Signing (#972)
aec5d7c 
* MM-18796 - Add POST `/plugins/marketplace` API to redux (#938)

* MM-18796 - Add POST '/plugins/marketplace'  API to redux

* Removing installMarketplacePlugin request reducer

* 🔨 linting

* MM-19318 - Update tracking events when marketplace plugin is ins… (#943)
lieut-data pushed a commit that referenced this pull request Nov 19, 2019
@alifarooq0 @lieut-data
MM-16368 - Plugin Signing (#972)
1976433 
* MM-18796 - Add POST `/plugins/marketplace` API to redux (#938)

* MM-18796 - Add POST '/plugins/marketplace'  API to redux

* Removing installMarketplacePlugin request reducer

* 🔨 linting

* MM-19318 - Update tracking events when marketplace plugin is ins… (#943)
@lindy65 lindy65 added the Tests/Not Needed Does not require tests label Nov 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@prapti prapti prapti approved these changes

@marianunez marianunez marianunez approved these changes

@iomodo iomodo iomodo approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request Tests/Not Needed Does not require tests
Projects
None yet
Milestone
v5.18.0
Development

Successfully merging this pull request may close these issues.
6 participants
@alifarooq0 @iomodo @prapti @marianunez @lindy65 @hanzei