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

[$1000] Migrate ReportActionCompose.js to function component #16263

Closed
1 task
marcaaron opened this issue Mar 20, 2023 · 68 comments
Closed
1 task

[$1000] Migrate ReportActionCompose.js to function component #16263

marcaaron opened this issue Mar 20, 2023 · 68 comments
Assignees
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Reviewing Has a PR in review

Comments

@marcaaron
Copy link
Contributor

marcaaron commented Mar 20, 2023

Class Component Migration

Filenames

Task

  • We currently have some class components in our codebase that we would like to refactor to a function component.
  • Here's a link with some general advice on how to refactor a class component to a function component: https://react.dev/reference/react/Component#alternatives
  • If you need additional guidance, please ask in #expensify-open-source
  • Test for any regressions and verify that there are no breaking changes
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01131d62c055beb52a
  • Upwork Job ID: 1668304833402220544
  • Last Price Increase: 2023-06-12
  • Automatic offers:
    • 0xmiroslav | Reviewer | 26454734
    • situchan | Contributor | 26454736
@marcaaron marcaaron added Engineering Improvement Item broken or needs improvement. labels Mar 20, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Mar 20, 2023
@Expensify Expensify unlocked this conversation Mar 21, 2023
@roryabraham roryabraham self-assigned this Mar 21, 2023
@roryabraham
Copy link
Contributor

This one seems particularly nasty, so I'd like to take a stab. I'm pretty familiar with hooks but could use a refresher.

@melvin-bot melvin-bot bot added the Reviewing Has a PR in review label Apr 5, 2023
@roryabraham roryabraham added Weekly KSv2 and removed Reviewing Has a PR in review labels Apr 6, 2023
@marcaaron marcaaron changed the title [HOLD] Migrate ReportActionCompose.js to function component [HOLD][$250] Migrate ReportActionCompose.js to function component Apr 13, 2023
@marcaaron
Copy link
Contributor Author

Heads up! The pricing for this issue has been adjusted based on the scope of the work and the fact that no complex bug investigations or proposal are required.

@melvin-bot melvin-bot bot added the Overdue label Apr 17, 2023
@hannojg
Copy link
Contributor

hannojg commented Apr 18, 2023

Not sure if the issue is external but I can help with that refactor? (Need it to happen to unblock another PR 😄 )

@marcaaron
Copy link
Contributor Author

We are going to open these up soon. Our internal team has been slowly chewing through the first round of 30 or so. But would love to have you work on this one or some others @hannojg !

@roryabraham
Copy link
Contributor

Yeah, @hannojg I have a pretty big WIP here, and where I left off I was blocked because the lower-level Composer component also needs to be migrated in order for this to work.

@melvin-bot melvin-bot bot removed the Overdue label Apr 19, 2023
@melvin-bot melvin-bot bot added the Overdue label May 1, 2023
@roryabraham
Copy link
Contributor

I think @hannojg is going to take over my PR. @hannojg can you comment to confirm so I can assign this to you?

@melvin-bot melvin-bot bot removed the Overdue label May 2, 2023
@hannojg
Copy link
Contributor

hannojg commented May 2, 2023

Yes, here we go

@melvin-bot melvin-bot bot added the Reviewing Has a PR in review label May 9, 2023
@stas-kravchuk
Copy link
Contributor

hey @roryabraham , the last few days I have been working on the ReportAcitionCompose.js migration, and the draft pr can be viewed here, the main migration has been finished and everything seems to be working fine at first sight. I wanted to ask, you mentioned above that you were blocked by the low-level Composer component which also needs to be migrated to get everything working, but I think I managed to migrate the component without Composer migrations
does it need to be migrated anyway, even if everything will work without migrating it?
P.S. PR still needs some polishing, but core migration has been finished, I've done some general testing on mobile and web and haven't noticed any obvious bugs or issues, but I'll keep looking

@chrispader
Copy link
Contributor

@roryabraham i took over this issue from @stas-kravchuk. Are there any updates on his last question? Other than that, the component should be completely migrated and merged in main recently!

@roryabraham
Copy link
Contributor

Answered in slack here and here

@melvin-bot
Copy link

melvin-bot bot commented Aug 8, 2023

@chrispader, @roryabraham, @dylanexpensify, @0xmiroslav 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

@roryabraham
Copy link
Contributor

@0xmiroslav I see that @hannojg has just cleaned up the PR and it's ready for another review

@dylanexpensify
Copy link
Contributor

@0xmiroslav did we get this reviewed?

@0xmiros
Copy link
Contributor

0xmiros commented Aug 10, 2023

yes reviewed so far but just handed over to @situchan today for faster progress

@dylanexpensify
Copy link
Contributor

Didn't get an update today, will on Monday! Thanks @0xmiroslav! Welcome @situchan!

@dylanexpensify
Copy link
Contributor

@situchan can we get an update

@situchan
Copy link
Contributor

Feedback was addressed a few hrs ago and now PR is in final review

@dylanexpensify
Copy link
Contributor

Perfect, ty!

@dylanexpensify
Copy link
Contributor

Waiting for PR to merge

@melvin-bot
Copy link

melvin-bot bot commented Aug 22, 2023

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@0xmiros
Copy link
Contributor

0xmiros commented Aug 22, 2023

Not a blocker. Someone linked wrong link - #25649 (comment)

@dylanexpensify
Copy link
Contributor

Reviewing tomorrow! @0xmiroslav @situchan mind giving an update

@situchan
Copy link
Contributor

PR deployed to staging

@dylanexpensify
Copy link
Contributor

Nice - waiting for prod>no regressions

@dylanexpensify
Copy link
Contributor

same ^

@dylanexpensify
Copy link
Contributor

Should be today!

@dylanexpensify
Copy link
Contributor

Hmm, Payment isn't autopopulating, but @marcaaron we're good to pay yeah?

@situchan
Copy link
Contributor

Hmm, Payment isn't autopopulating, but @marcaaron we're good to pay yeah?

@roryabraham is assigned engineer here.
PR was deployed to production on Aug 24 so yeah today is due.

It took much efforts to review 1.3k lines of code and do full regression test as this is the most important component in the app. Also there were tens of conflicts and had to retest whenever conflict fixed.
Hope that would be considered in payment.

@roryabraham
Copy link
Contributor

Confirming that this was deployed to prod on Aug 24th so is ready to pay out.

Regarding payment, we actually had two C+ here. My proposal for payments is:

  • @0xmiroslav, who reviewed several times but ended up having to pass the PR off. I think we should pay $500 to @0xmiroslav instead of $0 to account for the work they did in the earlier phases of the PR reviews.
  • $1000 to @situchan (i.e: 4x the normal amount for functional component migrations)

I understand that this was a more time consuming PR to work on, and that's why I quadrupled the bounty early on here.

@melvin-bot
Copy link

melvin-bot bot commented Sep 1, 2023

📣 @0xmiroslav 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

@melvin-bot
Copy link

melvin-bot bot commented Sep 1, 2023

📣 @situchan 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot
Copy link

melvin-bot bot commented Sep 11, 2023

@chrispader, @roryabraham, @dylanexpensify, @0xmiroslav, @situchan Huh... This is 4 days overdue. Who can take care of this?

1 similar comment
@melvin-bot
Copy link

melvin-bot bot commented Sep 11, 2023

@chrispader, @roryabraham, @dylanexpensify, @0xmiroslav, @situchan Huh... This is 4 days overdue. Who can take care of this?

@roryabraham
Copy link
Contributor

This is done – @dylanexpensify bump to pay plz

@melvin-bot
Copy link

melvin-bot bot commented Sep 18, 2023

@chrispader, @roryabraham, @dylanexpensify, @0xmiroslav, @situchan Whoops! This issue is 2 days overdue. Let's get this updated quick!

@dylanexpensify
Copy link
Contributor

Paying tonight!

@dylanexpensify
Copy link
Contributor

@situchan payment sent! @0xmiroslav can you accept offer please?

@0xmiros
Copy link
Contributor

0xmiros commented Sep 19, 2023

@dylanexpensify please hold my payment and just close this issue to prevent further notifications.
I will ping here when ready. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests