Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GPII-3921: Update gpii-express-user to use modern dependencies, especially gpii-json-schema. #6

Merged
merged 25 commits into from
Jul 10, 2020

Conversation

the-t-in-rtf
Copy link
Collaborator

See GPII-3921 for details.

@amb26, this should be reviewed after the related gpii-json-schema pull that fixed a blocking bug.

@the-t-in-rtf the-t-in-rtf requested a review from amb26 June 28, 2019 07:58
@the-t-in-rtf
Copy link
Collaborator Author

@sgithens, here's the pull to update gpii-express-user to support your work. I left the version of kettle alone, it still matches universal master. As I mentioned on IRC, there's a dev release for you to use:

1.0.2-dev.20190628T073950Z.6efdc50.GPII-3921

@the-t-in-rtf
Copy link
Collaborator Author

@amb26, I tried using the resource loader support for promises to bring in schema holder content. If I launch tests/js/launch-test-harness.js with debugging enabled, I get an error like the following:

14:55:58.229:  FATAL ERROR: Uncaught exception: that.generateSchema is not a function
fluid.js:256TypeError: that.generateSchema is not a function
    at gpii.schema.schemaHolder.generateIfNeeded (/Users/duhrer/Source/rtf/gpii-express-user/node_modules/gpii-json-schema/src/js/common/schemaHolder.js:22:84)

If I set a breakpoint at that line in schemaHolder.js, I can see that one of the invokers defined in that grade exists as part of that, but not the required generateSchema invoker. Please assist.

@the-t-in-rtf
Copy link
Collaborator Author

the-t-in-rtf commented Oct 3, 2019

Turns out that generateSchema will exist if there's an IoC reference to it, and it also returns the schema. That appears to have got me past the error, I will test it tomorrow.

@sgithens
Copy link
Contributor

sgithens commented Apr 1, 2020

@the-t-in-rtf Now that the resulting gpii-json-schema pull has been merged in, do you think we can look at merging this work back in to master?

@the-t-in-rtf
Copy link
Collaborator Author

@sgithens, thanks for reminding me here and in chat. I had actually completely forgotten that I'd already made a pass at this. It's been so long that I hadn't even checked out the repo on my new machine yet, and I've had that for months.

Anyway, @amb26, this is ready for review, and now more urgently needed. The changes are adjustments to use the new resource loader aware stuff from gpii-handlebars and gpii-json-schema, it would be good to get another pair of eyes on at least that part.

@sgithens, I did cut a dev release at the time if that helps at all, it's:

1.0.2-dev.20191014T143404Z.150969f.GPII-3921

@sgithens
Copy link
Contributor

