Patch fix#1227
Patch fix #1227
Uh oh!
There was an error while loading. Please reload this page.
Conversation
lib/resource-mapper.js Outdated
| path = path.substring(this._rootPath.length) | ||
| } | ||
| if (this._includeHost){ | ||
| console.log(path) |
lib/resource-mapper.js Outdated
| path = path.substring(this._rootPath.length) | ||
| } | ||
| if (this._includeHost){ | ||
| if (!path.startsWith(`/${hostname}`)){ |
There was a problem hiding this comment.
Needs to start with /${hostname}/ (otherwise, hostname prefixes would also be allowed).
There was a problem hiding this comment.
Might want to add a negative test for the slashless case.
lib/resource-mapper.js Outdated
| if (!path.startsWith(`/${hostname}`)){ | ||
| throw new Error(`Path must start with hostname (/${hostname})`) | ||
| } | ||
| path = path.substring(`/${hostname}`.length) |
There was a problem hiding this comment.
No need to create a new string just for length; let' take hostname.length + 1.
Uh oh!
There was an error while loading. Please reload this page.
| acl:trustedApp | ||
| [ acl:mode acl:Read, acl:Write; acl:origin <https://app.example.com> ]. | ||
| c0:me | ||
| c:me |
There was a problem hiding this comment.
Two different mes probably means trouble. Should be both the same, so both : / <#>.
Uh oh!
There was an error while loading. Please reload this page.
jaxoncreed commented Jun 13, 2019
@RubenVerborgh Everything look good? |
RubenVerborgh left a comment
RubenVerborgh left a comment There was a problem hiding this comment.
One concern about an RDF file remaining.
| acl:trustedApp | ||
| [ acl:mode acl:Read, acl:Write; acl:origin <https://app.example.com> ]. | ||
| c0:me | ||
| c:me |
There was a problem hiding this comment.
I still think these are wrong. Both should be :me. Somehow, the RDF serializer is getting the wrong URL.
Uh oh!
There was an error while loading. Please reload this page.
test/unit/resource-mapper-test.js Outdated
| path: `${rootPath}example.orgspace/foo.html`, | ||
| hostname: 'example.org' | ||
| }).then(() => done('Did not throw error')) | ||
| .catch(() => done()) |
There was a problem hiding this comment.
Nit: you can just write that as
it('throws an error when there is an improper file path',()=>{returnexpect(mapper.mapFileToUrl({path: `${rootPath}example.orgspace/foo.html`,hostname: 'example.org'})).to.be.rejectedWith(newError('error message'))})and simultaneously test the error message. (return necessary because it's a promise)
jaxoncreed commented Jun 14, 2019
@RubenVerborgh can you please approve. We need to get this in asap |
RubenVerborgh commented Jun 14, 2019
@jaxoncreed All good, excellent work on this! |
Fixes#1226