Updated location Param to Address Object param in Organizations (Updated). #1740
Conversation
Our Pull Request Approval ProcessWe have these basic policies to make the approval process smoother for our volunteer team. Testing Your CodePlease make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:
The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. ReviewersWhen your PR has been assigned reviewers contact them to get your code reviewed and approved via:
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
Anubhav-2003
changed the title
|
@palisadoes Respected sir, I have created both the PR'S for the ADMIN portal, and the Updated PR for the Talawa-API simultaneously as previously discussed. Both the PR'S must be merged together for the successful implementation of the feature. Thank You. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #1740 +/- ##
===========================================
+ Coverage 98.17% 98.26% +0.08%
===========================================
Files 184 206 +22
Lines 10767 12709 +1942
Branches 835 1054 +219
===========================================
+ Hits 10571 12489 +1918
- Misses 186 188 +2
- Partials 10 32 +22 ☔ View full report in Codecov by Sentry. |
|
@xoldyckk @noman2002 Please review this PR, there is also a corresponding PR almost ready in talwa-admin (PalisadoesFoundation/talawa-admin#1490). |
|
@Cioppolo14 Ok ma'am, let me fix the failing tests Thank You. |
| @@ -66,8 +74,31 @@ const organizationSchema = new Schema( | |||
| type: String, | |||
| required: true, | |||
| }, | |||
| location: { | |||
| type: String, | |||
| address: { | |||
There was a problem hiding this comment.
confirm which fields are non nullable and add required: true to them
There was a problem hiding this comment.
- City and country code should be mandatory for the purposes of this PR
- As country code is unchanging it should be an enum and added to this PR
There was a problem hiding this comment.
Ok sir, I will make those two mandatory. As for the enums, I have already hardcoded an entire list of objects with country codes and country names in the admin portal for the drop down list. I found that creating an object would be more self-explanatory and better as compared to just an enum. you can find that change in the other PR. Thank you.
There was a problem hiding this comment.
@palisadoes @xoldyckk Respected sir, I have made the changes requested, and made countrycode, and city mandatory. But due to this change. Some tests are failing. I will be fixing those tests as early as possible.
Thank You.
|
@palisadoes @xoldyckk Respected sir, I have completely implemented the feature both in the API, and the ADMIN, and it is working properly. I have also made the countryCode, and city in the address mandatory from both the ADMIN portal, and the createOrganization resolver. The ADMIN PR has passed all the tests. But one major problem for making the address countrycode, and city mandatory from the schema level is the fact that currently the location param is not mandatory in the schema. As a result most of the test cases have been created with their own instances of the organization without the location param (and without a single point of truth), now making the address mandatory at that level would require substantial changes to all the test files. Now I can make all the changes but it would take time, as every test file has its own logic which I would need to examine before making the changes. This would lead to unnecessary delays to this feature that is completely functional. As the fields have already been mandated at the resolver level, as well as from the frontend. Thank You. |
|
|
@palisadoes Respected sir, I have already made the required changes in the sample data, and the sampleOrganization create resolver to accommodate for the changes in the fields. The updated data is already part of this PR. I have checked, and the sample data is being imported correctly. I am attaching a screenshot of it too. I think the PR is good to merge along with the front-end PR in the admin portal. Thank You. |
palisadoes
merged commit fcc2f8f
into
PalisadoesFoundation:develop
|
@Anubhav-2003 just wanted to clarify are there any updations required in |
|
@SiddheshKukade Respected sir, Since my change mostly dealt with change in the structure of Organization type. So in terms of initial setup, I had to change the structure of the Sample Organizations, and the utility function that created the sample orgs. No changes were required in my opinion in the setup.ts. I checked, and the updated sample organizations are being imported correctly into the DB after the PR. Thank You. |
|
@Anubhav-2003 I have been assigned to fix this issue in mobile repo, I need some help of yours to fix this but I am unable to find you on slack could you please drop a text to me on slack(same name and pfp) Thanks. |
What kind of change does this PR introduce?
A feature.
Issue Number:
#1650
Fixes #
Did you add tests for your changes?
Yes
Snapshots/Videos:
If relevant, did you update the documentation?
No.
Summary
Does this PR introduce a breaking change?
Yes, this PR is an updated PR for the initially merged PR #1666.
This PR must be merged with the corresponding ADMIN portal PR: PalisadoesFoundation/talawa-admin#1490 for successful and complete implementation of the feature.
Other information
Have you read the contributing guide?
No.