Sign up route changed with bug fix and proper data importation #1770

Manik2708 · 2024-01-31T18:38:16Z

What kind of change does this PR introduce?

As per the need, now responsibility of creating a fresh member is shifted to Admin of organization from Super Admin.
Fixes the issue that previously adminApproved was only for testing purpose but now will be used for authenticating whether user is actually Admin approved or not.
acceptMemeberShip mutation is also modified to approve the user if it's his first request.
@ts-ignore and delete operand was used before to delete the password from createdUser, now omit method is used by creating a separate interface of the user which doesn't consists password.
Wrong import of data bug fixed

Issue Number:

Did you add tests for your changes?

Yes

Snapshots/Videos:

Video link for reference of bug getting fixed, and Talawa Admin working and importing of data in setup: https://www.mediafire.com/file/ujr35ynmlxy6ic3/Untitled+design.mp4/file

New SignUp Route

New Accept Membership Request Route

Query with output, when user tries to signUp with Organization that doesn't require User Registration

The organization which was used in above query

Query with output, when user tries to signUp with Organization that require User Registration

The default organization, which was also used in above query

Setup Route

If relevant, did you update the documentation?

Summary

Does this PR introduce a breaking change?

Yes, I have tried to make the system more secured by using `adminApproved` flag.

Other information

Have you read the contributing guide?

Yes

github-actions · 2024-01-31T18:38:41Z

Our Pull Request Approval Process

We have these basic policies to make the approval process smoother for our volunteer team.

Testing Your Code

Please make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:

The overall code coverage drops below the target threshold of the repository
Any file in the pull request has code coverage levels below the repository threshold
Merge conflicts

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.

Reviewers

Do not assign reviewers. Our Queue Monitors will review your PR and assign them.
When your PR has been assigned reviewers contact them to get your code reviewed and approved via:

comments in this PR or
our slack channel

Reviewing Your Code

Your reviewer(s) will have the following roles:

arbitrators of future discussions with other contributors about the validity of your changes
point of contact for evaluating the validity of your work
person who verifies matching issues by others that should be closed.
person who gives general guidance in fixing your tests

CONTRIBUTING.md

Read our CONTRIBUTING.md file. Most importantly:

PRs with issues not assigned to you will be closed by the reviewer
Fix the first comment in the PR so that each issue listed automatically closes

Other

🎯 Please be considerate of our volunteers' time. Contacting the person who assigned the reviewers is not advised unless they ask for your input. Do not @ the person who did the assignment otherwise.
Read the CONTRIBUTING.md file make

Manik2708 · 2024-01-31T18:41:04Z

I have tested the api in all the way I can think of, @palisadoes, the infinite scrolling bug is fixed now and importation of data and bug being fixed is shown in video (link is in PR description, as video was large)

codecov · 2024-01-31T18:46:48Z

Codecov Report

Attention: Patch coverage is 86.34921% with 43 lines in your changes are missing coverage. Please review.

Project coverage is 96.06%. Comparing base (36b3f5d) to head (e2b6800).

Files	Patch %	Lines
src/setup/importDefaultOrganization.ts	27.08%	35 Missing ⚠️
src/resolvers/Mutation/signUp.ts	96.42%	5 Missing ⚠️
src/utilities/loadDefaultOrganizationScript.ts	0.00%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1770      +/-   ##
===========================================
- Coverage    96.17%   96.06%   -0.11%     
===========================================
  Files          333      336       +3     
  Lines        22536    22726     +190     
  Branches      1949     1995      +46     
===========================================
+ Hits         21673    21831     +158     
- Misses         854      888      +34     
+ Partials         9        7       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Cioppolo14 · 2024-01-31T20:09:19Z

@Manik2708 Please fix the failing code coverage tests.

Manik2708 · 2024-02-01T03:09:39Z

@Manik2708 Please fix the failing code coverage tests.

Actually this is a bug fix for #1741, The files changed by me are singnUp.ts and acceptMembershipRequest.ts, and I have written tests for it, rest of the files are modified to resolve conflicts. If need is there, I can write tests for other files also, but it will require more time.

Cioppolo14 · 2024-02-02T13:14:36Z

@beingnoble03 @anwersayeed Can you review this PR?

palisadoes · 2024-02-04T19:10:34Z

@Manik2708 please fix the conflicting file

Manik2708 · 2024-02-04T19:46:44Z

