Fixed #1750: Unexpected behavior when updation of talawa admin members profile. #1762

Devesh326 · 2024-01-29T15:02:34Z

What kind of change does this PR introduce?

bugfix

Issue Number:

Fixes #1750

Did you add tests for your changes?

No

Snapshots/Videos:

Palisadoes.Foundation.Talawa.Members.mp4

Summary

Solves the following issues:

When attempting to update a member's profile, it required changing the email ID.
Despite updating the profile, the member's actual profile remained unchanged. Instead, the user's (your) profile credentials w were updated to match the member's credentials.

Have you read the contributing guide?

Yes

github-actions · 2024-01-29T15:03:04Z

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

github-actions

Congratulations on making your first PR! 🎊 If you haven't already, check out our Contributing Guidelines and PR Reporting Guidelines to ensure that you are following our guidelines for contributing and creating PR.

Cioppolo14 · 2024-01-30T01:19:31Z

@EshaanAgg @xoldyckk Can you review this PR?

xoldd · 2024-02-01T13:02:47Z

src/resolvers/Mutation/updateUserProfile.ts

@@ -59,7 +59,7 @@ export const updateUserProfile: MutationResolvers["updateUserProfile"] = async (
  // Update User
  const updatedUser = await User.findOneAndUpdate(
    {
-      _id: context.userId,
+      _id: args.data?.id || context.userId,


why would you allow anyone to update any user's profile they want to?

I think this api call would be done by the admin only.

this restriction isn't put in place, right now it allows any user to call this mutation

This needs to be resolved. Only the profile's user, Admins and Super Admins must be able to edit profile settings.

Sure, on it

xoldd · 2024-02-01T13:03:50Z

src/typeDefs/inputs.ts

@@ -339,6 +339,8 @@ export const inputs = gql`
    lastName: String
    maritalStatus: MaritalStatus
    phone: UserPhoneInput
+    applangcode: String


appLanguageCode

Hey,

Regarding the variable name; that the frontend uses applangcode, so should I change it there also, otherwise it would give errors?

Yes please do that @Devesh326

@Devesh326 stop using abbreviations, it helps no one reading abbreviations to make sense of things, most of the time your code should be self documenting

in that light actually make it applicationLanguageCode

if frontend makes wrong assumptions about things, it doesn't mean it will be enforced on the backend design, if frontend wants to make changes it has to first open an issue/feature request

xoldd · 2024-02-01T13:04:53Z

src/resolvers/Mutation/updateUserProfile.ts

@@ -102,6 +102,9 @@ export const updateUserProfile: MutationResolvers["updateUserProfile"] = async (
        firstName: args.data?.firstName
          ? args.data.firstName
          : currentUser?.firstName,
+        appLanguageCode: args.data?.applangcode


args.data?.appLanguageCode

this hasn't been resolved still

xoldd · 2024-02-01T13:07:47Z

src/typeDefs/inputs.ts

@@ -339,6 +339,8 @@ export const inputs = gql`
    lastName: String
    maritalStatus: MaritalStatus
    phone: UserPhoneInput
+    applangcode: String
+    id: ID
  }


rename input UpdateUserInput to input UpdateUserProfileInput

make following changes for mutation:-

type UpdateUserProfilePayload { user: User } type Mutation { updateUserProfile(input: UpdateUserProfileInput!): UpdateUserProfilePayload }

Please renamed to suit the conventions @Devesh326

xoldd · 2024-02-01T13:08:38Z

src/typeDefs/inputs.ts

@@ -339,6 +339,8 @@ export const inputs = gql`
    lastName: String
    maritalStatus: MaritalStatus
    phone: UserPhoneInput
+    applangcode: String
+    id: ID


remove the id field from here, the update will only be applied for a user with _id === context.userId

if this mutation is to be allowed to be triggered by both normal users and superadmins, some changes have to be made:-

if args.input.id is not provided(it is null or undefined), the update will be carried out on the user with user._id equals to context.userId

if args.input.id is provided, there are two scenarios:-

if it's a normal user, context.userId must be equal to args.input.id

if it's a superadmin there are no restrictions, they can update any user with user._id equals to args.input.id

some things to confirm with @palisadoes :-

can one superadmin update other superadmin's user data?

is this allowed only for superadmins, or admins of organizations too? the thing to remember is that a user is not tied to any one organization, they can be members/admins of many organizations simultaneously, this functionality doesn't exist in talawa-api right now because of existence of userType === "ADMIN" field on a user object, when in actuality the relationship between a user and an organization should exist on a junction between them, joining table in sql it is called

@xoldyckk is correct on the userType to organization relationship. This should be fixed in this issue.

API: Fix the usage of userType #1711

Senior usertypes should be able to edit or demote users of junior rank. They should be able to promote junior ranks to no more than their rank. Super Admins will need privileges to do this for everyone. I'm broadly basing this on Google's guidelines for their apps.

https://apps.google.com/supportwidget/articlehome?hl=en&article_url=https%3A%2F%2Fsupport.google.com%2Fa%2Fanswer%2F1219251%3Fhl%3Den&assistant_event=welcome&assistant_id=usermasterbot&product_context=1219251&product_name=UnuFlow&trigger_context=a

Hey @xoldyckk,

I included the id in the mutation because during the API call to edit a specific user, in the backend context.userId only contained the id of the logged-in user. As a result, the profile being updated was that of the current user rather than the intended one. To address this, I passed the id of the user that requires editing/updating in the API call.

Following this logic, if args.input.id is null, it indicates the root user; otherwise, it's the id of the user whose profile needs updating.

ok. then it looks fine.

@Devesh326 take some time and contemplate about what i said previously, the current logic as it stands allows a normal user to update user fields of any other user, they just need to pass in the other user's id in the input, frontend application can't prevent normal users from making these requests

understand the authentication/authorization flows, self review your code and possible edge cases, only then ask for reviews from other contributers

SiddheshKukade · 2024-02-04T13:03:38Z

README.md


- [Talawa API](#talawa-api)
-  - [Talawa Components](#talawa-components)
-  - [Documentation](#documentation)
-  - [Installation](#installation)
-  - [Image Upload](#image-upload)
+- [Talawa Components](#talawa-components)
+- [Documentation](#documentation)
+- [Installation](#installation)
+- [Image Upload](#image-upload)



This is not required

Yeah apologies, I dont know why was this a part of the commit, i'll do the necessary changes. Thanks!

akhilender-bongirwar · 2024-02-12T15:40:43Z

@Devesh326, please make the reviewed changes ASAP? All the PRs in the talawa-admin repo are failing the graphql- introspection workflow.

Devesh326 · 2024-02-15T06:33:23Z

Sorry for the delayed response. I was travelling, so I wasn't able to make changes to the PR. Will update and resolve the requested changes ASAP.
Apologies again for the delay.

palisadoes · 2024-02-16T17:56:07Z

NOTE Read very carefully

We just merged this PR which upgraded the prettier package.
1. Upgraded prettier from version 2.8,8 to 3.2.5. #1840
It reformatted over 400 files. This will put your PR at risk of extensive merge conflicts.
I suggest that before you merge your code to the latest upstream, or manually fix these merge conflicts that you:
1. upgrade your prettier in your local branch to the same version
2. run prettier

This will help to reduce the number of future merge conflicts for your PR.

SiddheshKukade

Please fix the small changes in comments. Then it's ready to be merged.

codecov · 2024-02-18T05:10:12Z

Codecov Report

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

Project coverage is 98.32%. Comparing base (9cb6272) to head (bbed653).
Report is 4 commits behind head on develop.

Files	Patch %	Lines
src/resolvers/Mutation/updateUserProfile.ts	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #1762   +/-   ##
========================================
  Coverage    98.32%   98.32%           
========================================
  Files          322      322           
  Lines        20296    20299    +3     
  Branches      1606     1631   +25     
========================================
+ Hits         19956    19959    +3     
+ Misses         323      322    -1     
- Partials        17       18    +1

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

akhilender-bongirwar · 2024-02-25T09:34:10Z

@Devesh326, are you still actively working on this? The completion of this pull request is crucial, as it is causing failures in the workflow checks for all pull requests in the admin. Please prioritize and resolve this issue as soon as possible.

// @Cioppolo14

Devesh326 · 2024-02-25T09:41:01Z

Please fix the small changes in comments. Then it's ready to be merged.

yeah okay, fixing the necessary changes

…awa-api into develop

palisadoes · 2024-02-25T18:12:14Z

src/resolvers/Mutation/updateUserProfile.ts

@@ -59,7 +59,7 @@ export const updateUserProfile: MutationResolvers["updateUserProfile"] = async (
  // Update User
  const updatedUser = await User.findOneAndUpdate(
    {
-      _id: context.userId,
+      _id: args.data?.id || context.userId,


This needs to be resolved. Only the profile's user, Admins and Super Admins must be able to edit profile settings.

xoldd · 2024-02-28T17:25:12Z

@palisadoes if one user could join multiple organizations under the same user profile, why would admin of any one organization be able to edit their user profile? that profile change would be reflected in all other organizations for that particular user. shouldn't this only be allowed to superadmins?

palisadoes · 2024-02-29T17:39:51Z

The design philosophy of making users become members of any organization means that operationally if a member asks an admin to update their information, such as a phone number (or possibly in the future credit card information) the admin won’t be able to do it for them.

At some future date we’ll need to think of ways of restricting this to superadmins on a per organization basis.

palisadoes · 2024-03-03T15:23:42Z

Please try to get the code coverage for the patch closer to that of the repository

palisadoes · 2024-03-11T01:28:10Z

We would like to merge this PR, however you haven’t addressed the reviewer questions. Please address each one and mark them as resolved when the work is completed. Only then will they be able to approve your work so that it can be merged into the code base.

Please also try to get the code coverage for the patch higher. 80% coverage is much lower than the 98% enjoyed by the project

palisadoes · 2024-03-22T16:52:51Z

This is an update on the PR merging freeze:

In the next few hours we will be merging the develop-userTypeFix branch into the develop branch.
The develop-userTypeFix branch was created to fix a long standing design flaw where Admins were Admins of all organizations, not specific ones.
The userType field has been removed from the User collection and it has been replaced by an appUserProfileId field.
1. This field is null if the user isn’t registered to use the apps. This will help people to add users manually during the event checkin process, or if an Admin wants to manually add someone in the Admin dashboard.
2. When not null the AppUserProfileID will reference a AppUserProfile collection with App related information such as the organizations for which a user may be an Admin.
3. The updated schema can be found here https://github.com/PalisadoesFoundation/talawa-api/blob/develop-userTypeFix/schema.graphql
4. This is the parent issue that we have been using to track progress:
  1. PARENT ISSUE: Fix the usage of userType #1965
This merge will cause some conflicts in your PR.

We decided to do this at the beginning of the weekend to give us all time to adjust PR code and create bug fixes that may arise.

Update your code at or after midnight GMT on the morning of March 23, 2024. (5:30am IST).

If your PRs have already been approved, request a re-review after fixing the conflicts and refactoring to the new AppUserProfileID methodology.

palisadoes · 2024-03-23T15:53:44Z

The PR merging freeze is lifted.

We are working on bug fixes that may arise. These should be few and are being tracked here:
1. https://github.com/orgs/PalisadoesFoundation/projects/24
Please update your PRs and local repos. Fix any new merge conflicts that may have occurred.

Background:

The develop-userTypeFix branch was created to fix a long standing design flaw where Admins were Admins of all organizations, not specific ones.
The userType field has been removed from the User collection and it has been replaced by an appUserProfileId field.
1. This field is null if the user isn’t registered to use the apps. This will help people to add users manually during the event checkin process, or if an Admin wants to manually add someone in the Admin dashboard.
2. When not null the AppUserProfileID will reference a AppUserProfile collection with App related information such as the organizations for which a user may be an Admin.
3. The updated schema can be found here https://github.com/PalisadoesFoundation/talawa-api/blob/develop-userTypeFix/schema.graphql
4. This is the parent issue that we have been using to track progress:
  1. PARENT ISSUE: Fix the usage of userType #1965
If your PRs have already been approved, request a re-review after fixing the conflicts and refactoring to the new AppUserProfileID methodology.

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

@Devesh326 Please fix the conflicting files.

palisadoes · 2024-03-24T17:56:16Z

I'm going to close this as abandoned. There have been no commits for over a month.

Devesh326 added 3 commits January 26, 2024 19:24

fixed updation of talawa admin members profile and app language code

17f9cdb

Merge branch 'PalisadoesFoundation:develop' into develop

7b63184

Merge branch 'PalisadoesFoundation:develop' into develop

46961bc

github-actions bot reviewed Jan 29, 2024

View reviewed changes

Cioppolo14 requested review from xoldd and EshaanAgg January 30, 2024 01:19

This was referenced Jan 31, 2024

feat: added validation for links in advertisement PalisadoesFoundation/talawa-admin#1516

Merged

fix: Fixed custom field submission on Enter key press PalisadoesFoundation/talawa-admin#1524

Merged

xoldd reviewed Feb 1, 2024

View reviewed changes

Anvita0305 mentioned this pull request Feb 1, 2024

Corrected Admin button functionality PalisadoesFoundation/talawa-admin#1509

Merged

NayOoLwin5 mentioned this pull request Feb 2, 2024

Fix orgList screen refreshing entire data on load more orgs PalisadoesFoundation/talawa-admin#1525

Merged

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

SiddheshKukade reviewed Feb 4, 2024

View reviewed changes

akhilender-bongirwar mentioned this pull request Feb 5, 2024

Fix the introspection GitHub action tests (Feb 2024) PalisadoesFoundation/talawa-admin#1542

Closed

Merge branch 'develop' into develop

3f6b789

SiddheshKukade requested changes Feb 18, 2024

View reviewed changes

xoldd mentioned this pull request Feb 18, 2024

fix User Update Profile #1847

Closed

Merge branch 'develop' of https://github.com/PalisadoesFoundation/tal…

b6adacf

…awa-api into develop

palisadoes requested a review from SiddheshKukade February 25, 2024 17:20

changed applangcode -> appLanguageCode

06e37f9

palisadoes requested changes Feb 25, 2024

View reviewed changes

palisadoes added 2 commits March 3, 2024 07:40

Merge branch 'develop' into develop

d151510

Merge branch 'develop' into develop

3655736

Merge branch 'develop' into develop

bbed653

palisadoes removed the Ready 4 PR Review label Mar 13, 2024

palisadoes closed this Mar 24, 2024

palisadoes mentioned this pull request Mar 24, 2024

Unexpected behavior when updation of talawa admin members profile. #1750

Closed

Fixed #1750: Unexpected behavior when updation of talawa admin members profile. #1762

Fixed #1750: Unexpected behavior when updation of talawa admin members profile. #1762

Conversation

Devesh326 commented Jan 29, 2024

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

Our Pull Request Approval Process

Testing Your Code

Reviewers

Reviewing Your Code

CONTRIBUTING.md

Other

github-actions bot left a comment

Choose a reason for hiding this comment

Cioppolo14 commented Jan 30, 2024

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akhilender-bongirwar commented Feb 12, 2024 • edited Loading

Devesh326 commented Feb 15, 2024

palisadoes commented Feb 16, 2024

SiddheshKukade left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 18, 2024 • edited Loading

Codecov Report

akhilender-bongirwar commented Feb 25, 2024 • edited Loading

Devesh326 commented Feb 25, 2024

Choose a reason for hiding this comment

xoldd commented Feb 28, 2024

palisadoes commented Feb 29, 2024

palisadoes commented Mar 3, 2024

palisadoes commented Mar 11, 2024

palisadoes commented Mar 22, 2024

palisadoes commented Mar 23, 2024

The PR merging freeze is lifted.

Cioppolo14 commented Mar 24, 2024

palisadoes commented Mar 24, 2024

github-actions bot commented Jan 29, 2024 •

edited

Loading

akhilender-bongirwar commented Feb 12, 2024 •

edited

Loading

codecov bot commented Feb 18, 2024 •

edited

Loading

akhilender-bongirwar commented Feb 25, 2024 •

edited

Loading