[HOLD for payment 2023-08-16] [$1000] Refactor MoneyAmountRequestPage to make it more reusable

[marcaaron]

The MoneyRequestAmountPage needs to be improved to make it more reusable for the "Edit Request" flow here. There are also some issues with the code.

Issues...

• MoneyRequestAmountPage has a hard dependency on props.iou. AFAICT that is a temporary Onyx key where we are storing things about a request that is currently being staged for creation. The long term plan is to use the transaction object. So, the first challenge is decoupling that dependency anywhere it is referenced.
• There is some navigation handling that should be moved out of this component. It also has a dependency on the props.iou Onyx key.
• There are a lot of things being derived from navigation params like - all over the place. Ideally we would be able to pass those as props and handle the navigation params in a parent page?
• All this navigation logic should be moved to a new component only used in the request creation flow.

Other things that are possibly incorrect...

• This is referencing an errors prop that doesn't seem to exist 🤔
• I am confused about why all of these values are stored in refs here.
• Having a useEffect() detect a change in props and then set state feels like an anti-pattern? Can we just use the prop?
• It would be good if all of the hooks were documented better + have some comment about why they are being used.

Deliverables:

• Create something like an AmountRequestPage and then have a MoneyRequestAmountPage and EditRequestAmountPage that both use AmountRequestPage.

MoneyRequestAmountPage - custom navigation stuff, uses the iou Onyx prop, etc.
EditRequestAmountPage - different navigation stuff, uses the transaction Onyx prop

• Clean up the existing issues and try to document the code a bit better so it is clearer how all of this works (especially the parts around custom navigation).

Thoughts on this approach?

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~018aca51d5f6723c8c
• Upwork Job ID: 1681488307261317121
• Last Price Increase: 2023-07-19
• Automatic offers:
• s77rt | Contributor | 26031120

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~018aca51d5f6723c8c

[melvin-bot]

Triggered auto assignment to @sophiepintoraetz (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to Design team member for new feature review - @shawnborton (NewFeature)

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)

[melvin-bot]

📣 @eVoloshchak Please request via NewDot manual requests for the Contributor role ($1000)

[ygshbht]

Proposal

The MoneyRequestAmountPage serves a significant purpose in the app but currently, it lacks reusability due to its structural issues and dependencies. The goal is to enhance its reusability by decoupling it from certain dependencies and moving non-core functionality, like navigation, to more appropriate areas:

Q: MoneyRequestAmountPage has a hard dependency on props.iou. AFAICT that is a temporary Onyx key where we are storing things about a request that is currently being staged for creation. The long term plan is to use the transaction object. So, the first challenge is decoupling that dependency anywhere it is referenced.

A: Indeed, to address this, we can assign props.iou to a variable, let's say iou, and use this variable throughout the component. This will eliminate the hard dependency on props.iou and make the component more versatile.

Q: There is some navigation handling that should be moved out of this component. It also has a dependency on the props.iou Onyx key. There are a lot of things being derived from navigation params - ideally, we would be able to pass those as props and handle the navigation params in a parent page?

A: Absolutely, moving navigation handling to a separate component will declutter the MoneyRequestAmountPage, allowing it to focus more on its core functionalities. Passing the navigation parameters as props will further improve the reusability of the component.

Q: It would be good if all of the hooks were documented better + have some comment about why they are being used.

A: Agree, proper documentation of hooks and the reasoning behind their use will not only increase readability but also ensure that their role within the component is clearly understood. This should be a standard practice for code maintainability.

Q: Having a useEffect() detect a change in props and then set state feels like an anti-pattern? Can we just use the prop?

A: Yes, particularly for the currency setting part, it can be handled via prop directly from the parent, eliminating the need for a state change on prop update - this is a more straightforward approach.

Q: This is referencing an errors prop that doesn't seem to exist.

A: The presence of an 'errors' prop that doesn't seem to exist suggests legacy code or a miscommunication in code intent. We need to investigate this further and modify or remove it accordingly to improve code quality.

Q: I am confused about why all of these values are stored in refs here.

A: The usage of refs here seems to be unnecessary for iouType, reportId, and isEditing as their values are derived from props and do not need to persist over rerenders. On the contrary, textInput seems to be correctly used as a Ref to store the DOM element. This suggests we need to review and modify the usage of refs in this component.

By following this plan, we aim to make MoneyRequestAmountPage more reusable and maintainable, which will prove beneficial for its use in the "Edit Request" flow and other potential future developments.

[ygshbht]

If I may express my thoughts,

considering the intricacy of MoneyRequestAmountPage, which isn't a brief file with approximately 500 lines, the task of refactoring it is not entirely straightforward. Beyond restructuring the code, we also need to guarantee that it operates flawlessly, devoid of any bugs or regressions. Given the scope of the task, I believe a minimum price range of $1500 to $2000 would be a more equitable starting point for this job

