feat: add more metadata in POST to /api/v1/books

[wrobbins]

• Added more optional fields that nodepub supports to the create book endpoint:

  • coverPath image used for the book (can only be http or https)
  • publisher default EpubPress
  • genre default Unknown
  • series default empty
  • sequence default empty
  • tags default empty

• add .nvmrc for node v13.14.0; although github actions say node 14, they are using 13. I had problems running tests when using 14.

Now you can do this:

curl --X POST 'http://localhost:3000/api/v1/books' \
--header 'Accept: application/json' \
--header 'Content-Type: application/json' \
-D '{
    "title": "This is the title",
    "description": "This is the description",
    "author": "This is the Author",
    "publisher": "the-publisher",
    "genre": "the-genre",
    "series": "Tests",
    "sequence": 1,
    "coverPath": "https://via.placeholder.com/816x1056.jpg?text=CoverPage",
    "tags": "Tag1,Tag2,Tag3",
    "urls": [
        "https://epub.press/"
    ]
}'

[haroldtreen]

Hey @wrobbins - thanks for the contribution! This is a neat idea.

Some questions:

1. Does you Cover Page example work? I think EpubPress just passes the cover to another library, but my assumption was it needed to be a local file. This PR suggests you've found urls to also work?
2. Because coverPage determines something that gets injected into the book from a path, I worry about letting a user pass in whatever they want. Would be good to verify that when a user passes coverPage it's a link we can download. Otherwise I picture users sending things like /etc/passwd and attempting to get sensitive files bundled in...
3. The metadata can actually only be a few set things (see nodepub docs). I question if it makes sense to accept anything if we only really want to support a finite set of fields. Perhaps we could iterate over the valid keys and copy over the ones passed?

Curious your thoughts.

Also - I'm in the middle of migrating things to a new host and updating the deploy pipeline. Hoping to get that work wrapped up soon and then pushing out these changes should be a breeze 😎

[wrobbins]

Howdy!

1. Yep - the url works. Dug into nodepub it reads the coverPage with fs.readFile (node docs)
2. I think it is fair to check for and require configured coverPage to start with http
   1. As far as accessing host files that should be covered by permissions on the host. In other words, I hope that anybody hosting this runs it for a user that is locked down. Of course, requiring only a http URI should eliminate this concern.
3. Totally fine moving coverPage up to a Named field and removing the metadata option. This would be like what you have done w/ the tests here.

Happy to continue discussions if warranted. Cool project !

[haroldtreen]

Oh boy! That's awesome! TIL fs.readFile can accept a url. That should work then 🎉 .

You're right about the host permissions - I feel paranoid though and would rather not trust the environment configuration to protect against that type of stuff 😅 . I think it's also from a user experience standpoint - if the user passes some weird file path we aren't leaving it to chance what they get back.

Ooo - didn't realize I already had validation for this. Seems like a good spot. Would also be reasonable to add the other nodepub options while we're at it.

Thanks for the being so thorough with this!

[wrobbins]

@haroldtreen changed to a Draft to facilitate a little less noisy collaboration, perhaps ?

I think I covered the immediate things we talked about but Code Climate may grow the scope of this change ...

lib\book.js is longer than CC likes - here are some of my suggestions to resolve, curious if you have other ideas:

• move static methods into lib\BookUtil.js

-- or --

• increase CC tolerances

-- or --

Book does have a lot of responsibilities which could be extracted out into more SRP files and classes

• Validation: section, metadata
• Formatting: character replacement and sanitizing
• Data access layer: find and commit
• ePub generation: creation of HTML, instance of nodepub
• File I/O: deleteFiles deleteAssets

To extract a handful of them it could look like:

• create lib\book and move class Book into \lib\book\index.js
  • extract DAL responsibilities by creating class BookRepository in lib\book\repository.js and moving Book.find and book.commit into it
  • extract ePub generation into lib\book\epub.js class Epub(book: Book)
  • extract sanitizing to lib\book\sanitization.js (sanitizeTitle, replaceCharacters)
  • extract validation to lib\book\validation.js

