[To Main] DESENG-656 - Add user management to engagement authoring wizard #2556
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2556 +/- ##
=======================================
Coverage 75.96% 75.96%
=======================================
Files 609 609
Lines 21959 21960 +1
Branches 1711 1711
=======================================
+ Hits 16681 16682 +1
Misses 5015 5015
Partials 263 263
Flags with carried forward coverage won't be shown. Click here to find out more.
|
NatSquared
changed the title
| is_reviewer = CompositeRoles.REVIEWER.value in user_details.get('main_role') | ||
| is_team_member = CompositeRoles.TEAM_MEMBER.value in user_details.get('main_role') | ||
| is_reviewer = user_details.get('main_role') == CompositeRoles.REVIEWER.value | ||
| is_team_member = user_details.get('main_role') == CompositeRoles.TEAM_MEMBER.value |
There was a problem hiding this comment.
This was to fix a 500 error - main_role is a single string so it doesn't make much sense to ask if a role is "in" it
| // Remove ourselves from the list | ||
| user.id !== currentUser.userDetail.user?.id && | ||
| // Remove admins from the list - they can already see everything and will cause an error if specified | ||
| user.main_role !== USER_COMPOSITE_ROLE.ADMIN.label, |
There was a problem hiding this comment.
Is this trailing comma necessary?
There was a problem hiding this comment.
Yes, according to Prettier:
| > | ||
| <Grid item> | ||
| <BodyText bold color="primary.light"> | ||
| Team Member{selectedUsers.length > 1 && 's'} Added |
There was a problem hiding this comment.
Nice touch :). I probably would have just done "team member(s) added"
|
|
||
| return ( | ||
| <Box width="100%"> | ||
| <BodyText bold size="small"> |
There was a problem hiding this comment.
Is this meant to be the label for the autocomplete field?
| formData | ||
| .get('users') | ||
| ?.toString() | ||
| .split(',') |
There was a problem hiding this comment.
Is this converting an array to a string and back again? Is this a way to copy an array or something?
| .get('users') | ||
| ?.toString() | ||
| .split(',') | ||
| .forEach(async (user_id) => { |
There was a problem hiding this comment.
I don't think async/await works with forEach loops. https://stackoverflow.com/questions/37576685/using-async-await-with-a-foreach-loop
If you want to wait for the calls to finish before returning, you could use promise.all, for example. Otherwise you could just remove the async/await keywords and the execution would be the same.
|
Issue #: 🎟️ DESENG-656
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the met-public license (Apache 2.0).