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

[HOLD for payment 2024-10-29] [$250] Add the Download option to the report details screen. #49638

Closed
JmillsExpensify opened this issue Sep 24, 2024 · 33 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.

Comments

@JmillsExpensify
Copy link

JmillsExpensify commented Sep 24, 2024

Problem

The original implementation for CSV downloads focused on bulk actions on the Search page. One small inconsistency this introduced is that it's still not possible to download individual reports. You could also argue that this inconsistency is now more glaring since it's possible to export reports to accounting systems, yet not CSV.

Solution

Let's add the ability to download reports to CSV at the report level, in addition to bulk CSV exports on the Search page. The solution will work like so:

  • All expense reports will contain a Download option in the details page for that report. This is the case no matter whether the report is a "one expense report" or whether it's a report that contains several "batched" expenses.
  • Clicking the Download option will generate a CSV and download it to the member's local device

And I think that's it? @trjExpensify do you think I'm missing anything else? Here's the relevant front-end example, for reference.

image

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021838716845402086708
  • Upwork Job ID: 1838716845402086708
  • Last Price Increase: 2024-09-24
  • Automatic offers:
    • nkdengineer | Contributor | 104193448
    • abzokhattab | Contributor | 104195345
Issue OwnerCurrent Issue Owner: @JmillsExpensify
@JmillsExpensify JmillsExpensify added Daily KSv2 NewFeature Something to build that is a new item. labels Sep 24, 2024
@JmillsExpensify JmillsExpensify self-assigned this Sep 24, 2024
Copy link

melvin-bot bot commented Sep 24, 2024

Current assignee @JmillsExpensify is eligible for the NewFeature assigner, not assigning anyone new.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Sep 24, 2024
@NJ-2020
Copy link
Contributor

NJ-2020 commented Sep 24, 2024

Edited by proposal-police: This proposal was edited at 2023-10-02T00:00:00Z.

Proposal

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

Add the Download option to the report details screen

What is the root cause of that problem?

New feature

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

We can add new menu item inside report details page

