Skip to content

Prepend http protocol#9

Merged
fabriciojs merged 2 commits intokool-dev:masterfrom
leandroncbrito:verify-http-protocol
Nov 10, 2020
Merged

Prepend http protocol#9
fabriciojs merged 2 commits intokool-dev:masterfrom
leandroncbrito:verify-http-protocol

Conversation

@leandroncbrito
Copy link
Copy Markdown
Contributor

@leandroncbrito leandroncbrito commented Nov 4, 2020

#8 It's not possible to convert using from-url without protocol http(s)

Added function to check if the URL has HTTP protocol and prepend it.

Copy link
Copy Markdown
Member

@fabriciojs fabriciojs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just one comment.

Comment thread pdf-service.js Outdated
});

function withHttp (url) {
if (!url) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need this check, do we?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't have the url parameter it'll return an error in line 90 (
I can change url to "let", remove it by applying the function after url parameter validation, like this:

    // change const to let
    let url = req.query.url;
    console.log('/from-url');

    if (!url) {
        let msg = 'missing parameter: \'url\'';
        deliverJson(res, {msg, params: req.params}, 400);
        return;
    }

    let pdfFilePath;
    try {
        // call the withHttp function before send to generatePdf()
        pdfFilePath = await generatePdf(withHttp(url), req.query.media);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabriciojs
What do you think? I like this new version of code, we check url parameter once

@fabriciojs fabriciojs merged commit cddd246 into kool-dev:master Nov 10, 2020
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.

2 participants