Update index.html based on whether meta-tag is set or not#953
Conversation
ghost
assigned
kjetilk
left a comment
kjetilk
left a comment
There was a problem hiding this comment.
LGTM, but I think I'll let others comment.
|
@RubenVerborgh and @dmitrizagidulin might be busy, so I hope that @rubensworks might be able to take a look. |
|
And how about you, @mielvds , you did a bit of looking on the frontend? |
rubensworks
left a comment
rubensworks
left a comment
There was a problem hiding this comment.
Looks good! Just a few minor comments.
|
|
||
| // Use flags with priority over config file | ||
| const config = JSON.parse(file) | ||
| Object.keys(config).forEach((option) => { |
There was a problem hiding this comment.
For consistency, like on line 31, this block may also be possible using an extend .
| } | ||
|
|
||
| readContainerMeta (directory, callback) { | ||
| async readContainerMeta (directory, callback = function () {}) { |
There was a problem hiding this comment.
Having both promise-based and callback-based resolving might be confusing. I propose to remove the callback parameter.
| let configFile = argv['configFile'] || './config.json' | ||
|
|
||
| try { | ||
| const file = fs.readFileSync(configFile) |
There was a problem hiding this comment.
Hm. Given that argv['configFile'] is user input... Could this be leveraged to a directory traversal attack?
I imagine accessing files from another user on the same pod.
So some sanitation would need to happen...
There was a problem hiding this comment.
The user that provides this input is the server admin, so they can do way worse 😉
| const isUserDirectory = new RegExp(`.${hostname}$`) | ||
| return files | ||
| .filter(file => isUserDirectory.test(file)) | ||
| .map(userDirectory => userDirectory.substr(0, userDirectory.length - hostname.length - 1)) |
There was a problem hiding this comment.
Since the right hand side comes up again and again, maybe it's time for aa helper function to make it more speaking.
There was a problem hiding this comment.
Where is this coming up again and again?
| const indexTemplate = await compileTemplate(indexTemplatePath) | ||
| const ldp = new LDP(config) | ||
| const accountManager = getAccountManager(config, { ldp }) | ||
| const usernames = loadUsernames(config) |
There was a problem hiding this comment.
So many local variables. Personally I'd have split up the function into smaller ones to ease understanding.
There was a problem hiding this comment.
I would've done that myself, but the server is sometimes difficult to write for, because there are so many things that need to be set up. The CLI scripts are kind of extreme in this sense =/
If you have concrete ways of rewriting this to make it easier to read, I'm all ears, but until then I'll leave it as it is.
|
Hmm, something strange happened when I tried to rebase this branch to master. There are a lot of commits in this that I'm unsure why is there =/ That being said, I've done a couple of changes based on the feedback here, and think the branch should be good for merge. Maybe @rubensworks wants to verify? |
|
It looks like something indeed went wrong during the rebase, the last 3 commits on master seem duplicated. I'm wondering if this should be targeted to the release branch? If so, it should be rebased on that. (In that case it might be easier to just fork a clean branch and cherry-pick your commits on there. This would fix the additional commits being here) |
Trying to reuse methods from invalidUsernames command
Will improve on it to make it easier to read and some functionality more reusable
And by that hopefully more maintainable
And by that hopefully more maintainable
And by that hopefully more maintainable
And by that hopefully more maintainable
megoth
force-pushed
the
feature/index-update
branch
from
9b2a592 to
bf9a59e
Compare
|
Then I've finally managed to rebase it to v5.0.0 (here's hoping Travis run all tests). Will continue with #937 when this is merged into the release-branch |
| const AccountManager = require('../../lib/models/account-manager') | ||
| const SolidHost = require('../../lib/models/solid-host') | ||
|
|
||
| module.exports.getAccountManager = getAccountManager |
There was a problem hiding this comment.
This could be rephrased as:
module.exports = {
getAccountManager,
loadAccounts,
loadConfig,
loadUsernames
}
|
👍 , looks good! |
9 participants
This is a fix for #688 that uses the solution in #936.
Parsing HTML is non-trivial, but I hope cheerio will do the work (I initially tried using htmlparser2, but I found the abstraction layer of cheerio much easier to handle).
Added some tests for getWebId and getName.