Issue #305: add check if slack panel is visible to prevent unnecessar… #332

annapieper · 2023-12-04T15:17:59Z

No description provided.

…ent unnecessary REST calls

mvlasovatl

@AnnaPiper Thank you for the submission!
Please see and address some comments.

mvlasovatl · 2023-12-07T16:34:38Z

bitbucket-slack-server-integration-plugin/pom.xml

@@ -42,7 +42,7 @@
        <atlassian-json-api.version>0.9</atlassian-json-api.version>
        <atlassian-concurrent.version>3.0.0</atlassian-concurrent.version>
        <commons-codec.version>1.11</commons-codec.version>
-        <slack.common.version>1.1.13</slack.common.version>
+        <slack.common.version>1.1.14</slack.common.version>


Versions of common modules aren't changed in this repo. Please roll back this change here and in other pom.xmls.

At the very beginning of work on plugin the team tried it, but then stopped. It appeared to be quite error prone and hard to manage that many different versions. Instead we don't publish common packages separately, only as part of a plugin. It makes code reuse between modules easier much. Once a change is done to a common module, just install it with the same version to the local repository and it's ready to be used in other modules.

mvlasovatl · 2023-12-07T16:38:27Z

jira-slack-server-integration/jira-slack-server-integration-plugin/pom.xml

    </parent>

    <groupId>com.atlassian.jira.plugins</groupId>
    <artifactId>jira-slack-server-integration-plugin</artifactId>
    <packaging>atlassian-plugin</packaging>
-    <version>3.0.10-SNAPSHOT</version>
+    <version>3.0.11-SNAPSHOT</version>


This version of the plugin module is updated only on release by Maven Release plugin. Please roll it back.

mvlasovatl · 2023-12-07T16:46:30Z

...-server-integration-plugin/src/main/resources/js/slack-feature/issuepanel/issuepanel-init.js

-            });
+        var $issuePanel = $("#slack-issue-panel");
+        if (!issueKey || !$issuePanel.length) {
+            return;


Rather than modifying the whole function to be a no-op, I'd recommend to prevent calling this function altogether. It would be less changes and the logic flow would be clearer.

I basically just added another condition to the already existing one that checks for the issueKey. Because getTemplate() is called 2 times it would be more code moving the condition out of the function. But if you want I can do the check before each function call.

Indeed. I agree. Then it may stay as it is.

mvlasovatl · 2023-12-07T16:53:03Z

slack-server-integration-common/src/main/resources/token/token-status.js

+        }
+        function isSlackPanelVisible() {
+            return $("#slack-issue-panel").length;
+        }


Is it possible to prevent duplication of these functions in multiple files?
Also, since it isn't Typescript, let's be more strict regarding types. The functions follow Java naming convention for boolean-returning function, so let's convert returned values to booleans to prevent ambiguities.

I also thought about that duplicated code but I didn't find out how to do it because the two JS files are located in separate apps. Do you have an idea?

Agree on the prevention of ambiguitites.

Oh, I see what you mean. This file is located in the common module, while other 2 ones are in Jira plugin module.
Ok, then some duplication is acceptable.

Could you also add a comment for these functions mentioning that they are expected to return true only when they are used in Jira plugin?
This file is also included into Confuence plugin descriptor and will be called there as well. The functions will return false there always, because there will be no elements with Jira-plugin-specific IDs on a Confluence page. It's better to keep this detail saved in a comment, then to learn it from debugging years later.

mvlasovatl · 2023-12-07T16:57:22Z

slack-server-integration-common/src/main/resources/token/token-status.js

