Open Kshitij-Katiyar opened 2 years ago
@cwarnermm Can you please review the wording in the following markdown files of this PR:
assets/templates/command/install_cloud.md
assets/templates/command/install_server.md
Thank you!
@Kshitij-Katiyar Is it possible to split this PR up into separate features to make it more reasonable to review?
@mickmister , we met today to discuss breaking this up. It would be really difficult to break this up into a bunch of separate features. Would it be helpful if we could at least separate this into 2 PRs - 1. the auth/notifications and 2. the create page ?
Would it be helpful if we could at least separate this into 2 PRs - 1. the auth/notifications and 2. the create page ?
Any possible separation would be helpful :+1: Along with the large size of an aggregate PR like this, it's hard to know why a given line was changed/introduced when there is more than one purpose of a PR. It can be difficult to pinpoint the source of bugs during QA review as well. Thank you!
@mkdbns I meant to mention you above
@Kshitij-Katiyar Is it possible to split this PR up into separate features to make it more reasonable to review?
@mickmister We can create 2 Incremental PRs one for auth/notification and one for create Page Module. As they are incremental and create page module is dependent on auth/notification and i can only 1 create PR here with a base branch of mattermost:master and the other one have to be internal as auth/notification Branch is not present in mattermost confluence repo. So should i create only 1 PR and wait for it to merge or should i create 1 branch with mattermost master and other as a internal PR on brightscout domain but that will be on a public forked repo .
@Kshitij-Katiyar Let's have the auth/notification PR be merged, and then have the create page module PR submitted. We can use this open PR as the create page PR if the git history makes sense for that.
@Kshitij-Katiyar Let's have the auth/notification PR be merged, and then have the create page module PR submitted. We can use this open PR as the create page PR if the git history makes sense for that.
@mickmister Create a PR for auth/notifications .Please find the at following link :- https://github.com/mattermost/mattermost-plugin-confluence/pull/89
Not sure if we will review everything on #89, just in case let's check there also when there are new changes.
@javaguirre I will be fixing all the above review comments in the PR https://github.com/mattermost/mattermost-plugin-confluence/pull/89 as that is the latest PR containing features like config manipulation through slash commands and migrations of subscriptions. All the code present in this is present in PR 89 apart from 1 feature (Create confluence page using mattermost) This PR is here only for reference.
@javaguirre I will be fixing all the above review comments in the PR #89 as that is the latest PR containing features like config manipulation through slash commands and migrations of subscriptions. All the code present in this is present in PR 89 apart from 1 feature (Create confluence page using mattermost) This PR is here only for reference.
Thank you for pointing that out! 💯
Please, let me know when to review.
@Kshitij-Katiyar Heads up that there are merge conflicts to resolve
@Kshitij-Katiyar Heads up that there are merge conflicts to resolve
@hanzei Please read this comment https://github.com/mattermost/mattermost-plugin-confluence/pull/88#issuecomment-1192195527 ,https://github.com/mattermost/mattermost-plugin-confluence/pull/88#issuecomment-1192200570 and https://github.com/mattermost/mattermost-plugin-confluence/pull/88#issuecomment-1192399744. This PR is a Draft PR I am not able to convert it to Draft for some reason, that's why added WIP in the title. Please see https://github.com/mattermost/mattermost-plugin-confluence/pull/89
Summary and Features Added:-
1) Removed the dependency on the Confluence-side plugin (OBR file) to support subscriptions on Confluence Server/Data Center.
2) Added support for OAuth 2.0 for both Cloud and Server editions of Confluence . Users can OAuth using /Confluence connect command.
3) Remove the need to use a single shared authentication token limiting subscriptions to admins only.
4) In case of subscriptions ,users can only subscribe to spaces and pages that their Confluence account has access to.
5) Support of following events is there:- • space_updated • page_created • page_restored • page_trashed • page_updated • comment_created • comment_updated
6) Added permissions for both Mattermost and confluence side which can be changed by Mattermost Admin in Plugin configuration.
7) Create Page through Mattermost on Confluence both for server as well as cloud i) A Post Dropdown Menu Action to create a new confluence page ii) Opens a modal that with following options:- • Confluence URL • Space Key/Name • Page Title • Page body text which shall be pre-populated with the post content iii) Once the new page is created, It will also post a notification of the new page in the channel that from which the page
was created.