Refactored the code and added flow manager for Oauth setup

[Kshitij-Katiyar]

Summary

• Added flow manager for Oauth Setup
• Refactored the code and moved code needed for the flow manager to the main package
• Updated plugin configuration to store Oauth configurations
• Added functions to test Confluence base URL

Screenshots

For confluence server version < 9

For confluence server version > 9

[fmartingr]

Did a first pass. As a global question, is there a reason the controller package files where moved to the main server package?

[Kshitij-Katiyar]

Did a first pass. As a global question, is there a reason the controller package files where moved to the main server package?

@fmartingr Yes, the controller functions were to be used in the flow manager and related files but importing controller into the main was causing the import cycle

[davidkrauser]

Hi @Kshitij-Katiyar, I'm new jumping in here and trying to understand context 👋. The PRs from #125 - #129 have summaries that describe a bit of what changed, but I'd love a bit more detail if possible. Would you mind expanding the descriptions on all of the PRs in that range to include the rationale behind why each change is necessary, and a bit more context on why you chose the proposed solutions?

Apologies as I know that most of this has probably been discussed before elsewhere, but it would be very helpful for me when reviewing these changes to have the context plainly outlined here 🙂

A few examples on this PR:

• Why did the code need to be refactored? What was the goal of that refactoring?
• Why did you add an oauth manager? What are it's responsibilities?
• This adds new external dependencies - why are they needed?
• This adds telemetry, but that's not specified in the description. What is that for?

[Kshitij-Katiyar]

Hi @Kshitij-Katiyar, I'm new jumping in here and trying to understand context 👋. The PRs from #125 - #129 have summaries that describe a bit of what changed, but I'd love a bit more detail if possible. Would you mind expanding the descriptions on all of the PRs in that range to include the rationale behind why each change is necessary, and a bit more context on why you chose the proposed solutions?

Apologies as I know that most of this has probably been discussed before elsewhere, but it would be very helpful for me when reviewing these changes to have the context plainly outlined here 🙂

A few examples on this PR:

• Why did the code need to be refactored? What was the goal of that refactoring?
• Why did you add an oauth manager? What are it's responsibilities?
• This adds new external dependencies - why are they needed?
• This adds telemetry, but that's not specified in the description. What is that for?

Hey @davidkrauser, here is the initial conversation messages. https://hub.mattermost.com/partners/pl/8ezhsep5nbb1fd9w9ayim48bbo
https://hub.mattermost.com/partners/pl/pnqqofskn7bp9pkwpeiteo3mmy

To summarize, the existing flow of the Confluence plugin is:

• installing and configuring the Confluence plugin on your MM
• Downloading an OBR file from the plugin config
• Uploading the OBR file to your confluence admin settings.

No OAuth/Login was required to get the notifications for the Confluence event on your MM.

A user raised an issue that the Plugin stopped working for him after he Upgraded his Confluence version to 9 and above.
We tried fixing the issue for the OBR file setup for Confluence version 9 but could not do so.
So we came up with the solution of adding Oauth flow and basic Webhook setup.

Telemetry is added to most of the plugins maintained by Mattermost like Jira etc so added it here too.

Let me know if we need to update the PR description on each on of them

[davidkrauser]

@Kshitij-Katiyar wrote:

So we came up with the solution of adding Oauth flow and basic Webhook setup.

Thank you for the additional context and background on the issue. That's useful 👍

I would like to see this sort of thing added to your open PR descriptions. It helps reviewers jump in and understand what they're reviewing. With this information, they won't need to ask as many questions, and the information can be useful to look back on when we want to understand why a change was made.

Additionally, I would like to see details added to the description that explain why your solution is implemented in the way that it is. I understand what you're trying to achieve, but the rationale behind each specific code change in this PR is not readily apparent. An outline of that rationale would answer questions like I posted above (and provide context for questions I didn't even think to ask!):

Why did the code need to be refactored? What was the goal of that refactoring?
Why did you add an oauth manager? What are it's responsibilities?
This adds new external dependencies - why are they needed?
This adds telemetry, but that's not specified in the description. What is that for?

This would also help us avoid questions like these that Felipe asked above:

As a global question, is there a reason the controller package files where moved to the main server package? #125 (review)

Why was it added in this PR instead of working with telemetry in a separate PR? #125 (comment)

You are the expert on this code change, so details explaining why you made the specific code changes you made are enormously valuable. Not only that, it will enable us to give you better feedback on your changes. 🙂

[Kshitij-Katiyar]

@davidkrauser Sure, implemented your suggestions for PR descriptions.