@@ -11,9 +11,10 @@ require([
    var flag = null;

    $(function () {
-        // skip if user is already in configuration view or edit pages
+        // skip if user is already in configuration view or edit pages or in issue view without active slack integration


The wording is technically incorrect here. What your meant probably is "or issue panel isn't shown".

…ule; prevent ambiguities in JS

annapieper · 2023-12-11T09:48:47Z

@mvlasovatl I added the requested changes. If you still see problems with the versions let me know or maybe fix them by yourself as you know better where to put which version.

mvlasovatl

fix them by yourself

I suspect that's not how contributions work on Github :) Reviewers review, contributors contribute.
I added a couple more minor comments. Please take a look at them too.

mvlasovatl · 2023-12-14T17:29:38Z

confluence-slack-integration/confluence-7-compat/pom.xml

@@ -6,7 +6,7 @@
    <parent>
        <artifactId>confluence-slack-integration</artifactId>
        <groupId>com.atlassian.plugins</groupId>
-        <version>1.1.13</version>
+        <version>1.1.14</version>


I am sorry for the confusion. All changes in pom.xml files in all modules need to be reverted. I just got used to thinking about repository modules as about 2 big groups: plugin and common modules. That's why I left just 2 comments - one comment for each group of changes.

mvlasovatl · 2023-12-14T17:40:21Z

slack-server-integration-common/src/main/resources/token/token-status.js

+        }
+        function isSlackPanelVisible() {
+            return $("#slack-issue-panel").length;
+        }


Could you also add a comment for these functions mentioning that they are expected to return true only when they are used in Jira plugin?
This file is also included into Confuence plugin descriptor and will be called there as well. The functions will return false there always, because there will be no elements with Jira-plugin-specific IDs on a Confluence page. It's better to keep this detail saved in a comment, then to learn it from debugging years later.

mvlasovatl · 2023-12-19T23:51:25Z

Closer review revealed that only change in issuepanel-init.js may be applied without breaking other features. It properly prevents loading issue mentions data if issue panel is hidden.

update-slack-link-banner.js is responsible for showing a banner to an Admin when some workspace connection gets broken. This feature isn't related to issue panel in any way and shouldn't be disabled on all other pages. It may indeed generate some redundant traffic, but assuming that it affects only admins and that the request response has no body if all connections are good, it should be an acceptable price for proactive failure notification. So, changes in this file are to be reverted.

Code in token-status.js is responsible for warning a user when their authentication gets broken and if workspace connections get broken. It looks like it includes the logic from from update-slack-link-banner.js and adds even more information. I need to discuss this duplication with original developers. For now we may limit changes in this PR to issue mentions data loading prevention and maybe get back to duplicated endpoints later. Changes here are also to be reverted.

annapieper · 2024-01-22T10:19:02Z

Thanks for the information. Let me know when you discussed it and came to a decision. I reverted the changes as requested.

… they may have unindented impact on other functionality

Issue atlassian-labs#305: add check if slack panel is visible to prev…

511e769

…ent unnecessary REST calls

mvlasovatl requested review from mvlasovatl and yevhenhr December 7, 2023 13:56

mvlasovatl suggested changes Dec 7, 2023

View reviewed changes

Issue atlassian-labs#305: revert version changes for slack common mod…

fe8520d

…ule; prevent ambiguities in JS

mvlasovatl suggested changes Dec 14, 2023

View reviewed changes

Issue atlassian-labs#305: revert version changes; add comments

0140ddd

Issue atlassian-labs#305: revert changes for preventing REST calls as…

6183806

… they may have unindented impact on other functionality

yevhenhr deleted the branch atlassian-labs:release-1.x April 16, 2024 11:42

yevhenhr closed this Apr 16, 2024

yevhenhr reopened this Apr 16, 2024

yevhenhr changed the base branch from dev to release-1.x May 19, 2024 23:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #305: add check if slack panel is visible to prevent unnecessar… #332

Issue #305: add check if slack panel is visible to prevent unnecessar… #332

annapieper commented Dec 4, 2023

mvlasovatl left a comment

mvlasovatl Dec 7, 2023

mvlasovatl Dec 7, 2023

mvlasovatl Dec 7, 2023

annapieper Dec 8, 2023

mvlasovatl Dec 8, 2023

mvlasovatl Dec 7, 2023

annapieper Dec 8, 2023

mvlasovatl Dec 8, 2023

mvlasovatl Dec 14, 2023

mvlasovatl Dec 7, 2023

annapieper commented Dec 11, 2023

mvlasovatl left a comment

mvlasovatl Dec 14, 2023

mvlasovatl Dec 14, 2023

mvlasovatl commented Dec 19, 2023

annapieper commented Jan 22, 2024

Issue #305: add check if slack panel is visible to prevent unnecessar… #332

Are you sure you want to change the base?

Issue #305: add check if slack panel is visible to prevent unnecessar… #332

Conversation

annapieper commented Dec 4, 2023

mvlasovatl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annapieper commented Dec 11, 2023

mvlasovatl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvlasovatl commented Dec 19, 2023

annapieper commented Jan 22, 2024