[haroldtreen]

Hey @wrobbins ! Thanks for all these updates. I have done most of the code with very little input, so really nice getting your perspective.

Code climate wise - I haven't been following it strictly. I mainly added it to see what it would suggest and put things on my radar. So could see a case for adding this feature and refactoring after.

That being said - I also see the case for breaking up the book class and like your proposed changes (although maybe instead of an Epub class it would be some kind of publisher class? Or book writer? The book is a bit of a weird class because a book can have one set of data and not be writeable, but if you send it through the BookServices pipeline then it becomes ready to publish... would be nice to make it more obvious when a book has been created from a request vs. been fully processed.)

Suppose it's up to you.

If getting this feature in will solve a use case for you and you're ready to move on - happy to just get it merged.
If you're curious to go down the path of patching things up - that would also be super cool. Otherwise I can find time to dig into it.

Also going to want to update epub-press-js when this is in... maybe it will Just Work ™️ , but at the very least going to want to document all these new flags and the cover page passing :). And then the chrome extension - lots of next steps :D.

[wrobbins]

@haroldtreen I'm good merging it now and following up with a bit of clean up of Book, Publisher could work I'm not attached to Epub. I'll continue thinking about it and toss some ideas your way.

Let me know how I can help getting docs updated - just not sure how you are doing things now. Since these capabilities are additive and option it should work with the clients.

I have a couple other ideas to modify this server if you're up for hearing them.

[haroldtreen]

Sounds good!
I've created some additional tickets to encapsulate the future work.

• Updating the clients
• Improving Code Climate

A lot of this code was written in 2016 with 0 external reviews - so appreciate getting your insight!

I've recently tried to revisit all this and improve things - just spent the last couple of weeks setting up continuous deployment and making things more observable. Would like to make the iteration quicker for this and simplify all the development / deployment.

Perhaps improvement/direction ideas would be best fleshed out in a ROADMAP.md type document? Or an issue? The document appeals as a single source of truth for things that will be prioritized. Happy to hear your thoughts.

With all the follow-up steps written out - LGTM 🎉 .

(I think this will be one of the first features to go out via CD - fingers crossed 🤞 !)

[haroldtreen]

Discovered some issues with this feature! Womp womp :(.

• There's a step where the provided cover is edited to add text - we want to skip that step if a user has provided a cover image.
• It looks like whatever you pass to NodePub needs to be a png - at least when it saves the file it defaults the name to cover.png. So a PR to nodepub might be in order to make that smarter / maybe EpubPress should download the image and convert it to png? It looks like the Epub standard only considers pngs, jpg, gifs and svg as supported.

I've created a ticket to track this work: #38

Always more work to be done 😅

[wrobbins]

I added the first requirement to #38

For the second; it works when you give it a JPG .. I'm not sure how far down the rabbit you want to go on validation. How about we test for valid file extensions in the url (png, jpg, gif) ?

How do you feel about adding some labels to this project ala https://help.github.com/en/github/managing-your-work-on-github/applying-labels-to-issues-and-pull-requests ?

[wrobbins]

@haroldtreen looks like c3e3b32 broke custom cover paths.

It appears that I was wrong about custom cover paths being downloaded from nodepub fs.ReadFile - instead it's Jimp that is responsible for pulling the image from the web. Skipping writeOnCover actually exposes the inability for fs.ReadFile to read the url correctly (it prepends a path on my box) then errors out.

I don't think a rollback of either commit is needed, but I'm going to make another PR to fix this issue if you're good with that.

[haroldtreen]

Ah - interesting... when I went to run the example I was seeing the default cover image coming through. I poked around and it seemed like the custom cover code was potentially setting it to something else... maybe I'll have to check again.

Agreed it doesn't seem to be broken so I didn't bother with rolling back. Not sure the best fix for this... perhaps the createCustomCover code could detect when the user has provided an image and download it in that step. Then update the coverPath of the book to be the file path instead of the url.

Open to other fix ideas though. The publishing pipeline is already pretty intense :P.