[HOLD for payment] Invalid Props error on Workspace Avatar Edit

[mananjadhav]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. On browser open the Network console/for App check the terminal/debugger on
2. Visit the sidebar
3. Click on any Workspace to visit the page
4. Click on the Avatar to open the editor page
5. If the Avatar exists you should see an error automatically
6. If the Avatar doesn't exist then upload an image and you should see the error in the console.

Expected Result:

Should not log any error

Actual Result:

invalid-props-workspace-edit-web_P2PnqMf2.mp4

Workaround:

NA

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number:
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:

View all open jobs on Upwork

[MelvinBot]

Triggered auto assignment to @ntrepanier (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[mananjadhav]

Proposal

Problem Explanation

The error is thrown due to PropTypes mismatch. If you check Avatar component, it has a prop containerStyles defined as:

containerStyles: PropTypes.arrayOf(PropTypes.object),

but in AvatarWithImagePicker code is:

	<Avatar
	    containerStyles={styles.avatarLarge} // and here styles.avatarLage is an object
	    imageStyles={[styles.avatarLarge, styles.alignSelfCenter]}
	    source={this.props.avatarURL}
/>

Solution

In Avatar.js, change styling prop types to the following:

containerStyles: PropTypes.oneOfType([PropTypes.object, PropTypes.arrayOf(PropTypes.object)]),


// Even for imageStyles to avoid an exception in the future.
imageStyles: PropTypes.oneOfType([PropTypes.object, PropTypes.arrayOf(PropTypes.object)]), 

[Beamanator]

Is there anything actually not working in this flow? If everything works as expected I don't think this needs to be "fixed" now

[mananjadhav]

Nothing breaks for the User. Then we can close it.

[Beamanator]

Hmm I've been discussing this internally (here) and we may not close this actually, I'll post back here by tomorrow 👍

[mananjadhav]

Okay sure. Can't access the link, guessing it is internal only.

[Beamanator]

@mananjadhav yeah sorry about that, it's an internal link as you guessed 👍

[MelvinBot]

Triggered auto assignment to @mallenexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[Beamanator]

I'm going to start accepting proposals for this issue 👍

[mananjadhav]

Added proposal in the previous comments: #4489 (comment)

[Beamanator]

@mananjadhav yes thanks for your previous proposal :D Do you think there's a way we can avoid making the props for containerStyles and imageStyles an "object" OR an "array of objects"? I don't think it's good practice to allow a prop to be what you're suggesting (an "object" OR an "array of objects") - instead, maybe the parent component should be responsible for passing the proper data & data type into the component

As for imageStyles let's not preemptively change this proptype, only once we get a proptype error that's missed in a PR review 👍

[mananjadhav]

I agree. I gave this suggestion based on an existing code in the App. Honestly, I would recommend using StyleProp, ViewStyle, etc. for the relevant props. This ensures that we don't pass random keys and good enough validation.

Let me know what you think of this approach?

[mallenexpensify]

Hi @mananjadhav , per our Contributing.md , new contributors are only allowed to work on one issue at a time. Since I already created an Upwork job for #4434, how about you finish that then come back to this once that job is finished?

[mananjadhav]

@mallenexpensify I am aware of the policy, but I've already submitted the PR for the job. That should allow me to continue applying to this right?

[mallenexpensify]

@mananjadhav if you're aware of the policy, why did you apply for two jobs at one time? Please be sure to familiarize yourself with all aspects of the CONTRIBUTING.md and README.md files. It appears the two issues already have PR submitted. Please refrain from proposing solutions for additional jobs until 7 days after both PRs have merged. We do this to ensure there are no regressions.

[mananjadhav]

@mallenexpensify I thought PR submission should count for work done. I didn't realize it should be a PR merge + regression period before picking up the next gig. Noted.

[mallenexpensify]

@mananjadhav

I thought PR submission should count for work done.

Fair point, thanks for bringing it up. I'm addressing the issue with the team and will likely update the CONTRIBUTING.md to reflect a more accurate timeline.

[mallenexpensify]

@mananjadhav we're updating the CONTRIBUTING.md to read "New contributors are limited to working on one job at a time, their first PR must be merged before another proposal is accepted."

I'm putting this on hold til #4434 is merged. @Beamanator once 4434 is merged, are we set to hire @mananjadhav for this?

[mananjadhav]

@mallenexpensify That sounds fair. Appreciate your help with this.

[Beamanator]

I'm back with reviewing your proposal @mananjadhav :D

Honestly, I would recommend using StyleProp, ViewStyle, etc. for the relevant props. This ensures that we don't pass random keys and good enough validation.

Hmm I haven't seen this approach before, can you put example code here that you would use? Also do we do this anywhere else in the app so far? If not, maybe we can just keep the styles as generic PropTypes.arrayOf(PropTypes.object),, so in AvatarWithImagePicker we can just pass containerStyles as an array (containerStyles={[styles.avatarLarge]})

[mananjadhav]

StyledProp I've used with Typescript such as StyledProp<ViewStyle>.

And as I mentioned earlier, we don't use the above approach anywhere currently in the app.

But we do use PropTypes.oneOfType([PropTypes.object, PropTypes.arrayOf(PropTypes.object)]), in TextLink and AvatarWithImagePicker.

I would recommend using both object and array of objects check.. You don't want somebody accidentally passing an object again and throwing an exception. Let me know what you think?

[Beamanator]

Hmm on second thought I definitely agree with your first proposal, using PropTypes.oneOfType([PropTypes.object, PropTypes.arrayOf(PropTypes.object)]), - I hadn't noticed we use that in lots of other components, thanks for pointing that out 👍

@mallenexpensify once #4434 is merged we're ready to hire @mananjadhav 💪

[mananjadhav]

@Beamanator Just confirming again, you don't want me to touch the imageStyles right?

My PR for the other issue should get merged today. So I'll close this today itself.

[Beamanator]

Confirmed - please don't touch imageStyles unless you have another PR where you think you'd benefit from changing that or if you see it breaking in the future 👍

[mananjadhav]

Cool. Not breaking as of now. Will raise it separately if it occurs in the future.

[mananjadhav]

#4434 was merged yesterday. Hence, I've gone ahead and submitted this PR.

[Beamanator]

@mananjadhav I think it would be best if you wait for me or Matt to take this issue off "HOLD" before submitting the PR (next time) :D but I'd love your thoughts on that @mallenexpensify !

Anyway, I'm taking this off hold b/c you're right your other PR was merged 👍

[mananjadhav]

@Beamanator Okay noted. I'll wait for it next time.

Then one other quick question @Beamanator and @mallenexpensify, here we confirmed on Github that I'll be hired but not actually on Upwork yet. In that case, should I wait for the Upwork job to start before submitting the PR? or confirmation on GH is enough to get started on the work?

[Beamanator]

Great question @mananjadhav - If you're comfortable submitting the PR before you officially get hired in Upwork, please do! Confirmation in GH is "official" -> A.k.a. if we say you're hired here (in Github) then we'll definitely hire you in Upwork - there just may be a delay since Matt and I work in very different timezones 😅

[mananjadhav]

Thanks for the clarification.

That’s also the reason I submitted the PR. All three of us are in different time zones.

[mallenexpensify]

Created job and invited you @mananjadhav https://www.upwork.com/jobs/~01014d3713b3a319c7. Let me know when you've submitted a proposal and I'll hire you. I should have created it when I was assigned. (maybe I waited because it was on hold and it didn't make sense for others to submit proposals if you were going to? maybe I just forgot, ha)

@mananjadhav I think it would be best if you wait for me or Matt to take this issue off "HOLD" before submitting the PR (next time) :D but I'd love your thoughts on that @mallenexpensify !

This is a unique issue that has 'bounced around' more than others. Based on

@mallenexpensify once #4434 is merged we're ready to hire @mananjadhav 💪

It looks like it makes sense you'd submit a PR @mananjadhav , appreciate the promptness. The Upwork acceptance of a job is a formality but it won't always be done before a contributor starts work. We will always create the job and pay through Upwork though.

[mananjadhav]

@mallenexpensify I've submitted the proposal. Appreciate your help in resolving all queries 🙏

[mallenexpensify]

Hired in Upwork. Thanks for the quick work @mananjadhav

[Beamanator]

Forgot to assign @mananjadhav since he was assigned this job 👍

Also edited title to note it's on hold (so I can not worry about it on my todo list)

[mananjadhav]

@mallenexpensify Payment for this and the other job will be as per the updated "deploy to production" guidelines? or the earlier "7-8 days after merge"?

[mallenexpensify]

Hi @mananjadhav I paid both issues this morning and added the bonus for reporting the issues.
For any jobs created after last Friday, they'll be paid 7 days after production, per the post in Slack
Thanks!
Leaving this open til code hits production

[mananjadhav]

Thank you so much @mallenexpensify

[mallenexpensify]

Waiting for this to hit production..