mozilla / security-advisor-shield-study

Mozilla Public License 2.0
2 stars 7 forks source link

migrate to shield-studies-addon-utils version 2 #54

Closed casebenton closed 7 years ago

casebenton commented 7 years ago

@gregglind how does this usage of the new utils library look? r?

Osmose commented 7 years ago

@groovecoder has gracefully accepted my request to look over this PR :D

groovecoder commented 7 years ago

Are you still getting the require(...).study.startup is not a function error? I got that when I had an out-dated shield-studies-addon-utils module in my node_modules/ directory. I rm -rf node_modules && npm install'd and it fixed it.

On this latest commit/branch I am getting:

console.error: security-advisor:
JPM [error]   Message: TypeError: Advise is null
  Stack:
    isEligible@resource://gre/modules/commonjs/toolkit/loader.js -> resource://security-advisor/lib/study.js:35:12
casebenton commented 7 years ago

I'm no longer getting the require(...).study.startup is not a function error. That was a mistake on my part: I forgot to run npm install and actually include the new study utils after the rebase. I'll check out the Advise is null issue now.

casebenton commented 7 years ago

I'm also getting the Advise is null error, and I think I know why.

The Advise object is set to null at the time that the utils check for eligibility, so the call to Advise.isEligible() no longer works. Previously, the Advise object was initialized in the SecurityAdvisorStudy class constructor, but I moved the initialization of the Advise object into the code for each branch because I don't want the Advise object to be non-null on the observe-only branch. I'll remove the Advise.isEligible method from the Advisor class and move that code directly into the SecurityAdvisorStudy.isEligible() method. That should fix the issue.

casebenton commented 7 years ago

@groovecoder how does it look now?

I've moved the logic for determining eligibility out of Advisor.isEligible() and right into SecurityAdvisorStudy.isEligible(), so that eligibility can be determined when Advisor = null, as it is on the observe-only branch.

groovecoder commented 7 years ago

Looks good to me, and works well on all variations.