@beingnoble03 @EshaanAgg @anwersayeed Please review!

EshaanAgg

I can't understand the reasoning behind the UserToReturn model. The best way would be just to remove the password field from the User model in src/typeDefs/types so that the client can never request for the same. Database schema and GraphQL can be different (and in most cases they should be).

EshaanAgg · 2024-02-05T15:07:28Z

src/models/User.ts

+ * This is an interface of the user that will be returned to the client side.
+ * The differrence between this interface and the real User Interface is that it doesn't contains password field
+ * Although this is a poor way, a better way will include implementing this Model with inclusion of password field and then using that model everywhere.
+ */
+export interface InterfaceUserToReturn {
+  _id: Types.ObjectId;
+  address: {
+    city: string;
+    countryCode: string;
+    dependentLocality: string;
+    line1: string;
+    line2: string;
+    postalCode: string;
+    sortingCode: string;
+    state: string;
+  };
+  adminApproved: boolean;
+  adminFor: PopulatedDoc<InterfaceOrganization & Document>[];


Why are we creating a model for this? This is already handled in the GraphQL resolvers by adding the password: 0 argument to the database calls so that the hashed password is not fetched from the database by Mongoose.

Manik2708 · 2024-02-07T06:40:38Z

I can't understand the reasoning behind the UserToReturn model. The best way would be just to remove the password field from the User model in src/typeDefs/types so that the client can never request for the same. Database schema and GraphQL can be different (and in most cases they should be).

I also firstly tried in the same way, but User type doesn't have password field already! See this image:

But generatedGraphqltypes is taking User (GraphQL) as InterfaceUserModel which takes password as parameter. See this image:

See this also:

And that's why signUp payload is still giving password (Actually in the repo, a hack was there to skip this error by using delete operand, Graphql is not able to detect that password field is not there. Although there is no problem in it except that we have to @ts-ignore in it) See the image of signUp.payload when delete operand is not used

Manik2708 · 2024-03-24T12:05:38Z

@Manik2708

* Please fix the conflicting files so that we can review again and merge

Number of conflicts are very high. Please give me some time.

Cioppolo14 · 2024-03-24T12:05:48Z

@Manik2708 Please fix the conflicting files and failing tests.

Manik2708 · 2024-03-26T14:02:00Z

@palisadoes There is a doubt while changing the logic according to latest changes. Why adminApproved is still a part of AppUserProfile? Referring to this comment: #1862 (comment), this variable was to be removed. But it is not removed yet! And if it is not to be removed then how it is being used?

Another question which is there: Should user be allowed to enter Talawa Ecosystem if the membership request made by it (Please refer to PR template for the route) for the defaultOrganization is rejected? If no then again what's the use of adminApproved?

palisadoes · 2024-03-26T14:21:56Z

We are now approving members joining on a per-organization basis, that's why it's there.
Some organizations may want to automatically approve new members which would set this flag to true by default, that's why it's still there.

Manik2708 · 2024-03-26T14:29:27Z

We are now approving members joining on a per-organization basis, that's why it's there.

Some organizations may want to automatically approve new members which would set this flag to true by default, that's why it's still there.

Okay, so in this PR, I am going with this logic now:

If selectedOrganization requires registration, then a membership request will be made to that organization and if it is accepted adminApproved will become true.
Else adminApproved will become true in signUp mutation itself.

Olatade · 2024-03-26T16:51:32Z

@Manik2708 please can you fix the conflicting files?

palisadoes · 2024-03-26T22:47:45Z

We are now approving members joining on a per-organization basis, that's why it's there.

Some organizations may want to automatically approve new members which would set this flag to true by default, that's why it's still there.

Okay, so in this PR, I am going with this logic now:
1. If `selectedOrganization` requires registration, then a membership request will be made to that organization and if it is accepted `adminApproved` will become `true`.

2. Else `adminApproved` will become `true` in `signUp` mutation itself.

Yes.
Please fix the conflicting files.
I'd like to get this reviewed and merged soon. The issue has been open for 10 weeks

palisadoes · 2024-03-27T15:35:28Z

Please fix the failing GitHub actions.
We need to get this PR merged, it's been too long.

palisadoes · 2024-03-27T15:47:20Z

The setup script is out of scope of the issue.
You are trying to do too many things here.
Please exclude all the setup files from the PR as they don't relate to the issue. You can do this by running the commands below from the root directory of the repository

git add -u
git reset HEAD path/to/file
git commit

I'll create a separate issue to fix setup.

Focus on the original issue. This PR is taking much too long. I will soon have to close it

Manik2708 · 2024-03-27T17:37:15Z

@palisadoes Please review!

Manik2708 · 2024-03-27T18:03:36Z

Please see that tests for importDefaultOrganization can't be written. It can be tested directly in the CI, just like importSampleData.

Manik2708 · 2024-03-27T19:20:15Z

The setup script is out of scope of the issue.

You are trying to do too many things here.

Please exclude all the setup files from the PR as they don't relate to the issue. You can do this by running the commands below from the root directory of the repository
git add -u
git reset HEAD path/to/file
git commit
I'll create a separate issue to fix setup.

Focus on the original issue. This PR is taking much too long. I will soon have to close it

The logic is implemented but the problem lies in testing them. Writing tests for yargs is not easy and will take time. I have tried my best to test the maximum but it will definitely take inputs from other contributors. Most importantly assistance from mentors is required. I will not suggest to write completely exclude setup, as already a very high time has been given to this. So I don't think other contributors should spend time in implementing logic, rather they can spend more time on testing these features and fixing the issues that may rise once this PR is merged.

palisadoes · 2024-03-28T00:03:40Z

We can make the setup tests another issue. This is taking too long.
Is it ready for review now?

Manik2708 · 2024-03-28T02:17:17Z

We can make the setup tests another issue. This is taking too long. Is it ready for review now?

Yes

palisadoes

It fails here. I already have data in the DB

palisadoes

The code doesn't work with the latest Admin code.

The logo is missing
Registration fails
I imported the sample data

palisadoes · 2024-03-28T03:44:09Z

Focus on the registration part. This is taking too long, and the basic testing hasn't been applied.

I've opened this issue to fix setup.

Make setup create a default organization #2109

palisadoes · 2024-03-28T04:00:19Z

I'd like to put this in context. There has been:

35 comments in the issue
130 comments in this PR
10 weeks of discussion on a relatively simple PR
No resolution

In addition to this:

You said the PR was ready for review, and rudimentary manual tests were not done.
You have been working on other simple test PRs, ignoring this one.

I'm going to close this PR, and reassign the issue.

This is unfortunate.

Manik2708 · 2024-03-28T04:40:01Z

I'd like to put this in context. There has been:

35 comments in the issue

130 comments in this PR

10 weeks of discussion on a relatively simple PR

No resolution

In addition to this:

You said the PR was ready for review, and rudimentary manual tests were not done.

You have been working on other simple test PRs, ignoring this one.

I'm going to close this PR, and reassign the issue.

This is unfortunate.

No problem @palisadoes. I tried but failed. I hope my efforts in this PR will help the new assignee.

Manik2708 · 2024-03-28T04:44:06Z

I'd like to put this in context. There has been:

35 comments in the issue

130 comments in this PR

10 weeks of discussion on a relatively simple PR

No resolution

In addition to this:

You said the PR was ready for review, and rudimentary manual tests were not done.

You have been working on other simple test PRs, ignoring this one.

I'm going to close this PR, and reassign the issue.

This is unfortunate.

I never ignored this PR. Changes were so abrupt in the last month, that it was really difficult to cop-up. I always wanted to give my best that's why I picked other testing issues. I can't give the time back but can help the new assignee.

surajshirke160000

m purva [email protected]

Manik2708 and others added 14 commits January 24, 2024 19:29

Signup route changed

d606e28

Formatting done

026744d

Schemas fix

6ee8381

Setup File edited

be1b39b

Setup Route Changed

d0348de

Fix as per the need

8021cb1

Setup file working finally

8452ede

Merge branch 'develop' into SignUpRoute

9e5389a

Linting fix

0790722

Merge branch 'develop' into SignUpRoute

d4baf30

Conflicts fix

91932f4

Type Errors fixed

62cf9e0

Type Errors fixed

2002b50

Signup route changed with bug fix

a7a10c0

Manik2708 changed the title ~~Sign up route~~ Sign up route changed with bug fix and proper data importation Jan 31, 2024

Cioppolo14 requested review from beingnoble03 and anwersayeed February 2, 2024 13:13

palisadoes requested a review from EshaanAgg February 2, 2024 19:11

palisadoes added the Ready 4 PR Review label Feb 3, 2024

Merge branch 'develop' into SignUpRoute

48ddeb4

EshaanAgg requested changes Feb 5, 2024

View reviewed changes

Merge branch 'develop' into SignUpRoute

075ed40

palisadoes mentioned this pull request Mar 27, 2024

Make setup create a default organization #2109

Closed

Manik2708 and others added 5 commits March 27, 2024 22:41

Final logic

75457aa

Merge branch 'develop' into SignUpRoute

e716073

Reverting husky

344ca49

Reverting husky

f2342df

Reverting husky

5ab6096

Merge branch 'develop' into SignUpRoute

e2b6800

palisadoes requested changes Mar 28, 2024

View reviewed changes

palisadoes closed this Mar 28, 2024

surajshirke160000 reviewed May 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sign up route changed with bug fix and proper data importation #1770

Sign up route changed with bug fix and proper data importation #1770

Manik2708 commented Jan 31, 2024

github-actions bot commented Jan 31, 2024 •

edited

Loading

Manik2708 commented Jan 31, 2024 •

edited

Loading

codecov bot commented Jan 31, 2024 •

edited

Loading

Cioppolo14 commented Jan 31, 2024

Manik2708 commented Feb 1, 2024

Cioppolo14 commented Feb 2, 2024

palisadoes commented Feb 4, 2024

Manik2708 commented Feb 4, 2024

EshaanAgg left a comment

EshaanAgg Feb 5, 2024

Manik2708 commented Feb 7, 2024

Manik2708 commented Mar 24, 2024

Cioppolo14 commented Mar 24, 2024

Manik2708 commented Mar 26, 2024

palisadoes commented Mar 26, 2024

Manik2708 commented Mar 26, 2024

Olatade commented Mar 26, 2024

palisadoes commented Mar 26, 2024

palisadoes commented Mar 27, 2024

palisadoes commented Mar 27, 2024

Manik2708 commented Mar 27, 2024

Manik2708 commented Mar 27, 2024

Manik2708 commented Mar 27, 2024

palisadoes commented Mar 28, 2024

Manik2708 commented Mar 28, 2024

palisadoes left a comment

palisadoes left a comment •

edited

Loading

palisadoes commented Mar 28, 2024

palisadoes commented Mar 28, 2024

Manik2708 commented Mar 28, 2024

Manik2708 commented Mar 28, 2024

surajshirke160000 left a comment

Sign up route changed with bug fix and proper data importation #1770

Sign up route changed with bug fix and proper data importation #1770

Conversation

Manik2708 commented Jan 31, 2024

github-actions bot commented Jan 31, 2024 • edited Loading

Our Pull Request Approval Process

Testing Your Code

Reviewers

Reviewing Your Code

CONTRIBUTING.md

Other

Manik2708 commented Jan 31, 2024 • edited Loading

codecov bot commented Jan 31, 2024 • edited Loading

Codecov Report

Cioppolo14 commented Jan 31, 2024

Manik2708 commented Feb 1, 2024

Cioppolo14 commented Feb 2, 2024

palisadoes commented Feb 4, 2024

Manik2708 commented Feb 4, 2024

EshaanAgg left a comment

Choose a reason for hiding this comment

EshaanAgg Feb 5, 2024

Choose a reason for hiding this comment

Manik2708 commented Feb 7, 2024

Manik2708 commented Mar 24, 2024

Cioppolo14 commented Mar 24, 2024

Manik2708 commented Mar 26, 2024

palisadoes commented Mar 26, 2024

Manik2708 commented Mar 26, 2024

Olatade commented Mar 26, 2024

palisadoes commented Mar 26, 2024

palisadoes commented Mar 27, 2024

palisadoes commented Mar 27, 2024

Manik2708 commented Mar 27, 2024

Manik2708 commented Mar 27, 2024

Manik2708 commented Mar 27, 2024

palisadoes commented Mar 28, 2024

Manik2708 commented Mar 28, 2024

palisadoes left a comment

Choose a reason for hiding this comment

palisadoes left a comment • edited Loading

Choose a reason for hiding this comment

palisadoes commented Mar 28, 2024

palisadoes commented Mar 28, 2024

Manik2708 commented Mar 28, 2024

Manik2708 commented Mar 28, 2024

surajshirke160000 left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 31, 2024 •

edited

Loading

Manik2708 commented Jan 31, 2024 •

edited

Loading

codecov bot commented Jan 31, 2024 •

edited

Loading

palisadoes left a comment •

edited

Loading