Skip to content

Conversation

@senica
Copy link

@senica senica commented May 18, 2016

In utils, if the leading slash was gone from the path.normalize when rendering the template path, it would look in the wrong location for the template and break the sending of emails.

@senica
Copy link
Author

senica commented May 18, 2016

Note, I didn't write any tests for this as I am actually not sure how to alter the relative lookup path from testing inside a node module. Would love for someone to enlighten me. Also, the master branch has some failing tests, FYI.

@solarisn
Copy link

solarisn commented Aug 5, 2016

I can definitely corroborate that this is broken. Strangely enough, it worked fine when lifting locally but was unable to resolve the path when deployed to a server. I'm not sure what would cause that to happen...must have something to do with changes that occur from Sails' automatically generated "production" environment?

@pfructuoso
Copy link
Contributor

Subscribe.
@senica I tried to change tests be able to check this but it´s not as easy as I expected... waterlock-local-auth and waterlock don´t lift sails for testing. Instead of it, they use proxyquire to replace path.normalize(). It means path.normalize(__dirname+'../../../'+authConfig.passwordReset.template.file) is never executed. My idea to change this involves lots of changes in the code and I don´t have time. So I just create a small test to verify the missing space does create an error. You just have to add it to test/utils.test.jsand execute make test

  it('missing slash after __dirname fails', function(done) {
      utils = proxyquire('../lib/utils',
        {
          './waterlock-local-auth': waterlock,
          'path': {
            normalize: function(str) {
              return __dirname+"./email.test.jade";
            }
          }
        });

      var error = new Error('ENOENT: no such file or directory, open \''+__dirname+'./email.test.jade\'');
      error.errno = -2;
      error.code = 'ENOENT';
      error.syscall = 'open';
      error.path = __dirname+'./email.test.jade';

      (function(){ utils.getHtmlEmail({owner: "test", resetToken: "token"})}).should.throwError(error);
      done();
    });

@senica
Copy link
Author

senica commented Oct 11, 2016

Added your test. It does fail.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling b509aef on senica:jade-path-fix into * on waterlock:master*.

1 similar comment
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling b509aef on senica:jade-path-fix into * on waterlock:master*.

@pfructuoso
Copy link
Contributor

From my point of view this test shouldn´t be in the test suite. It was only for local checking :)

@senica
Copy link
Author

senica commented Oct 11, 2016

I put it there more for the project maintainer to check on their side as they haven't been quick in resolving this.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 37.313% when pulling 8a9ae1c on senica:jade-path-fix into 5ac2f15 on waterlock:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 37.313% when pulling 8a9ae1c on senica:jade-path-fix into 5ac2f15 on waterlock:master.

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