Skip to content
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

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

AkalUstat
Copy link
Collaborator

@AkalUstat AkalUstat commented Jul 15, 2020

If a GET request is submitted to /shabads/updates/{INSERT DATE}, the total verses changed since that date is displayed

@tsingh777
Copy link
Contributor

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}'`;
Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Collaborator Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

@AkalUstat
Copy link
Collaborator Author

i have an idea for updating a locally downloded db after some updates w/o releasing a new version like sundae gutka does

@navdeepsinghkhalsa
Copy link
Contributor

How does the count help, though? Wouldn't you just the appropriate endpoint for what you want to pull?

@AkalUstat
Copy link
Collaborator Author

pull after a certain amt of changes

@tsingh777
Copy link
Contributor

tsingh777 commented Jul 16, 2020

@ManjotS does the Updated column behave the way this PR is expecting?

@AkalUstat
Copy link
Collaborator Author

its ok if its not really needed i can find some other way

@ManjotS
Copy link
Contributor

ManjotS commented Jul 16, 2020 via email

@KhalisFoundation KhalisFoundation locked and limited conversation to collaborators Jul 16, 2020
@KhalisFoundation KhalisFoundation unlocked this conversation Jul 16, 2020
@AkalUstat AkalUstat closed this Jul 16, 2020
@AkalUstat
Copy link
Collaborator Author

maybe this isnt the best solution

@AkalUstat AkalUstat reopened this Jul 16, 2020
@ManjotS
Copy link
Contributor

ManjotS commented Sep 9, 2020

is this WIP?

@AkalUstat
Copy link
Collaborator Author

it is done but not sure if needed/wanted

@AkalUstat
Copy link
Collaborator Author

bump @tsingh777 @navdeepsinghkhalsa do you guys think this is useful to have or we can do without?

Copy link
Contributor

@tsingh777 tsingh777 left a 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=> {
Copy link
Contributor

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' });
Copy link
Contributor

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}'`;
Copy link
Contributor

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.

Copy link
Contributor

@tsingh777 tsingh777 left a 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

@tarunsingh5
Copy link
Collaborator

@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

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.

None yet

5 participants