Test for Get Api Updated #97

Sukriti-sood · 2021-05-03T02:15:04Z

Fixes #82
Fixes #107
@Manvityagi
I have added test but , facing a problem , I am not getting idea how to stub chained mongoose Operation , I tried but only One is working (skip or limit ) not both.

Manvityagi · 2021-05-08T08:08:21Z

Fixes #82

@Manvityagi
I have added test but , facing a problem , I am not getting idea how to stub chained mongoose Operation , I tried but only One is working (skip or limit ) not both.

@Sukriti-sood
Try something like this, it should work

 try {
        result.results = await this.opportunity.find(
          {
            /* Everything*/
          },
          {
            /* No constraints */
          },
          {
            skip: skip,
            limit: limit,
          }
        );

Manvityagi · 2021-05-08T08:10:40Z

Also, pull latest code before making next commit @Sukriti-sood

Sukriti-sood · 2021-05-09T04:48:47Z

@Manvityagi @abdus Added Test , Please Review.

Sukriti-sood · 2021-05-24T06:22:14Z

@Manvityagi Please Review PR

ay2306 · 2021-05-24T07:23:17Z

You are writing unit tests, you should not change the source code. Please correct it. And your Travis Build is failing, look into it as well.

Sukriti-sood · 2021-05-24T07:25:07Z

You are writing unit tests, you should not change the source code. Please correct it. And your Travis Build is failing, look into it as well.

@ay2306 Actually Logic of Pagination Helper Was Wrong I just Corrected it.

ay2306 · 2021-05-24T07:30:50Z

Please raise an issue, explain why is it wrong? Make PR to that. Cannot review code without context.

Sukriti-sood · 2021-05-24T07:40:05Z

Please raise an issue, explain why is it wrong? Make PR to that. Cannot review code without context.

@ay2306 I have fixed the error, Can I explain it here , rather than making new issue which is not needed as changes already made I guess .

In the pagination helper variable like skip , start index , begin was not needed and
if (numDocs >= limit) { totalPages = Math.floor(numDocs / limit) + (numDocs % limit); }
This logic for total pages was also wrong.

Is making an Issue Needed ? I will make If its that necessary

ay2306 · 2021-05-24T08:00:04Z

Yes, create a new issue, issues should be tracked properly, link it in this PR, and indent properly so that the diff checker shows the changes only. You have added extra characters in many places and it is difficult to see what changes have you made. Please revert all changes shown in diff which you haven't actually changed.

Sukriti-sood · 2021-05-24T08:06:45Z

@ay2306 Why are u asking for revert, all the changes mentioned in this PR are only made by me. and that extra characters are due to prettifying code .

ay2306 · 2021-05-24T08:30:01Z

Yeah, dont beautify. Have to read complete file to find out changes.

Sukriti-sood · 2021-05-24T08:31:51Z

Yeah, dont beautify. Have to read complete file to find out changes.

Are you Mentor Here ? As Mentor/PA of This Project believes in Clean Code as much I know.

ay2306 · 2021-05-24T09:57:36Z

Dude, I was asked to review this PR. The issue is not with beautify, the issue is that since you beautified the whole file, it is difficult to see where are the changes. Beautify when we are sure that nothing new is added but I cannot say okay to a file without reading what is changed and it is difficult to do so because the whole file has been beautified. Imagine you are reviewing your PR and you see most of the diff say a file is changed because a tab has been added, but there are some changes as well so now you have to go line by line matching if this has been changed or not.

Sukriti-sood · 2021-05-24T10:07:07Z

@ay2306 I have already Opened Issue #107 for Pagination Helper

And For File Changed , There are Only 7 files Changed and Only 2 files are extra having with no code change ,

Still I will remove those 2 files in upcoming commit .

Anything Else needed.

ay2306 · 2021-05-24T10:09:23Z

In pagination.js, remove extra space so they are not treated as changed in diff

Sukriti-sood · 2021-05-24T10:25:42Z

@ay2306 I have removed extra File , Please Review it

ay2306 · 2021-05-24T10:33:01Z

You have added extra space, it is making helper/pagination.js like that

Remove beautification and just keep your changes there.

Sukriti-sood · 2021-05-24T10:37:56Z

@ay2306 Sir , where is extra space , I am not getting your words, Please explain me.

ay2306 · 2021-05-24T10:49:50Z

For example in helper/pagination.js
Earlier there was this line

Now that line has become this

Like this, there are several lines that are not changed at all but are shown as changed due to added indentation. This is not required and makes reviewing difficult because I have to compare each line one by one to see if this is something you added or this is only a tab. This defeats the whole purpose of having a diff.

Even if we review it, there will be many cases when we may need to come back to this PR in the future to see what changes were made. We need the changes to be reviewed by any developer as quickly as possible.

Sukriti-sood · 2021-05-24T13:16:40Z

@ay2306 Sir Please have a look now, Is this fine or still any changes needed..?

ay2306

Please look at the code review comments.

helpers/pagination.js

ay2306 · 2021-05-24T13:38:53Z

helpers/pagination.js

  // If the page entered exceeds the Max allowed value(totalPages)
  if (page > totalPages) {
    let maxAllowedPages = totalPages;
    result.results = 'The maximum allowed pages are ' + maxAllowedPages;
    return result;
  }
-
-  // StartIndex should be atleast 0 and for the min val of Page should be 1
-  if (startIndex < 0) {


Why did you remove these checks?

Purpose this has been updated in my code.

helpers/pagination.js

ay2306 · 2021-05-24T13:53:58Z

tests/fakedata.js

@@ -22,4 +22,6 @@ let stubValue = {
  updatedAt: faker.date.past(),
 };

-export default stubValue;
+let fakedataArray = [stubValue, stubValue, stubValue, stubValue];


Won't this generate an array of 4 same stubValues since stubValue is generated only once? Please note stubValue is a variable and not a method, so here it will be same

@ay2306 This is generated to test the cases like if limit is given so whats the output , and related to page query example :-
if page is 2 and limit is 2 resultant array will be of length 2.

What if limit is more than 4? You should add test for that as well. And wont it be better if we convert it to a function so we can call it whenever we want 6 or 10 times or even more.

Ya it will be nyc , @ay2306 Currently I am facing issue for stubbing both mongoose operation find, skip and limit , I need help in this , .

ay2306 · 2021-05-24T13:56:22Z

tests/managers/opportunity-test.js

+      expect(result.results).to.equal(
+        'The Parameters cannot be Negative or Zero'
+      );
+    });
  });