[samh-nl]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Refactor the MoneyRequestAmountPage component to enhance reusability.

What changes do you think we should make in order to solve the problem?###

Strategy

• Only keep core functionality and move the rest upstream, including navigation and the mentioned iou/transaction Onyx data.
• Simplify/break down in smaller functions the complex logic contained in the hooks and callbacks. This will result in more maintainable and testable code.
• The file does not adhere to the airbnb styling guidelines with regards to for example multi-line comments, with different styles being applied. I think we should use this refactoring opportunity to rid it of all these inconsistencies:
  

  App/src/pages/iou/steps/MoneyRequestAmountPage.js

  Lines 126 to 135 in 9508dfa

  	// The following logic will prevent users from pasting an amount that is excessively long in length,
  	// which would result in the 'absAmount' value being expressed in scientific notation or becoming infinity.
  	if (/\D/.test(absAmount)) {
  	return CONST.IOU.AMOUNT_MAX_LENGTH + 1;
  	}
  	/*
  	Return the sum of leading zeroes length and absolute amount length(including fraction digits).
  	When the absolute amount is 0, add 2 to the leading zeroes length to represent fraction digits.
  	*/

• Reconsider odd design choices like using useRef, or anti-patterns (setting state inside useEffect).
• Reconsider function names that violate naming conventions laid out in STYLE.md, such as onMouseDown.
• Consider moving util functions to a Utils file and writing unit tests.
• Consider turning it into a component rather than a page.
• Remove redundant code, such as unused dependencies (props.errors).
• Improve documentation where possible.

I think the bounty is fine, but a more lenient time window to complete the refactor may be appropriate.

What alternative solutions did you explore? (Optional)

N/A

[rayane-djouah]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The MoneyRequestAmountPage component in our codebase currently has several issues that limit its reusability and can potentially lead to bugs or confusion for developers. The issues are as follows:

1. The component has a hard dependency on props.iou, making it tightly coupled and less reusable.
2. Navigation handling logic is handled within the component, complicating its structure.
3. The component references an errors prop that doesn't seem to exist.
4. Multiple values are stored in refs without clear reasons, causing confusion.
5. The component has a useEffect hook that detects a change in props and then sets state, which might be an anti-pattern.
6. The hooks used in the component are not documented well.

What is the root cause of that problem?

The root cause of these problems can be attributed to the way the MoneyRequestAmountPage component was initially implemented. The component was not designed with reusability in mind and has grown to include responsibilities that could be better handled elsewhere. Moreover, certain implementation decisions, like storing values in refs or the misuse of the useEffect hook, have added to the complexity and potential confusion.

What changes do you think we should make in order to solve the problem?

1. Decouple Dependencies: Refactor MoneyRequestAmountPage to remove the hard dependency on props.iou. Instead, destructure the necessary properties from props.iou and pass them as individual props. This change decouples the MoneyRequestAmountPage from the iou object, making the component more flexible and easier to reuse in different contexts.

// Before
function MoneyRequestAmountPage(props) {
    const id = props.iou.id;
    // Rest of the component
}

// After
function MoneyRequestAmountPage({id, ...props}) {
    // Now use id directly
    // Rest of the component
}

2. Externalize Navigation Handling: The navigation logic within the component increases its complexity and responsibilities. By moving this logic outside of the MoneyRequestAmountPage component, we can simplify the component and make it easier to test and reuse.

Create a custom navigation handling hook that accepts the necessary parameters and handles the navigation accordingly.

// Custom hook for navigation
function useNavigationHandler(iouType, reportID, iouId) {
    const navigation = useNavigation();

    useEffect(() => {
        // navigation logic...
    }, [iouId]);
}

// Use the custom hook in the component
function MoneyRequestAmountPage({iouType, reportID, iouId, ...props}) {
    useNavigationHandler(iouType, reportID, iouId);
    // Rest of the component
}

3. Handle Navigation Params in Parent Page: This change would involve handling navigation parameters at a higher level (in the parent component or page) and then passing them down as props to MoneyRequestAmountPage. This approach would simplify the component's structure and make it more reusable.

// Parent component
function ParentComponent() {
    const route = useRoute();
    const params = route.params;
    return <MoneyRequestAmountPage {...params} />;
}

// Child component
function MoneyRequestAmountPage({iouType, reportID, ...props}) {
    // Use iouType and reportID directly
    // Rest of the component
}

4. Remove Nonexistent Prop References: The reference to a nonexistent errors prop should be removed from the component. This will prevent potential bugs and make the component's code cleaner and more understandable.

5. Clarify Use of Refs: The use of refs in this component seems to be causing confusion. It's important to clarify why each ref is used by adding comments explaining their purpose. If possible, refactor the code to eliminate the need for refs.

6. Refactor useEffect Usage: The current use of useEffect to detect changes in props and then set state seems to be an anti-pattern. This could potentially lead to unnecessary re-renders and make the component more difficult to reason about. Instead, you can use the prop directly where it's needed.

// Before
useEffect(() => {
    setAmount(props.iou.amount);
}, [props.iou.amount]);

// After
// Use props.iou.amount directly in the component

7. Improve Documentation of Hooks: Each hook used in the component should be accompanied by comments explaining why it's being used and how it works. This will make the component's code easier to understand and maintain.

8. Create a Common AmountRequestPage Component: To facilitate code sharing, a common AmountRequestPage component should be created. This component would contain the shared logic and UI elements between MoneyRequestAmountPage and EditRequestAmountPage. Then, MoneyRequestAmountPage and EditRequestAmountPage can use AmountRequestPage, passing their specific behaviors via props.

// AmountRequestPage.js
function AmountRequestPage({amount, onAmountChange}) {
    // Shared logic
    return (
        <div>
            <input type="number" value={amount} onChange={onAmountChange} />
            {/* More shared UI elements... */}
        </div>
    );
}

// MoneyRequestAmountPage.js
function MoneyRequestAmountPage({iou, ...props}) {
    const onAmountChange = useCallback((newAmount) => {
        // Update amount in iou...
    }, []);

    return <AmountRequestPage amount={iou.amount} onAmountChange={onAmountChange} />;
}

// EditRequestAmountPage.js
function EditRequestAmountPage({transaction, ...props}) {
    const onAmountChange = useCallback((newAmount) => {
        // Update amount in transaction...
    }, []);

    return <AmountRequestAmountPage amount={transaction.amount} onAmountChange={onAmountChange} />;
}

These proposed changes aim to improve the MoneyRequestAmountPage component's reusability, simplify its structure, and enhance its maintainability. The changes would also make the component's code easier to understand, which is beneficial for both current and future developers.

What alternative solutions did you explore? (Optional)

None.

[twisterdotcom]

@marcaaron do we need a C+ here to evaluate the proposals, or will you do it?

Any thoughts on bumping the price here as @ygshbht proposes? Our usual process is to rely on the market of proposals and bump if we don't think there are any within a week that are good to go.

Secondly, if the proposer we want to go with requests more to complete the job, we can discuss that then.

[koko57]

Hi :) I'm very close to opening my draft PR. But before I do that I need to do some cleanup and I've got one question: is there any possible scenario that would require that logic? Can we have a situation when the iou id from params don't match the iou id from Onyx? I'm asking because I feel that it's no longer necessary, but maybe I'm missing something and I just wanted to confirm.

[mountiny]

Good question, I think that you are right, but we might as well discover this during testing

[marcaaron]

Without having a ton of context (and being quite confused myself about it) I think the best path would be to try and git blame whichever PR added it and make sure we are not re-introducing any regressions / follow whatever test steps we had when it was first added.

[mountiny]

@koko57 Lets make sure to raise the PR tomorrow and lets work with some C+ to get it merged ASAP. We are focusing a lot now to finish the edit money request feature and this refactor/ improvement will help a lot so (its harder to continue without it and it will double the work for one of us) if we could nail this tomorrow, it would be super helpful. I can help out to get some C+ who could actively help to test it with you tomorrow

Here is WIP PR for the edit money request btw #23703

[koko57]

Draft PR #23979

[s77rt]

Can I get assigned here?

[koko57]

@marcaaron @mountiny I've opened a draft follow-up PR addressing all the NAB comments from #23979. I replied to some of them explaining why I solved the problems a bit differently. Let me know if I can open the PR for review.

I'll be ooo tomorrow, so if you request any changes, I'll be working on them on Friday.

[twisterdotcom]

@s77rt - is this for the reviews?

[s77rt]

@twisterdotcom Yes

[melvin-bot]

📣 @eVoloshchak Please request via NewDot manual requests for the Reviewer role ($1000)

[melvin-bot]

📣 @s77rt 🎉 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 📖

[mountiny]

@koko57 Is there anything left to do in this issue?

[koko57]

No, I think everything's done here

[mountiny]

thanks! @twisterdotcom I think we can then pay @s77rt for the review $1000 and close this out. Thank you!

[twisterdotcom]

I think the offer needs to be accepted first? I can't pay it, only make a new one.

[s77rt]

@twisterdotcom Payment is handled already on #24125

[twisterdotcom]

Ah, nice!