Migrate policy API to remove policy members #10452
Conversation
iwiznia
changed the title
iwiznia
changed the title
|
This is on hold, but let's get it reviewed in the meantime |
| @@ -24,7 +24,9 @@ function isReady() { | |||
| Onyx.connect({ | |||
| key: ONYXKEYS.ACTIVE_CLIENTS, | |||
| callback: (val) => { | |||
| activeClients = !val ? [] : val; | |||
| if (val) { | |||
| activeClients = _.unique(activeClients.concat(val)); | |||
There was a problem hiding this comment.
Suggestion: Use if (val && activeClients.indexOf(value) < 0) instead of _.unique(). I think it would be better to do if you're trying to prevent the same value being put into the array again.
(sorry, I'm jaded against _.unique() after learning that it's not very performant).
There was a problem hiding this comment.
Not that it matters much here, since at most there will be 20 items, but updated
| @@ -1263,6 +1263,7 @@ function editReportComment(reportID, originalReportAction, textForNewComment) { | |||
| isEdited: true, | |||
| html: htmlForNewComment, | |||
| text: textForNewComment, | |||
| type: originalReportAction.message[0].type, | |||
There was a problem hiding this comment.
Why is this change in this PR? It seems unrelated.
There was a problem hiding this comment.
It is not. Messages is an array and we were not sending the type in the optimistic action (neither in the action from the server), so when we replace the message fully now, this property was not set and was causing issues (we would render the comment as fragment.text here
| .value(); | ||
| const policyMemberList = lodashGet(this.props, 'policyMemberList', {}); | ||
| const removableMembers = []; | ||
| let data = _.map(policyMemberList, (value, email) => { |
There was a problem hiding this comment.
value is very ambiguious. Name suggestion: policyMember
| .value(); | ||
| const policyMemberList = lodashGet(this.props, 'policyMemberList', {}); | ||
| const removableMembers = []; | ||
| let data = _.map(policyMemberList, (policyMember, email) => { |
There was a problem hiding this comment.
This logic is a bit weird. You are using a map to build data but also using it to generate removableMembers values. I feel like this would have been easier with a _.each to do different things.
Anyway besides that, I feel like data would be a lot cleaner with the chain. Its weird that it's doing some things here and then in the next line it does sortBy. Also its not doing filter so it can end up with empty values which will fail when lowerCase is done,
I think data should be generated via chain and removableMembers should keep its operations separate. Or use a _.each to build both but that's extra work which I feel is not worth it just yet here.
There was a problem hiding this comment.
Updated to use each
| const policyMemberList = lodashGet(this.props, 'policyMemberList', {}); | ||
| const removableMembers = []; | ||
| let data = []; | ||
| _.each(policyMemberList, (policyMember, email) => { |
There was a problem hiding this comment.
The update here to each alone is not correct. Notice in the previous code we filter to remove empty values but that's not here.
So over here I think you should not update data if details is empty otherwise you'll still get the same error when testing locally because it cannot do lowercase if value is empty. Sample data provided here. This will result in an app crash when members page is clicked which is bad,
So my suggestion is to update this, to be,
if (details) {
data.push({
...policyMember,
...details,
});
}There was a problem hiding this comment.
That was a hack that someone added because the app crashed and instead of fixing the real issue, they just removed the detail from the list, which is wrong. This works if you are using the latest version of auth, that returns personal details for all emails, even if they don't have explicitly set a name.
Well, at least I hope the crash you saw was due to outdated auth, sounds like it because it's the same I was getting before sending the auth fix.
There was a problem hiding this comment.
Oh, you also need the web PR of course
There was a problem hiding this comment.
Odd, im sure i updates all repos and did a makeall yesterday. Maybe its buggy on an existing workspace, I'll try again on a fresh workspace.
If its buggy on existing data we still have to consider it so that users existing workspaces don't crash.
There was a problem hiding this comment.
Should not be buggy on existing data. Maybe.you need to sign out and back in though, not sure.
| @@ -24,7 +24,9 @@ function isReady() { | |||
| Onyx.connect({ | |||
| key: ONYXKEYS.ACTIVE_CLIENTS, | |||
| callback: (val) => { | |||
| activeClients = !val ? [] : val; | |||
| if (val && activeClients.indexOf(val) < 0) { | |||
There was a problem hiding this comment.
Whats this change for? When i test your PR the command DeleteMembersFromWorkspace never gets called in the network tab and I think its because of the code here.
There was a problem hiding this comment.
I had to refactor this a bit because this was using the array merging feature of onyx that we changed. It is not called in the flow of that api command.
|
Discussed 1:1 with ioni. The app still crashes and network requests are a bit broken. I believe he mentioned he'll debug both on Monday now that he has more clues on why its breaking. |
|
ok, updated to fix that. |
|
Error message text is not centered here |
|
@iwiznia looks like this was merged without passing tests. Please add a note explaining why this was done and remove the |
|
Not true Melvin, go home, you are drunk |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to production by @AndrewGable in version: 1.2.11-4 🚀
|
…rsFromWorkspace Migrate policy API to remove policy members
|
I am tagging this PR to highlight an issue fixed here. All conditions in ternary expressions or left-hand operands on conditional renders, should be boolean. This PR is one of the PRs that uses conditional render with string operands, hence I am tagging it here for the contributors to check. We've also updated the item in the checklist with this PR to avoid this issue in the future. |
| style={[styles.flex1, styles.justifyContentEnd, styles.overflowHidden]} | ||
| onLayout={event => this.setState({skeletonViewContainerHeight: event.nativeEvent.layout.height})} | ||
| <FullPageNotFoundView | ||
| shouldShow={!this.props.report.reportID} |
There was a problem hiding this comment.
If we are viewing a report and it gets deleted we will get the not found view, this caused #28969. We should have taken into consideration if the report was previously accessible.
Hold on https://github.com/Expensify/Web-Expensify/pull/34618Needs:
Details
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/211560
$ #11067
Tests
No QA:
QA:
You don't have access to this chatmessageYou don't have access to this chatmessageYou don't have access to this chatmessagePR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
Screenshots
Web
Screen.Recording.2022-09-28.at.7.57.11.PM.mov
Mobile Web
Desktop
iOS
Android