const menuItems: ReportDetailsPageMenuItem[] = useMemo(() => {
const items: ReportDetailsPageMenuItem[] = [];

// For handling error
const {isSmallScreenWidth} = useResponsiveLayout(); 
const [downloadErrorModalVisible, setDownloadErrorModalVisible] = useState(false);

if (isExpenseReport || isSingleTransactionView) {
    items.push({
        key: CONST.REPORT_DETAILS_MENU_ITEM.DOWNLOAD,
        translationKey: 'common.download',
        icon: Expensicons.Download,
        action: () =>
            Report.downloadReportCSV(report.reportID, () => {
                setDownloadErrorModalVisible(true);
            }),
        isAnonymousAction: true,
    });
}

return (
    ...
    <DecisionModal
        title={translate('common.downloadFailedTitle')}
        prompt={translate('common.downloadFailedDescription')}
        isSmallScreenWidth={isSmallScreenWidth}
        onSecondOptionSubmit={() => setDownloadErrorModalVisible(false)}
        secondOptionText={translate('common.buttonConfirm')}
        isVisible={downloadErrorModalVisible}
        onClose={() => setDownloadErrorModalVisible(false)}
    />
)

And add download option inside REPORT_DETAILS_MENU_ITEM property
DOWNLOAD: "download"

App/src/CONST.ts

Lines 2807 to 2819 in accea60

REPORT_DETAILS_MENU_ITEM: {
MEMBERS: 'member',
INVITE: 'invite',
SETTINGS: 'settings',
LEAVE_ROOM: 'leaveRoom',
PRIVATE_NOTES: 'privateNotes',
EXPORT: 'export',
DELETE: 'delete',
MARK_AS_INCOMPLETE: 'markAsIncomplete',
CANCEL_PAYMENT: 'cancelPayment',
UNAPPROVE: 'unapprove',
DEBUG: 'debug',
},

And create new function to download report as csv inside actions/Report.ts folder because almost all download as csv function are created under actions libs folder

/** Download report details as CSV */
function downloadReportCSV(reportID: string, onDownloadFailed: () => void) {
    const finalParameters = enhanceParameters(WRITE_COMMANDS.DOWNLOAD_REPORT_CSV, {
        reportID,
    });

    const fileName = `report_${reportID}.csv`;

    const formData = new FormData();
    Object.entries(finalParameters).forEach(([key, value]) => {
        formData.append(key, String(value));
    });

    fileDownload(ApiUtils.getCommandURL({command: WRITE_COMMANDS.DOWNLOAD_REPORT_CSV}), fileName, '', false, formData, CONST.NETWORK.METHOD.POST, onDownloadFailed);
}

We also need to create new endpoint inside the BE for download report as csv (because almost all download as csv function had endpoint to the BE like downloadCategoriesCSV, downloadTagsCSV, etc)

And create new API params for DOWNLOAD_REPORT_CSV

// DownloadReportCSVParams.ts
type DownloadReportCSVParams = {
    reportID: string;
};

export default DownloadReportCSVParams;

// And export it inside parameters/index.ts
export type {default as DownloadReportCSVParams} from './DownloadReportCSVParams';
 
// API/types.ts
...
DOWNLOAD_REPORT_CSV: 'DownloadReportCSV',
...
[WRITE_COMMANDS.DOWNLOAD_REPORT_CSV]: Parameters.DownloadReportCSVParams;

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Sep 24, 2024
@abzokhattab
Copy link
Contributor

abzokhattab commented Sep 24, 2024

Edited by proposal-police: This proposal was edited at 2024-09-24 14:02:47 UTC.

Proposal

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

Add a "Download" option to the expense report details.

What is the root cause of that problem?

This is a request for a new feature.

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

  1. Add a "Download" menu item in the report details component here under the condition that the current report is an expense report. The requirement is that it should be shown for expense reports only not for all products
if (isExpenseReport) {
    items.push({
        key: CONST.REPORT_DETAILS_MENU_ITEM.DOWNLOAD,
        translationKey: 'common.download',
        icon: Expensicons.Download,
        action: () => ReportUtils.downloadReportAsCsv(report.reportID), 
        isAnonymousAction: false,
        shouldCloseModalOnSelect: true  // Optional, for consistent behavior with the search page
    });
}
  1. Update the CONST.REPORT_DETAILS_MENU_ITEM to include the new record DOWNLOAD: "download"
  2. The ReportUtils.downloadReportAsCsv function should be created. It will retrieve the report, convert the JSON object to CSV, and then initiate a download. You can refer to this example for guidance on converting JSON to CSV (or we can use existing npm packages such as jsonexport, this library has 0 dependencies and would be useful in our case).
  3. Once the data is converted, the function should call localFileDownload . We should also add an argument to accept the file extension, with a default value of .txt.

What alternative solutions did you explore? (Optional)

Alternatively, the conversion from JSON to CSV could be handled on the backend. We can expose an endpoint in the backend .. then create a function similar to exportSearchItemsToCSV , where the URL is formulated, and data is requested from the database.

@trjExpensify
Copy link
Contributor

@rlinoz I'm going to export this, but CC'ing you for vis for the internal proposal approval with context on this feature.

@trjExpensify trjExpensify added the External Added to denote the issue can be worked on by a contributor label Sep 24, 2024
Copy link

melvin-bot bot commented Sep 24, 2024

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

@melvin-bot melvin-bot bot changed the title Add the Download option to the report details screen. [$250] Add the Download option to the report details screen. Sep 24, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 24, 2024
Copy link

melvin-bot bot commented Sep 24, 2024

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

@cretadn22
Copy link
Contributor

cretadn22 commented Sep 25, 2024

Proposal

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

Add the Download option to the report details screen

What is the root cause of that problem?

New feature

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

  1. In the ReportDetailsPage, we need to add a new Download button if the current report is an expense report.
  2. We'll create a new function called downloadReportCsv that will be triggered when the user clicks the Download button (similar to the downloadCategoriesCSV function). This function will accept two parameters: reportID and onDownloadFailed, and will call the backend to retrieve the CSV file.
  3. In the ReportDetailsPage, we need to handle the scenario when onDownloadFailed is triggered by creating a modal to inform the user that the download failed (similar to what we did in Display download failure modal #49343 cc @Guccio163).

What alternative solutions did you explore? (Optional)

@cretadn22
Copy link
Contributor

@JmillsExpensify Just want to confirm if this report should have Download button

  1. This report include many child money requests. Should we display Download button on this report?
11

@nkdengineer
Copy link
Contributor

nkdengineer commented Sep 25, 2024

Edited by proposal-police: This proposal was edited at 2024-09-25 19:22:41 UTC.

Proposal

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

Add the Download option to the report details screen.

What is the root cause of that problem?

This is a new feature

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

  1. Add the download option to CONST.REPORT_DETAILS_MENU_ITEM
  2. We need to create exportReportToCSV function that is similar to exportSearchItemsToCSV function
function exportReportToCSV({reportID, transactionIDList}: ExportReportCSVParams, onDownloadFailed: () => void) {
    const finalParameters = enhanceParameters(WRITE_COMMANDS.EXPORT_REPORT_TO_CSV, {
        reportID,
        transactionIDList
    }) as Params;

    const formData = new FormData();
    Object.entries(finalParameters).forEach(([key, value]) => {
        if (Array.isArray(value)) {
            formData.append(key, value.join(','));
        } else {
            formData.append(key, String(value));
        }
    });

    fileDownload(ApiUtils.getCommandURL({command: WRITE_COMMANDS.EXPORT_REPORT_TO_CSV}), 'Expensify.csv', '', false, formData, CONST.NETWORK.METHOD.POST, onDownloadFailed);
}
  1. We should add a download option to menuItems above the export option here if the report is a money request report and we're online. OPTIONAL: We can also add isInvoiceReport and isTrackExpenseReport if we want to export the CSV from these report.
const [transactions] = useOnyx(ONYXKEYS.COLLECTION.TRANSACTION);
const transactionIDList = useMemo(() => {
    if (!isMoneyRequestReport) {
        return [];
    }
    return getAllReportTransactions(report.reportID, transactions).map((transaction) => transaction.transactionID);
}, [isMoneyRequestReport, report.reportID, transactions]);
if (isMoneyRequestReport && !isOffline) {
    items.push({
        key: CONST.REPORT_DETAILS_MENU_ITEM.DOWNLOAD,
        translationKey: 'common.download',
        icon: Expensicons.Download,
        isAnonymousAction: true,
        action: () => {
            Report.exportReportToCSV({reportID: report.reportID, transactionIDList},() => {
                // Display the download fail modal here
            })
        },
    });
}

if (policy && connectedIntegration && isPolicyAdmin && !isSingleTransactionView && isExpenseReport) {

  • The params is the require param for this API
  • The onDownloadFailed callback, we can display the download fail modal as we do here
  • We need a new API from BE to download the expense report CSV

What alternative solutions did you explore? (Optional)

NA

@NJ-2020
Copy link
Contributor

NJ-2020 commented Sep 25, 2024

Proposal Updated

1 similar comment
@NJ-2020
Copy link
Contributor

NJ-2020 commented Sep 25, 2024

Proposal Updated

@sobitneupane
Copy link
Contributor

sobitneupane commented Sep 25, 2024

@JmillsExpensify

All expense reports will contain a Download option in the details page for that report. This is the case no matter whether the report is a "one expense report" or whether it's a report that contains several "batched" expenses.

  • For reports with multiple expenses, should the Download option be available only at the report level (i.e., for the entire batch of expenses)?
  • In cases where there's only a single expense (and no separate report is created), should the Download option be available when viewing that individual expense?

Also, could you confirm the structure of the data in the CSV?

@rlinoz rlinoz self-assigned this Sep 25, 2024
@rlinoz
Copy link
Contributor

rlinoz commented Sep 25, 2024

Assigning myself as I think this will require minor backend changes

@rlinoz
Copy link
Contributor

rlinoz commented Sep 25, 2024

Example CSV:
Expensify-2024-09-25 12_37_36.117.csv

In cases where there's only a single expense (and no separate report is created), should the Download option be available when viewing that individual expense?

The way I am reading the issue, yes, per:

This is the case no matter whether the report is a "one expense report" or whether it's a report that contains several "batched" expenses.

I will defer the other one to @JmillsExpensify as I don't really know if we should have the download option at the expense level.

@trjExpensify
Copy link
Contributor

I think it's just the report level, yeah. So in the case of a "one-expense report" that can be exported to CSV in the same waya "multi-expense report" can.

@JmillsExpensify
Copy link
Author

Yes, just the report level.

@nkdengineer
Copy link
Contributor

Thanks.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 30, 2024
Copy link

melvin-bot bot commented Sep 30, 2024

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

@abzokhattab
Copy link
Contributor

The #49638 (comment) from @abzokhattab looks good to me

It looks like this task was mistakenly assigned to nkdengineer according to @sobitneupane 's review here

Could we please update the assignment so I can proceed?

@rlinoz rlinoz assigned abzokhattab and unassigned nkdengineer Sep 30, 2024
Copy link

melvin-bot bot commented Sep 30, 2024

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

@rlinoz
Copy link
Contributor

rlinoz commented Sep 30, 2024

Im sorry about that, fixed it.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Oct 3, 2024
@JmillsExpensify
Copy link
Author

Looks like the PR is awaiting a reviewer.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 22, 2024
@melvin-bot melvin-bot bot changed the title [$250] Add the Download option to the report details screen. [HOLD for payment 2024-10-29] [$250] Add the Download option to the report details screen. Oct 22, 2024
Copy link

melvin-bot bot commented Oct 22, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Oct 22, 2024
Copy link

melvin-bot bot commented Oct 22, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.51-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-10-29. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Oct 22, 2024

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@sobitneupane] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [@JmillsExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 28, 2024
@JmillsExpensify
Copy link
Author

Payment summary:

@JmillsExpensify
Copy link
Author

Contributor paid out and reviewer is paid via NewDot. Closing this out, though please make sure to complete the checklist, thanks!

Copy link

melvin-bot bot commented Oct 29, 2024

@JmillsExpensify @rlinoz Be sure to fill out the Contact List!

@sobitneupane
Copy link
Contributor

Regression Test Proposal

  1. Submit an expense report to a user
  2. Open the created expense
  3. open the report details from the header
  4. Click on Download
  5. Verify that a csv file containing the expense data is downloaded

Do we agree 👍 or 👎

@garrettmknight
Copy link
Contributor

$250 approved for @sobitneupane

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.
Projects
No open projects
Status: Polish
Development

No branches or pull requests

9 participants