Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Haoji fetch images from team page and multiple profile images selection for individuals #1531

Conversation

Haoj1
Copy link
Contributor

@Haoj1 Haoj1 commented Nov 12, 2023

Description

image

Related PRS (if any):

This frontend PR is related to the #608 backend PR.
and based on the previous PR #1470. In previous PR there was a typo in the filename

Main changes explained:

  • Update src/actions/userManagement.js and add a new column for owners to manage the profile pictures
  • Create src/actions/userManagement.js for admin to select proper profile picture that matches the user

How to test:

  1. check into current branch
  2. do npm install and npm run start:local to run this PR locally
  3. See backend how to setup #608, change the cronjob period
  4. log in as owner user
  5. go to other link→ user management → Profile Image →…
  6. Create a new user according to the name on https://www.onecommunityglobal.org/team/. If you cannot see your name on the site, feel free to create a new user with the following cases. Then wait for the cronjob execution
  7. Case 1: If the user already has a profile picture or the user is inactive, the cronjob will skip and not overwrite the original picture. (You can paste an image URL into the "Profile Image" and click "add". Wait and see if cronjob will overwrite the picture)
  8. Case 2: If the user name is unique in the team page and there is no profile picture, the cronjob automatically assigns one

To improve the loading time of usermanagement page, Now the user already assigned a profile pic would still have a "ADD" button

  1. Case 3: If the user name matches multiple images on the sites. For example, there is a person named Yifan Wang and another named Yi Wang, cronjob may not be able to tell the belongings of pictures. Owners in that case, could select the proper one from all possible pictures.
  2. Deleted, do not test(And this user who matches multiple images on the sites would receive an email for notification about the case, and could select a proper one in their own profile page.)

Screenshots or videos of changes:

Case1 & 2:

fetch.a.picture.if.do.not.have.one.mp4

Case 3:

handle.multiple.pictures.issues.mp4

Email sender:

Screen.Recording.2023-11-11.at.1.14.24.AM.mp4

Note:

URL is currently valid and available for the profile picture
Setup email sender in backend before testing
I have made a banner after removed after update pic rather than the email sending, so please do not test email sending

Copy link
Contributor

@Sophie-lei Sophie-lei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! I just tested this PR and I meet problems in cases 1 &2.
I found that after I created a new user who has a photo on the team website. The backend said that the profile is updated successfully. but I can not see any change in the photo. Please view the video for more details.
Screenshot 2024-01-24 at 16 50 44

Screen.Recording.2024-01-24.at.16.50.55.mov

I also test for case 3. I chose the name that has the same last name as the team number. but the select photo is not working. Please view the video.
Screenshot 2024-01-24 at 17 02 34

Screenshot 2024-01-24 at 17 03 07

Copy link
Contributor

@Pandani07 Pandani07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, I tested this PR and followed your steps, when I create a new user with the same name as one that exists on the website, the backend console says 'update picture' and nothing happens. The photo is not updated, please check the video.

Screen.Recording.2024-02-02.at.1.44.35.PM.mov

@XiaohanMeng XiaohanMeng self-requested a review February 15, 2024 21:04
Copy link
Contributor

@XiaohanMeng XiaohanMeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I tested this PR and encountered a problem at the very beginning. I followed your steps to change the first name in userHelper.js, but the code is underlined in red, possibly indicating a problem with JSON parsing. After creating the new user, there was no response on my backend terminal and no photo showing in the new user profile. For detailed information, please see my video.

1531-bug.mov

@nnamdixobi
Copy link

I reviewed it and found out that it does not reflect photo updates for new users on the website and does not update the photo for duplicate user names. I also got the same JSON parsing error when modifying the first name in userHelper.js.

@XiaohanMeng
Copy link
Contributor

Hi Haoji,
I retested this PR following your guidance on Slack. Now, the "update picture" function works in my backend terminal, but the picture does not update in the profile on the frontend page. Please see the video below.

