added onlyForFemale field #76

ritik307 · 2021-04-27T05:26:26Z

Description

The GET API currently can now filter on the basis of onlyForFemale field.

Fixes #69

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Test A
Test B

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

ritik307 · 2021-04-27T16:01:36Z

@Manvityagi Plz review

Manvityagi

There are merge conflicts that you need to resolve.
I don't see the corresponding updates in tests

ritik307 · 2021-04-28T14:26:35Z

1. There are merge conflicts that you need to resolve.

2. I don't see the corresponding updates in `tests`

will fix that asap

ritik307 · 2021-04-28T18:02:41Z

@Manvityagi Done Plz check

Manvityagi

Please take the latest pull of code, and
fix the things mentioned in comments.
There is no field called as opportunityId in the schema now

Manvityagi · 2021-05-01T06:38:32Z

services/opportunity/index.js

@@ -3,6 +3,7 @@ class opportunityService {
    this.opportunityManager = opportunityManager;
  }
  async createOpportunity(
+    opportunityId,


opportunityId field has been removed from the schema, Please remove it

Manvityagi · 2021-05-01T06:38:36Z

services/opportunity/index.js

@@ -18,6 +19,7 @@ class opportunityService {

    try {
      let newOpportunity = await this.opportunityManager.createOpportunity(
+        opportunityId,


opportunityId field has been removed from the schema, Please remove it

Manvityagi · 2021-05-01T06:56:34Z

routes/opportunity.js

@@ -30,6 +30,7 @@ const opportunityManager = new OpportunityManager(),
 *             - SCHOLARSHIP
 *             - CONFERENCE
 *             - CODINGCOMPETITION
+ *        description: Retrieve a list of opportunities based on particular type or only for female.
 *        description: Retrieve a list of opportunities based on particular type.
 *      - in: query


In documentation also, you should add female query parameter,
Visiting http://localhost:3030/playground/#/default/get_opportunity shall have female parameter added.

ritik307 · 2021-05-01T17:51:22Z

@Manvityagi @abdus Plz review

Manvityagi · 2021-05-01T18:22:06Z

@ritik307 The build fails on your PR.
Run your code & tests locally and fix the issue.
You can see the build log by clicking on the Travis details link in checks and see what's causing the build to fail.

ritik307 · 2021-05-01T19:32:18Z

@Manvityagi Plz review

Manvityagi

Please take the latest pull and resolve merge conflicts

ritik307 · 2021-05-02T08:15:07Z

@Manvityagi Done! Kindly review

abdus · 2021-05-04T17:55:23Z

managers/opportunity/index.js

@@ -54,6 +54,17 @@ class opportunityManager {
        queryObject['opportunityType'] = queryObject.type;
        delete queryObject.type;
      }
+      if (queryObject.female) {
+        queryObject['onlyForFemale'] = queryObject.female == 'true';


should not this be a boolean value? and please use === instead of ==.

abdus · 2021-05-04T17:56:11Z

managers/opportunity/index.js

+      let fetchedOpportunitiesQuery = this.opportunity.find(queryObject);
+      // fetchedOpportunitiesQuery.select('-__v');
+      // fetchedOpportunitiesQuery.select('-_id');
+      console.log(fetchedOpportunitiesQuery.data);


remove console.log(s). makes test output untidy.

ritik307 · 2021-05-04T18:02:22Z

@abdus Done! Plz review

abdus · 2021-05-08T12:06:18Z

@ritik307 please solve the merge conflict. I aim to accept/reject this PR by this weekend. It has been here for a long time.

ritik307 · 2021-05-08T12:15:24Z

@abdus Plz review
Just a matter of fact I always make the requested changes within a day or two but they didn't get a review for weeks.

abdus · 2021-05-08T16:54:54Z

the pagination thing is missing. I am attaching screenshots so that it's easier for you to understand.

sorry about being late. less free time than before.

ritik307 · 2021-05-08T17:06:53Z

@abdus My task was to add onlyforfemale field.

abdus · 2021-05-08T17:13:00Z

possibly overridden code blocks that were responsible for pagination? looking at your commit history, it looks like you have removed part of the js-doc.

abdus · 2021-05-08T17:13:54Z

managers/opportunity/index.js

+        delete queryObject.type;
+      }
+      if (queryObject.female) {
+        queryObject['onlyForFemale'] = queryObject.female === 'true';


also, why a boolean value is written as a string?

will fix that

ritik307 · 2021-05-08T17:16:02Z

possibly overridden code blocks that were responsible for pagination? looking at your commit history, it looks like you have removed part of the js-doc.

can you plz elaborate which js-doc you are talking about?

abdus · 2021-05-08T17:18:19Z

routes/opportunity.js

- *        name: page
+ *        name: female
 *        schema:
- *          type: Integer
- *          default: 1
- *        description: The Current Page for which the data User Wants
- *      - in: query
- *        name: limit
- *        schema:
- *          type: Integer
- *          default: 10
- *        description: The count of items/documents that should be returned on each page


ritik307 · 2021-05-08T17:32:54Z

@abdus plz review. btw I haven't removed the pagination code as far as I think the pagination code was merged before my issue that could be the reason it's missing from my PR.

abdus · 2021-05-09T08:27:23Z

did you actually test the code after pushing? the GET for opportunity is missing from swagger. after making changes, please go to localhost:3030/playground and check whether your code is actually working or not.

abdus · 2021-05-14T08:38:59Z

@ritik307 any updates?

abdus · 2021-05-16T15:35:38Z

participant inactive. closing...

added onlyForFemale field

7f343dc

ritik307 marked this pull request as draft April 27, 2021 06:17

added onlyforfemale filter

715989b

ritik307 marked this pull request as ready for review April 27, 2021 16:01

Manvityagi requested changes Apr 28, 2021

View reviewed changes

ritik307 added 2 commits April 28, 2021 23:30

added requested code to test

a905927

Merge branch 'develop' into opportunityType-onlyForFemale

7418058

ritik307 requested a review from Manvityagi April 28, 2021 18:02

Manvityagi requested changes May 1, 2021

View reviewed changes

ritik307 added 3 commits May 1, 2021 23:04

updated requested changes

d50437c

resolved conflict

a9a2a45

Merge branch 'develop' into opportunityType-onlyForFemale

1f8a9e3

ritik307 requested a review from Manvityagi May 1, 2021 17:51

ritik307 added 2 commits May 2, 2021 00:50

resolved travis conflict

a14535b

resolved travis conflict

4e33ff8

Manvityagi requested changes May 2, 2021

View reviewed changes

Merge branch 'develop' into opportunityType-onlyForFemale

b61375b

ritik307 requested a review from Manvityagi May 2, 2021 08:15

ritik307 added 2 commits May 2, 2021 14:37

Resolved build conflict

a3b34ca

Resolved build conflict

0d7bae5

abdus requested changes May 4, 2021

View reviewed changes

Update index.js

75efa4a

ritik307 requested a review from abdus May 4, 2021 18:02

Merge branch 'develop' into opportunityType-onlyForFemale

f9a13da

abdus reviewed May 8, 2021

View reviewed changes

Update index.js

6b41c38

abdus reviewed May 8, 2021

View reviewed changes

made pagination changes

d204fcf

abdus closed this May 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added onlyForFemale field #76

added onlyForFemale field #76

ritik307 commented Apr 27, 2021

ritik307 commented Apr 27, 2021

Manvityagi left a comment

ritik307 commented Apr 28, 2021

ritik307 commented Apr 28, 2021

Manvityagi left a comment •

edited

Manvityagi May 1, 2021

Manvityagi May 1, 2021

Manvityagi May 1, 2021

ritik307 commented May 1, 2021

Manvityagi commented May 1, 2021

ritik307 commented May 1, 2021

Manvityagi left a comment

ritik307 commented May 2, 2021

abdus May 4, 2021

ritik307 May 4, 2021

abdus May 4, 2021

ritik307 May 4, 2021

ritik307 commented May 4, 2021

abdus commented May 8, 2021

ritik307 commented May 8, 2021

abdus commented May 8, 2021

ritik307 commented May 8, 2021

abdus commented May 8, 2021

abdus May 8, 2021

ritik307 May 8, 2021

ritik307 May 8, 2021

ritik307 commented May 8, 2021

abdus May 8, 2021

ritik307 May 8, 2021

ritik307 commented May 8, 2021 •

edited

abdus commented May 9, 2021

abdus commented May 14, 2021

abdus commented May 16, 2021

added onlyForFemale field #76

added onlyForFemale field #76

Conversation

ritik307 commented Apr 27, 2021

Description

Type of change

How Has This Been Tested?

Checklist:

ritik307 commented Apr 27, 2021

Manvityagi left a comment

Choose a reason for hiding this comment

ritik307 commented Apr 28, 2021

ritik307 commented Apr 28, 2021

Manvityagi left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritik307 commented May 1, 2021

Manvityagi commented May 1, 2021

ritik307 commented May 1, 2021

Manvityagi left a comment

Choose a reason for hiding this comment

ritik307 commented May 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritik307 commented May 4, 2021

abdus commented May 8, 2021

ritik307 commented May 8, 2021

abdus commented May 8, 2021

ritik307 commented May 8, 2021

abdus commented May 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritik307 commented May 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritik307 commented May 8, 2021 • edited

abdus commented May 9, 2021

abdus commented May 14, 2021

abdus commented May 16, 2021

Manvityagi left a comment •

edited

ritik307 commented May 8, 2021 •

edited