-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add total number lines changed since #202
base: dev
Are you sure you want to change the base?
Conversation
What's the use case for this? |
conn = await req.app.locals.pool.getConnection(); | ||
|
||
if (!Date) res.json({ error: 'please enter valid date' }); | ||
const query = `SELECT COUNT(*) FROM Verse WHERE Updated >= '${Date}'`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably use a parameterized query and/or do more thorough input sanitization on line 612
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some input sanitization that need to be done
- Date in the future
- Date not formatted properly.
both of those should return 422 with appropriate messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do i check if the date is formatted correctly? Do i have to use some regex thingy or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regex could work. check out https://regexlib.com/REDetails.aspx?regexp_id=95
i have an idea for updating a locally downloded db after some updates w/o releasing a new version like sundae gutka does |
How does the count help, though? Wouldn't you just the appropriate endpoint for what you want to pull? |
pull after a certain amt of changes |
@ManjotS does the |
its ok if its not really needed i can find some other way |
Yes updated is updated when the line changes in any way. Keep in mind it
could be translit change.
…On Wed, Jul 15, 2020, 9:07 PM Akal-Ustat Singh ***@***.***> wrote:
its ok if its not really needed i can find some other way
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#202 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE5OFJPE3RVVEH4NPOSSS3R3Z4HTANCNFSM4O3FADBQ>
.
|
maybe this isnt the best solution |
is this WIP? |
it is done but not sure if needed/wanted |
bump @tsingh777 @navdeepsinghkhalsa do you guys think this is useful to have or we can do without? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AkalUstat Please address code review suggestions.
@@ -102,7 +102,8 @@ exports.search = async (req, res) => { | |||
|
|||
if (searchQuery) { | |||
if (searchType === 0) { | |||
// First letter start | |||
// First letter start=> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=>{} not needed in this comment
try { | ||
conn = await req.app.locals.pool.getConnection(); | ||
|
||
if (!Date) res.json({ error: 'please enter valid date' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use lib.error
here with status code 400. Make the error message "Date not specified."
conn = await req.app.locals.pool.getConnection(); | ||
|
||
if (!Date) res.json({ error: 'please enter valid date' }); | ||
const query = `SELECT COUNT(*) FROM Verse WHERE Updated >= '${Date}'`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some input sanitization that need to be done
- Date in the future
- Date not formatted properly.
both of those should return 422 with appropriate messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test as well
@AkalUstat wouldnt a fixed # of line changes lead to fewer and fewer updates? ie as the db gets better youd wait longer and longer b/c there are less lines that will need changing |
If a GET request is submitted to
/shabads/updates/{INSERT DATE}
, the total verses changed since that date is displayed