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

Path.normalize would break without the leading slash #61

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

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