1531+608_error.mov

@DiegoSalas27
Copy link

I have made an inline comment in the userHelper.js file suggesting a code change to prevent non active users profile images being updated. Overall, great work!

screen-recording-2024-03-01-at-123007-pm_siO0U1qh.mp4

@XiaohanMeng XiaohanMeng self-requested a review March 7, 2024 23:39
Copy link
Contributor

@XiaohanMeng XiaohanMeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I retested this PR and noticed some improvements from before. In Case 1, when I created a new user with a unique name, the picture was displayed correctly (Video 1). Then, I tried to create a user with a duplicate name, and while the picture was still displayed, there was an error shown in the backend terminal (Video 2). In Case 3, I followed your guidance in the video with name: "Haoji Wang", but there was no option for picture selection on my screen (Video 3).

PR1531+608-1.mov
PR1531+608-2.mov
PR1531+608-3.mov

Copy link
Contributor

@kylepham kylepham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested case 2 where I created a new user with a unique first and last name ("Marcus Nguyen") that I found on the team page, but it fetched multiple images of other users with the same last name "Nguyen". Please see the video.

Mar.26.Screen.Recording.mp4

Copy link

@hetvi11110 hetvi11110 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed this PR, as mentioned, and encountered a few bugs while testing. They are as follows:

  1. The user 'Hetvi Owner' has the last name 'Owner,' yet it fetched and assigned the image of Aly Shannon.
  2. I created a user named 'Falgun Patel' with the last name 'Patel.' Although there are four team members listed on the team page, only three user images were fetched, and Falgun's image was missing. However, when I tested the same case with another user with the last name 'Feng,' it worked perfectly.
  3. After assigning an image to the user, I still found the option to add an image to the same user by clicking the 'add' button.

I was also unsure how to test users with more than two words in their names. Additionally, in case three, I integrated the SMTP variables but still didn't receive any mail. I have attached a reference video here for the review.

PR.1531.-.1.mp4
PR.1531.-.2.mp4
PR.1531.-.3.mp4

Copy link
Contributor

@kylepham kylepham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Haoj1, I retested your PR in all 3 cases.

  • Case 1: inactive user doesn't get updated. Worked as intended.
case-1-inactive.mov
  • Case 2: active and unique user gets updated. I'm not sure if there was an issue with caching, but I used the same user as case 1, just switch the status from inactive to active, and it failed to update the profile picture.
case-2-active-failed.mov

However, after terminating the backend and spinning up again, it successfully updated the picture.

Screen.Recording.2024-04-17.at.3.16.40.PM.mov
  • Case 3: multiple active users with both first and last name, or just last name. I tested with the name "Haoji Lu" since I found multiple people with "Lu" in their names, but it only returned a single picture of yours. If I misunderstood, please let me know.
case-3.mov

@hetvi11110 hetvi11110 self-requested a review April 17, 2024 22:15
@XiaohanMeng XiaohanMeng self-requested a review April 26, 2024 14:48
Copy link
Contributor

@XiaohanMeng XiaohanMeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Haoji, the feature about adding pic now is great(Video1). But in case 3, I notice that the banner still appear when I refresh the screen after I close them(Video2).

1.mov
2.mov

@Chuehleo Chuehleo added High Priority - Please Review First This is an important PR we'd like to get merged as soon as possible and removed High Priority - Please Review First labels May 1, 2024
Copy link

@TXMO-dev TXMO-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fetch images from team page and multiple profile images selection for individuals, works as expected with no issues. Approved!

@one-community one-community added Needs New Developer This is a PR that is partially developed but needs someone new to take it over and finish it. do not review Do not review or look at code without full context and removed High Priority - Please Review First This is an important PR we'd like to get merged as soon as possible labels Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not review Do not review or look at code without full context Needs New Developer This is a PR that is partially developed but needs someone new to take it over and finish it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.