Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.48k stars 2.83k forks source link

[HOLD for Payment [2024-05-16][$250] Feature Request: Desktop app: Clicking on `update` button shouldn't lead to re download it #39526

Closed m-natarajan closed 5 months ago

m-natarajan commented 6 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Problem:

on the Mac Desktop app, have this "update" button update the current app directly, instead of leading you to re-download it.

Solution:

User should be able to update without downloading again

Context/Examples/Screenshots/Notes:

Screenshot 2024-04-02 at 12 52 04β€―PM

Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @puneetlath Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1712096487518269

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01c264c5555e17f971
  • Upwork Job ID: 1777186397013553152
  • Last Price Increase: 2024-04-08
  • Automatic offers:
    • DylanDylann | Reviewer | 0
    • gijoe0295 | Contributor | 0
melvin-bot[bot] commented 6 months ago

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

melvin-bot[bot] commented 6 months ago

:warning: It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time :warning:

gijoe0295 commented 6 months ago

Proposal

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

Feature Request: Desktop app: Clicking on update button shouldn't lead to re download it

What is the root cause of that problem?

Currently we only open desktop app download link:

https://github.com/Expensify/App/blob/207385ad09ec9228e55a04f62654b47f57031548/src/libs/actions/AppUpdate/updateApp/index.desktop.ts#L5

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

We should replace the above by:

https://github.com/Expensify/App/blob/67aadf26bd90a80fe597355a3238ea5094b9654c/src/components/UpdateAppModal/index.desktop.tsx#L9

We already handle the update event in electron:

https://github.com/Expensify/App/blob/d22957d292661ef10ea539f49725c388b3237dc9/desktop/main.ts#L211

By this, the app would restart to install the update automatically.

We also need to update the prompt text accordingly.

What alternative solutions did you explore? (Optional)

NA

Christinadobrzyn commented 6 months ago

I think this can be external

melvin-bot[bot] commented 6 months ago

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

melvin-bot[bot] commented 6 months ago

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

melvin-bot[bot] commented 6 months ago

πŸ“£ @ordinall! πŸ“£ Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Screen Shot 2022-11-16 at 4 42 54 PM Format:
    Contributor details
    Your Expensify account email: <REPLACE EMAIL HERE>
    Upwork Profile Link: <REPLACE LINK HERE>
ordinall commented 6 months ago

Contributor details Your Expensify account email: abdullah4abidansari@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~01e2212b454027f46b

melvin-bot[bot] commented 6 months ago

βœ… Contributor details stored successfully. Thank you for contributing to Expensify!

DylanDylann commented 6 months ago

@gijoe0295 Your proposal looks promising, but how you can verify that it works well? When testing your proposal, I see that quitAndInstallWithUpdate callback isn't triggered when we click Update button

gijoe0295 commented 6 months ago

Updated proposal

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

Feature Request: Desktop app: Clicking on update button shouldn't lead to re download it

What is the root cause of that problem?

Currently we only open desktop app download link:

https://github.com/Expensify/App/blob/207385ad09ec9228e55a04f62654b47f57031548/src/libs/actions/AppUpdate/updateApp/index.desktop.ts#L5

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

electron-updater's autoUpdater works like this:

We need to create a custom event for this and handle it appropriately following the instruction:

https://github.com/Expensify/App/blob/d22957d292661ef10ea539f49725c388b3237dc9/desktop/README.md?plain=1#L39-L44

  1. Create a new channel/event in contextBridge, for example MANUALLY_CHECK_FOR_UPDATE.
  2. Trigger the event in updateApp/index.desktop.ts:
window.electron.send(ELECTRON_EVENTS.MANUALLY_CHECK_FOR_UPDATE);
  1. Modify manuallyCheckForUpdates a bit because it was initially used as a callback for toolbar's menu item. We can handle this in the PR. Then bind it to the MANUALLY_CHECK_FOR_UPDATE event in desktop/main.ts:

https://github.com/Expensify/App/blob/d22957d292661ef10ea539f49725c388b3237dc9/desktop/main.ts#L131

ipcMain.on(ELECTRON_EVENTS.MANUALLY_CHECK_FOR_UPDATE, () => {
    manuallyCheckForUpdates(undefined, browserWindow);
});
  1. As explained above, the UPDATE_DOWNLOADED event handler had a logic to notify user when downloading is done here, that requires user interaction to actually install the update, but I don't think we need it in this case because user already pressed Update in the modal.
    • Create a variable to indicate whether we want silently update isSilentUpdate and set it to true in MANUALLY_CHECK_FOR_UPDATE handler.
    • Then modify the UPDATE_DOWNLOADED event handler to call quitAndInstallWithUpdate immediately if isSilentUpdate.

By this, when user press Update in the modal, we check if there's available update and show a dialog to user, then download and install the update silently.

We also need to update the prompt text accordingly.

What alternative solutions did you explore? (Optional)

NA

gijoe0295 commented 6 months ago

@DylanDylann I updated my proposal. We can follow this instruction to test this functionality locally but that's quite complicated and we could do it in implementation phase. For now we can hard-code some places to make it work.

DylanDylann commented 6 months ago

@gijoe0295 After testing I see that ipcMain.on(ELECTRON_EVENTS.MANUALLY_CHECK_FOR_UPDATE, ....) isn't triggered after we call window.electron.send(ELECTRON_EVENTS.MANUALLY_CHECK_FOR_UPDATE);

gijoe0295 commented 6 months ago

@DylanDylann Note that console.log does not work in Electron runtime. You can use dialog.showMessageBox instead.

https://github.com/Expensify/App/blob/468ee2f66b84dad74d6ab2cc6ec71ad2d9bd629d/desktop/main.ts#L148

DylanDylann commented 6 months ago

@gijoe0295 Ohh, cool. It works for me.

DylanDylann commented 6 months ago

@gijoe0295's proposal looks good to me

πŸŽ€ πŸ‘€ πŸŽ€ C+ reviewed

melvin-bot[bot] commented 6 months ago

Triggered auto assignment to @Beamanator, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

melvin-bot[bot] commented 6 months ago

πŸ“£ @DylanDylann πŸŽ‰ 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[bot] commented 6 months ago

πŸ“£ @gijoe0295 πŸŽ‰ 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 πŸ“–

DylanDylann commented 6 months ago

@gijoe0295 Any update here?

DylanDylann commented 6 months ago

@gijoe0295 Please prioritize this PR instead of proposing new issues

gijoe0295 commented 6 months ago

@DylanDylann I do not propose any new πŸ˜„ I'm actively working on it as you could see in PR commit history. The code change and testing are good on my side but I'm looking for a way for you and QA team to test easily. I need to purchase a paid Apple Developer account for that.

DylanDylann commented 6 months ago

You proposed 2 hours ago https://github.com/Expensify/App/issues/40316#issuecomment-2059982364

Christinadobrzyn commented 5 months ago

Hi @DylanDylann could you provide an updated when you have a moment! Thank you!

DylanDylann commented 5 months ago

Waiting for reviewing from @Beamanator

Beamanator commented 5 months ago

sorry for the delay, just merged!

Christinadobrzyn commented 5 months ago

monitoring PR - https://github.com/Expensify/App/pull/40253

Christinadobrzyn commented 5 months ago

getting ready for payment...

Payouts due:

Upwork job is here.

Do we need a regression test for this?

gijoe0295 commented 5 months ago

@DylanDylann Could you check ^?

DylanDylann commented 5 months ago

Regression Test Proposal

Precondition: The desktop app's outdated version and there is new update version

  1. Open App
  2. Press Update
  3. Verify a dialog shows Update Available
  4. Press Sounds good
  5. Verify after a while, app is restarted
  6. Press New Expensify menu >> About New Expensify
  7. Verify the version is the latest production build

Do we agree πŸ‘ or πŸ‘Ž

Christinadobrzyn commented 5 months ago

Created regression test - https://github.com/Expensify/Expensify/issues/396639

Paid out based on this payment summary - https://github.com/Expensify/App/issues/39526#issuecomment-2109656088

I think we're good to close!

DylanDylann commented 2 months ago

@Christinadobrzyn Apology for catching up late here, this issue was created from April 3 (prior to the base price change), as such according to this announcement, it should remain as $500. So this payment summary needs adjustment.

Could you help to handle it (maybe as a bonus to the existing job), thanks!

Christinadobrzyn commented 2 months ago

HI @DylanDylann so sorry I didn't see this comment.

Thanks, @gijoe0295, for reaching out! I just added a $250 bonus to each of the Upwork job payments to account for the $500 price. Let me know if there's anything else!

DylanDylann commented 2 months ago

@Christinadobrzyn No problem at all. Thanks for stepping in!