@the-t-in-rtf Awesome thanks. I might try the dev release, although it does actually work as is (but I'm just using the password unlocking, may need more of the repo soon).
The more urgent reason for getting this through is that I just want to start upgrading js libraries with security issues, which seems important since this is an account utility.


/**
*
* Evaluate a single entry and retrieve the associated global variable if possible.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be a bit clearer about the workflow here, and what object is returned, since the logic is a little complex. If the argument is a string, it is looked up, and any value other than undefined is returned, else {} .... otherwise the input argument is returned. Does this really need to be this complex? I don't see that it is used elsewhere in the pull.
In my experience, functions like this which include lots of special cases in order to be "helpful" end up causing problems down the line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was left over from a previous approach, I have deleted it.

* Evaluate a single entry and retrieve the associated global variable if possible.
*
* @param {String|Object} toEvaluate - A single message bundle represented as a raw object or as a global variable name.
* @return {Object} - A message bundle object.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that it might not be of type {Object} if some value other than undefined or an Object is stored in the global name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's gone.

package.json Outdated
"gpii-grunt-lint-all": "1.0.5",
"gpii-mail-test": "1.0.5",
"gpii-webdriver": "1.1.0",
"grunt": "1.0.4",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few of these have become further outdated since this pull was issued, e.g. grunt -> 1.1.0, eslint -> 6.8.0 (doesn't lint-all pull in its own deps anyway?)
Also please could you try updating to kettle 1.12.0 and infusion 3.0.0-dev.20200317T122331Z.1aa7ea8a1.FLUID-6145 - thanks

@amb26
Copy link
Member

amb26 commented Apr 23, 2020

Due to some bug in Chrome and/or chromedriver 81, I am no longer able to run the webdriver tests - https://stackoverflow.com/questions/61321143/12296266720420-163936-459errorbrowser-switcher-service-cc238-xxx-init-er
I get this error, and Chrome merely starts up with the url "data:;" and then the tests hang

@gpii-bot
Copy link

@the-t-in-rtf
Copy link
Collaborator Author

I initially got an error related to a chrome/chromedriver mismatch, just fixed that. I still see the same issue when running in Vagrant locally, although interestingly the gpii-webdriver tests pass with the same VM image and chrome/chromedriver version.

@gpii-bot
Copy link

@the-t-in-rtf
Copy link
Collaborator Author

Now it's broken in the same way as everything else. My next ideas are:

  1. Looking for errors that are preventing the tests from launching Chrome properly.
  2. Rewriting these tests to use Testem (couple of hours, probably).

@amb26
Copy link
Member

amb26 commented May 12, 2020

Cheers @the-t-in-rtf - I've rolled infusion 3.0.0-dev.20200512T100752Z.0d704c4.FLUID-6148 from the current head of that branch. Interesting that my choice to keep FLUID-6148 and FLUID-6145 development separated seems somewhat vindicated which otherwise might have been a pointless piece of bureaucracy ....

@amb26
Copy link
Member

amb26 commented May 14, 2020

Ok - there is now 3.0.0-dev.20200514T203129Z.505489c.FLUID-6145 which fixes this issue, you should be able to update to it

@gpii-bot
Copy link

@the-t-in-rtf
Copy link
Collaborator Author

Provisioning error in the VM creation stage. Ok to test again.

@gpii-bot
Copy link

@the-t-in-rtf
Copy link
Collaborator Author

@amb26, just refreshing this with the newer release. It still has the circular dependency error when run locally in a VM.

@the-t-in-rtf
Copy link
Collaborator Author

So, I updated all dependencies just to make sure I hadn't had to make a code change in either gpii-handlebars or gpii-json-schema. No change. What's odd is that if I sidestep the issues in the schema holder grades, I run into the same issue with options inheritance in the test grades. I committed my work in progress so you can see the new variation.

@gpii-bot
Copy link

@gpii-bot
Copy link

@amb26
Copy link
Member

amb26 commented May 28, 2020

Cheers - I have rolled a fresh 3.0.0-dev.20200528T154535Z.9cd9261.FLUID-6145 which contains even an even more egregious version of the hack to avoid tripping this as a circular dependency - all of this code so badly needs rewriting.
The next issue you will run into is that gpii-mail-test will require a fluid.copy on this line https://github.com/GPII/gpii-mail-test/blob/master/src/js/simpleSmtpServer.js#L30 in order to make it FLUID-6145 compliant. And then after that you will run into an issue where the test fixtures mysteriously hang just after the server starts up - but I will have to leave you in the hands of that Giant Rat of Sumatra. Unfortunately with all of the recent updates to the FLUID-6145 branch I think we will have to say that the FLUID-6148 branch is now extinct and we will have to tough it out by updating all of our ecosystem all the way if we can.

@the-t-in-rtf
Copy link
Collaborator Author

The latest failure looks like another options-munging issue where I used the old pattern of stuffing material to be inspected later on into the request:

https://github.com/the-t-in-rtf/gpii-express-user/blob/403e8ae1fe5d466c701bc7258d3fdd3466225324/tests/js/server/forgot-tests.js#L223

I'll add a member variable or other safe stash for that, and then fingers crossed.

@the-t-in-rtf
Copy link
Collaborator Author

Ugh, it was nothing nearly as innocent, it was straight-up options munging to avoid writing a proper termMap and passing direct options to incorporate into the path. Fixing it properly now.

@the-t-in-rtf
Copy link
Collaborator Author

When we switched to a FLUID-6145 variant I had to update the tests to avoid options munging. They now pass locally, fingers crossed to round this out shortly.

@gpii-bot
Copy link

@the-t-in-rtf
Copy link
Collaborator Author

The second build is just with the full release of gpii-mail-test, I saw once that passes we're good here.

@gpii-bot
Copy link

properties: {
username: {
anyOf: [
// TODO: Revert
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this TODO can be done now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this, fixing it now.

title: "gpii-express-user user resend verification code schema",
description: "This schema defines the format accepted when requesting that a verification code be resent.",
properties: {
// TODO: Revert
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this one

@the-t-in-rtf
Copy link
Collaborator Author

Last build seemed to hang and was killed off. Tests still pass locally, the TODO changes will kick of another build, let's see how it goes.

@gpii-bot
Copy link

@the-t-in-rtf
Copy link
Collaborator Author

Ready to merge this when you are, @amb26.

@the-t-in-rtf the-t-in-rtf merged commit 364d0cc into fluid-project:master Jul 10, 2020
@the-t-in-rtf
Copy link
Collaborator Author

Self-merged based on conversations with reviewer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants