Allow installing older version of extension if latest not compatible #5653

peterflynn · 2013-10-23T17:58:37Z

Change ExtensionManager.getCompatibilityInfo() so it finds the newest compatible version, rather than only looking at the very newest version and giving up after that. Returned info now indicates which version was returned and whether it was the very newest or not.
ExtensionManagerView enables the install button if any available version is compatible, but shows warning message if it's not the very newest version.
Also fixes bug Extension Manager permits incompatible extension updates via Installed tab #5640 by cleaning up how the Update button enablement is determined. (This still follows the intent of master, where the button is disabled any time the very latest version isn't compatible. In the future, you could imagine following the new Install precedent more -- enabling the button with warning note if there's any compatible newer version to update to, even if we can't update to the very latest. But this doesn't do that yet.)

Now with unit tests too!

not compatible

… with Brackets (previously was erroneously left enabled in Installed tab). - Clarify in _renderItem() when button is disabled (making it more obvious that its tooltip string is always correct). - Make it more clear that flags like 'context.isCompatible' are specifically referring to the latest version available, not the currently installed version.

peterflynn · 2013-10-23T18:02:39Z

@njx / @dangoor, curious if one of you guys think we could land this before the sprint ends. Adam seemed in support of it last time I talked to him, and no one raised any concerns when I posted about this to the newsgroup.

Getting it in this sprint would be good because it would ship before the disruptive file system changes, helping smooth the transition there. And it would be guaranteed to make it into the next Edge Code release (whose users stand to gain the most from this due to its less-frequent release schedule).

I'll post screenshots shortly, since we'd have to finalize the strings today if this is going to have a chance of making it in.

njx · 2013-10-23T18:49:23Z

src/extensibility/ExtensionManagerView.js

+            context.requiresNewer = latestVerCompatInfo.requiresNewer;
+            context.isCompatibleLatest = latestVerCompatInfo.isLatestVersion;
+        } else {
+            context.isCompatible = context.isCompatibleLatest = true;


Seems like this should be = compatInfo.isCompatible rather than = true?

Also, just so I understand, is the only time we can get into this case when we haven't been able to download the registry for some reason? (I should know this :), but it's been awhile since I wrote this code and it might have changed when @dangoor added teh update stuff.)

There is no generic compatInfo anymore (see deleted lines above) -- that was part of #5640, the confusion over what compatInfo means when you're not looking at registry-specific info.

I believe we'd hit the !entry.registryInfo 'else' case here in several cases:

You have an installed extension that's not listed in the registry (because it's a dev extension, someone manually handed you a ZIP, etc.) -- which includes as a sub-case legacy or dev extensions that lack package.json altogether

You're offline, in which case all your installed extensions appear to fall into the previous category.

In all those cases you can only be looking at the "Installed" tab, and there can't be any updates available (context.updateAvailable is always false)... so I think all of the uses of these flags to set the other flags are moot. I think now that I've changed the Mustache template to check {{#showInstallButton}} before looking at isCompatible, these could just as well be left false in this case too. Which means I think I can take out the else block if you want.

Probably fine to leave it in, but add a comment clarifying things.

dangoor · 2013-10-23T18:56:31Z

Yeah, this change is not too big. I think we can do this.

njx · 2013-10-23T19:04:05Z

src/htmlContent/extension-manager-view-item.html

-                {{^requiresNewer}}{{Strings.EXTENSION_INCOMPATIBLE_OLDER}}{{/requiresNewer}}
-            </div>
-        {{/isCompatible}}
+        {{#showInstallButton}}


Do we also need to change the version shown in the dialog to be the latest compatible version? Right now it shows {{metadata.version}}, which (if I'm understanding things correctly) will always be the latest version, not the latest compatible version.

I think this is resolved by the latest string, right?

njx · 2013-10-23T19:08:44Z

I did a drive-by review, but I think @dangoor should do a thorough review as well. Also, I'd only feel comfortable with this if we had detailed unit tests covering the new cases as well as some thorough manual testing.

To be safe, maybe you should submit a separate PR with just the strings, and land that ASAP, and aim to get this landed tomorrow after we have unit tests, etc.

dangoor · 2013-10-23T19:22:45Z

@njx Peter and I just briefly chatted about this on IRC and our conclusion was the same: land the strings ASAP and then finish with a thorough review/tests.

peterflynn · 2013-10-23T21:12:31Z

@dangoor @njx Posted PR #5659 with just the strings (and a screenshot)

peterflynn · 2013-10-23T23:41:26Z

Will update with a slew of unit tests tonight. I tested tons of cases out locally (with a fake registry.json) already, so it should be fairly quick to convert that into testcases.

Strings for PR #5653 (Allow installing older version of extension if latest not compatible)

…t-compatible-ext Conflicts: src/nls/root/strings.js

- Add more unit tests for Extension Manager install/update button state & ExtensionManager.getCompatibilityInfo() utility - Fix crash when calling getCompatibilityInfo() directly on an installInfo (instead of registryInfo) - though I think this now only happens in unit tests - Ensure getCompatibilityInfo() never returns isLatestVersion unless isCompatabile is true

peterflynn · 2013-10-24T07:45:01Z

@dangoor @njx Updated with a bunch of unit tests, and two small fixes to fix unit-test-specific (I think) edge cases. Also updated to do the proper substitutions for the new string we decided upon.

peterflynn · 2013-10-24T07:46:17Z

test/spec/ExtensionManager-test.js


+                it("should show disabled update button for items whose available update requires newer API version", function () {   // isLatestVersion: false, requiresNewer: true


This test and the next one would fail on master due to #5640 -- the Update button is incorrectly enabled there

dangoor · 2013-10-24T19:33:19Z

test/spec/ExtensionManager-test.js

@@ -325,6 +364,40 @@ define(function (require, exports, module) {
                    .toEqual({isCompatible: false, requiresNewer: true});
            });

+            it("should calculate compatibility info for registry extensions", function () {
+                function fakeEntry(versionRequirements) {


Not a big deal, but shouldn't this just be:

var fakeEntry = makeMockExtension;

dangoor · 2013-10-24T20:04:13Z

The changes look good. Very nice collection of tests there (gives me a fair bit of confidence without even having tested it yet).

I'll fire up the registry on my machine later on and set up some test scenarios to see how it all feels. Assuming I don't notice or think of something while testing, I think this is ready to merge.

peterflynn · 2013-10-24T20:44:39Z

Sounds great, thanks Kevin! Let me know if you want me to fix that one nit with fakeEntry() before you merge (holding off for now in case of other changes).

dangoor · 2013-10-25T02:57:12Z

Looks good in testing. Is there already an issue already filed for adding the warning message to updates? That was the only behavior that seemed less than ideal (hmm... disabled update button. I wonder why?)

It's great to see this implemented, because there's a reason the registry was built to be able to serve multiple versions of extensions 😄

Allow installing older version of extension if latest not compatible

dangoor · 2013-10-25T03:04:52Z

I took the liberty of adjusting the fakeEntry() definition myself, since it was such a trivial thing.

peterflynn added 2 commits October 23, 2013 00:25

Allow installing older version of an extension if the latest version is

8dbccb2

not compatible

njx reviewed Oct 23, 2013
View reviewed changes

peterflynn added a commit that referenced this pull request Oct 23, 2013

Strings for PR #5653 (branch pflynn/latest-compatible-ext)

eaf94f2

peterflynn mentioned this pull request Oct 23, 2013

Strings for PR #5653 (Allow installing older version of extension if latest not compatible) #5659

Merged

njx pushed a commit that referenced this pull request Oct 23, 2013

Merge pull request #5659 from adobe/pflynn/strings-latest-compatible-ext

afcb64f

Strings for PR #5653 (Allow installing older version of extension if latest not compatible)

peterflynn added 2 commits October 24, 2013 00:19

Merge branch 'pflynn/strings-latest-compatible-ext' into pflynn/lates…

c02d051

…t-compatible-ext Conflicts: src/nls/root/strings.js

peterflynn reviewed Oct 24, 2013
View reviewed changes

ghost assigned dangoor Oct 24, 2013

dangoor reviewed Oct 24, 2013
View reviewed changes

dangoor added a commit that referenced this pull request Oct 25, 2013

Merge pull request #5653 from adobe/pflynn/latest-compatible-ext

33a9f05

Allow installing older version of extension if latest not compatible

dangoor merged commit 33a9f05 into master Oct 25, 2013

dangoor deleted the pflynn/latest-compatible-ext branch October 25, 2013 02:57

core-ai-bot mentioned this pull request Aug 30, 2021

[CLOSED] Allow installing older version of extension if latest not compatible brackets-archive/bracketsIssues#5184

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow installing older version of extension if latest not compatible #5653

Allow installing older version of extension if latest not compatible #5653

peterflynn commented Oct 23, 2013

peterflynn commented Oct 23, 2013

njx Oct 23, 2013

njx Oct 23, 2013

peterflynn Oct 23, 2013

njx Oct 23, 2013

dangoor commented Oct 23, 2013

njx Oct 23, 2013

peterflynn Oct 23, 2013

njx Oct 23, 2013

njx commented Oct 23, 2013

dangoor commented Oct 23, 2013

peterflynn commented Oct 23, 2013

peterflynn commented Oct 23, 2013

peterflynn commented Oct 24, 2013

peterflynn Oct 24, 2013

dangoor Oct 24, 2013

dangoor commented Oct 24, 2013

peterflynn commented Oct 24, 2013

dangoor commented Oct 25, 2013

dangoor commented Oct 25, 2013


		it("should show disabled update button for items whose available update requires newer API version", function () { // isLatestVersion: false, requiresNewer: true

Allow installing older version of extension if latest not compatible #5653

Allow installing older version of extension if latest not compatible #5653

Conversation

peterflynn commented Oct 23, 2013

peterflynn commented Oct 23, 2013

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

dangoor commented Oct 23, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njx commented Oct 23, 2013

dangoor commented Oct 23, 2013

peterflynn commented Oct 23, 2013

peterflynn commented Oct 23, 2013

peterflynn commented Oct 24, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dangoor commented Oct 24, 2013

peterflynn commented Oct 24, 2013

dangoor commented Oct 25, 2013

dangoor commented Oct 25, 2013