Create a database for tracking sweepstakes entries. #80
Conversation
|
I should have time to review this this week. Very exciting! |
|
It definitely works, which is totally awesome, but I have some nitpicky items that I'd like to request be done before merging. One performance issue, others not as important or I could clean up later. I'll write them up in a review. |
| @@ -47,6 +48,51 @@ if (args.sendgrid_cc && args.sendgrid_cc !== '') { | |||
| process.env.SENDGRID_CC = args.sendgrid_cc; | |||
| } | |||
|
|
|||
| //A common location for performing DB upgrades | |||
| async function updateDB() { | |||
There was a problem hiding this comment.
As this function will continue to grow as new db changes are made, it should be in it's own file.
|
|
||
| if (version == 0) { | ||
| var pRet = sqlite.run( | ||
| 'CREATE TABLE GG_DATA(sweepURL VARCHAR(100) NOT NULL PRIMARY KEY, ' + |
There was a problem hiding this comment.
I mentioned a performance issue, but now I realize it's not an issue, sorry. I didn't see that sweepURL was the primary key, I was going to ask you to add an index. We're all good!
|
|
||
| /** | ||
| * Creates or opens existing database. | ||
| * @param path Database location |
There was a problem hiding this comment.
I really appreciate that you added jsdoc comments!
Two comments though. For @param, it should include the type, in this case {string}. And the @returns in this file all have Promise<void>, but they all return values so the void part isn't correct. Here should be Promise<string>, for example.
| @@ -298,6 +320,11 @@ async function handleGiveawayResult(page) { | |||
| console.log('sending email'); | |||
| await sgMail.send(msg); | |||
| } | |||
| //Store that we won | |||
| setProcessingCode('W'); | |||
There was a problem hiding this comment.
The codes should probably be defined as constants so in the future it will be easier to add/remove/look up them and be able to reference them across multiple files.
| var escGiveaway = `'${giveawayURL}'`; | ||
| sql = | ||
| 'UPDATE GG_DATA SET process_code = ' + | ||
| escCode + |
There was a problem hiding this comment.
Since code here coming from outside the function, this has potential for a sql injection. Should use db.prepare function to protect against that.
| @@ -407,6 +489,7 @@ async function enterVideoGiveaway(page) { | |||
| */ | |||
| async function enterGiveaways(page, pageNumber) { | |||
| //loop through each giveaway item | |||
| await sqlite.open('./gg.db'); | |||
There was a problem hiding this comment.
I don't know how much it hurts, but I'd assume opening and closing the db connection every page probably doesn't help anything? From a little googling it sounds like opening can be expensive, so it might hurt performance a little over time. I think it would be fine to open and close the connection once in index.js.
| async function updateDB() { | ||
| await sqlite.open('./gg.db'); | ||
| let version = -1; | ||
| var r = null; |
There was a problem hiding this comment.
Super nitpicky (and my fault for not putting this in eslint rules) but prefer let and const instead of var. It totally doesn't matter though (besides being consistent), so feel free to ignore it.
| version = r.vers; | ||
| } | ||
|
|
||
| if (version == 0) { |
There was a problem hiding this comment.
Another nitpicky one, use === instead of ==.
|
Thanks for taking a look at this. I have never wrote production level NodeJS before and I appreciate the input. I had some similar thoughts as you about opening and closing the DB, but I couldn't seem to workout the scope. I also had the === at first, but backed it off. LOL. I have never worked with jsdoc, so I was doing lots of copy and paste. I will work on your recommendations over the next couple of days and push them into my forked branch. I am pretty sure it will flow into the merge request. Please correct me if I am wrong on how to push fixes. |
|
You are correct, pushing them into your branch is all you need to do. Thanks for being gracious about the feedback! I have to do code reviews all the time at work, I've come to really appreciate getting and receiving feedback myself. Looking forward to releasing this. |
Apologies if this PR is a duplicate of jpchip#80 but I was not sure if the requested changes would be made after a month. Couldn't contribut to Kyptonova's branch due to restrictions. The original PR was good, but made changes according to the reviews and used some more ES7 syntax. From the original PR: Creates a database for tracking sweepstakes entries. After sweepstakes are entered, it will skip checking it in the future. This will prevent unnecessary hops between the main screen and individual sweepstakes. The thought being that this will cut down on login page requests and CAPTCHAs. Addresses jpchip#74.
Creates a database for tracking sweepstakes entries. After sweepstakes are entered, it will skip checking it in the future. This will prevent unnecessary hops between the main screen and individual sweepstakes. The thought being that this will cut down on login page requests and CAPTCHAs.
Addresses #74.