Jiyuan - Fixed Assign Badges permission function#1387
Conversation
luisarevalo21
left a comment
There was a problem hiding this comment.
Hello @Jiyuanxia,
I tested your PR and works as intended. Great work!
Logging in as an owner and adding the assigning the badge permission to my volunteer account.
I then logged into the volunteer account and was able to assign badges to accounts.
Great work!
Screen.Recording.2023-10-06.at.7.36.46.PM.mov
JYXiao-2021
left a comment
There was a problem hiding this comment.
Hi, I test this pr and it works as intended.
Screen.Recording.2023-10-07.at.1.04.30.AM.mov
|
Hi @Jiyuanxia I tested this branch and the code work as intended. I am able to assign badges from my volunteer account. new_recording_-_10_7_2023._10_52_15_am.Original.mp4 |
wantingxu7
left a comment
There was a problem hiding this comment.
Hi,
the PR LGTM, before being given the "assign badge" permission, the volunteer cannot see the "assign badges" button. After being given the permission, the button shows up. Good work!
But the assign badge function seems not working for now from my local test.
1387.mp4
TuanDinh140194
left a comment
There was a problem hiding this comment.
Hi! I have test this PR and it worked as expected. I have logged in as admin and assign add badget to my volunteer account. After logged in my volunteer account, I have see the assign badget button in my profile. Very good job !
HGN.APP.-.Google.Chrome.2023-10-07.12-02-35.mp4
StrawberryCalpico
left a comment
There was a problem hiding this comment.
Hi! I just checked your PR, it worked perfectly! The badge assign button show up after updating the permission! Great job
video1277034748.mp4
navneeeth
left a comment
There was a problem hiding this comment.
Hi @Jiyuanxia! Tested your changes and the frontend fixes look good to me. But, I believe that a fix for this issue also requires a backend fix as the user will be unable to view all the badges and assign new badges to the user which is the purpose of this permission fix.
Testing video:
1387.mp4
The error:
Two areas in src/controllers/badgeController.js that need to be fixed in the backend
If you follow the convention in this frontend PR and its backend PR, you should be able to fix the issue. I see that you have mentioned in the Note that 'there is nothing on Assign Badges page' but this PR should also require that functionality to work to be considered a fix. Thank you!
There was a problem hiding this comment.
Hi Jiyuan
I have pulled both the front end and back end changes you made but am having trouble assigning the badges permission to a user
Below I log in as my Owner account and attempt to give my volunteer account the assign badges permission but get a error in the console and unable to successfully assign badges on the volunteer account
Untitled_.Oct.9.2023.2_31.PM.webm
|
Hi @Jiyuanxia I also pulled FE + BE branches to test out the functionalities, however, it turned out I cannot see the badges on the list (But I can see the assign badges button after modified the permission as an Owner). |
Hi @navneeeth , Thank you for reviewing my PR. The reason why user can not see Assign badges page is because lacking of "seeBadges" permission which is not provided in the permissions management system. Only admin and owner role has this permission by default. I have added it to the volunteer role and tested and it works well. So the issue is when "assignBadges" permission is assigned to a user, the "seeBadge" permission is not assigned accordingly so 403 error appears. I can submit another PR by adding "seeBadges" permission to default but it can not solve this issue totally. What we need to do is to make "seeBadges" permission either avalible in the system or when we assign a user "assignBadges" permission, assign them "seeBadges" permission too. |
vikrambadhan
left a comment
There was a problem hiding this comment.
Hi, I followed the instructions given in your PR and I was unable to view the badges. I think a backend fix is required to solve this issue.
2023-10-13.11-11-53.mp4
JeffLi117
left a comment
There was a problem hiding this comment.
Hi @Jiyuanxia, the code works all the way up until I try to assign another user a badge. The steps I took are as follows:
Logged in as Owner→ dashboard→ Other Links→ Permissions Management→ Manage User Permissions→ select my Volunteer account→ add "Assign Badges"→ Submit
Logged into said account→ looked into another user's account and was able to see "Assign Badges":

However, when I try to see if that newly granted permission is usable for my Volunteer account, I get the following result:

I tried searching for a few common badge names, but got no result. I saw your conversation with @navneeeth on "seeBadges" permission -- as this seems to need another PR to address (or to completely overhaul it into a new PR), I am approving it because you have added the ability for another user to "Assign Badges", they just can't see them.
|
Hi I'm testing your PR, I'm able to 'assign badges' from my account but after I click 'search badges' it came out nothing. Other than that, it works as expected. Screen.Recording.2023-10-14.at.11.33.41.mov |
Hi @Jiyuanxia! Thank you for addressing my review. Yes, such a permission does not exist in the system and that is why it needs to be created for this permission to work as expected in the frontend. I will approve this PR now since it only aims to fix the button's visibility in the UI. If you will be working next on a full fix on the permission in a new FE + BE PR set, please follow the conventions in the frontend and backend links I shared above and also the rules in the Permissions Management Fixes spreadsheet (You can find the link on the Bugs document by searching for the same title). Thank you! |
|
Hi @Jiyuanxia, Nicely done; I believe that there is a need for modifications and upgrade of the backend permisions for fetching badges. |
sxiong5
left a comment
There was a problem hiding this comment.
Hey @Jiyuanxia, I have pulled your changes and tested the PR, but I cannot see the badge list because of the error code 403. I think the issue comes from the backend, and it needs to be fixed.
Screen.Recording.2023-10-19.at.16.05.32.mov
Alforoan
left a comment
There was a problem hiding this comment.
I see the assign badge button. Nice!
pr.review.1387.mp4
Hi @sxiong5 and @vikrambadhan , thank you for reviewing. This pull request is intended to address the UI problem, and it has successfully fixed it. I have also identified the backend issue you mentioned. You can review the conversation between Naeneeeth and me above. Now, I am requesting you to re-review this. |
The aim of this PR is to fix the UI problem and it does.
|
Thank you all, moving to final review. |











Description
Related PRS (if any):
This frontend PR is related to the #547 backend PR.
Main changes explained:
How to test:
npm installand...to run this PR locallyScreenshots or videos of changes:
demo.mp4
Note:
If you click the Assign Badges button and see 403 error and there is nothing on Assign Badges page, it is ok because it is caused by lack of seeBadges permission.