Please add tests for all situations where it will throw an error as well, like non-positive limit, non-positive page, cover more edge cases. These are only positive tests.

Ook I will do.

ay2306 · 2021-05-24T14:03:01Z

tests/services/opportunity-test.js

        type: stubValue.opportunityType,
-      });
+      })).results[0];


Add more assertions like expected length, if all entries are correct or not.

uncompleted tests added

5f46e51

abdus requested a review from Manvityagi May 9, 2021 08:45

tried to fix extra space

99ea209

ay2306 reviewed May 24, 2021

View reviewed changes

Sukriti-sood and others added 2 commits May 24, 2021 20:57

fix in paginationhelper.js

254ce7a

Merge branch 'develop' into feature

e9b0550

Sukriti-sood requested a review from ay2306 May 24, 2021 15:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test for Get Api Updated #97

Test for Get Api Updated #97

Sukriti-sood commented May 3, 2021 •

edited

Loading

Manvityagi commented May 8, 2021 •

edited

Loading

Manvityagi commented May 8, 2021

Sukriti-sood commented May 9, 2021

Sukriti-sood commented May 24, 2021

ay2306 commented May 24, 2021 •

edited

Loading

Sukriti-sood commented May 24, 2021 •

edited

Loading

ay2306 commented May 24, 2021 •

edited

Loading

Sukriti-sood commented May 24, 2021 •

edited

Loading

ay2306 commented May 24, 2021

Sukriti-sood commented May 24, 2021 •

edited

Loading

ay2306 commented May 24, 2021

Sukriti-sood commented May 24, 2021

ay2306 commented May 24, 2021 •

edited

Loading

Sukriti-sood commented May 24, 2021

ay2306 commented May 24, 2021

Sukriti-sood commented May 24, 2021 •

edited

Loading

ay2306 commented May 24, 2021

Sukriti-sood commented May 24, 2021

ay2306 commented May 24, 2021

Sukriti-sood commented May 24, 2021

ay2306 left a comment

ay2306 May 24, 2021

Sukriti-sood May 24, 2021

ay2306 May 24, 2021

Sukriti-sood May 24, 2021

ay2306 May 24, 2021

Sukriti-sood May 24, 2021 •

edited

Loading

ay2306 May 24, 2021 •

edited

Loading

Sukriti-sood May 24, 2021

ay2306 May 24, 2021

Test for Get Api Updated #97

Are you sure you want to change the base?

Test for Get Api Updated #97

Conversation

Sukriti-sood commented May 3, 2021 • edited Loading

Manvityagi commented May 8, 2021 • edited Loading

Manvityagi commented May 8, 2021

Sukriti-sood commented May 9, 2021

Sukriti-sood commented May 24, 2021

ay2306 commented May 24, 2021 • edited Loading

Sukriti-sood commented May 24, 2021 • edited Loading

ay2306 commented May 24, 2021 • edited Loading

Sukriti-sood commented May 24, 2021 • edited Loading

ay2306 commented May 24, 2021

Sukriti-sood commented May 24, 2021 • edited Loading

ay2306 commented May 24, 2021

Sukriti-sood commented May 24, 2021

ay2306 commented May 24, 2021 • edited Loading

Sukriti-sood commented May 24, 2021

ay2306 commented May 24, 2021

Sukriti-sood commented May 24, 2021 • edited Loading

ay2306 commented May 24, 2021

Sukriti-sood commented May 24, 2021

ay2306 commented May 24, 2021

Sukriti-sood commented May 24, 2021

ay2306 left a comment

Choose a reason for hiding this comment

ay2306 May 24, 2021

Choose a reason for hiding this comment

Sukriti-sood May 24, 2021

Choose a reason for hiding this comment

ay2306 May 24, 2021

Choose a reason for hiding this comment

Sukriti-sood May 24, 2021

Choose a reason for hiding this comment

ay2306 May 24, 2021

Choose a reason for hiding this comment

Sukriti-sood May 24, 2021 • edited Loading

Choose a reason for hiding this comment

ay2306 May 24, 2021 • edited Loading

Choose a reason for hiding this comment

Sukriti-sood May 24, 2021

Choose a reason for hiding this comment

ay2306 May 24, 2021

Choose a reason for hiding this comment

Sukriti-sood commented May 3, 2021 •

edited

Loading

Manvityagi commented May 8, 2021 •

edited

Loading

ay2306 commented May 24, 2021 •

edited

Loading

Sukriti-sood commented May 24, 2021 •

edited

Loading

ay2306 commented May 24, 2021 •

edited

Loading

Sukriti-sood commented May 24, 2021 •

edited

Loading

Sukriti-sood commented May 24, 2021 •

edited

Loading

ay2306 commented May 24, 2021 •

edited

Loading

Sukriti-sood commented May 24, 2021 •

edited

Loading

Sukriti-sood May 24, 2021 •

edited

Loading

ay2306 May 24, 2021 •

edited

Loading