[HOLD for payment 2024-08-13] [$500] Desktop - "Check for update" never actually shows option to update the Desktop app

[lanitochka17]

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

---

Version Number: 1.4.86-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): emilio.utest@gmail.com
Issue reported by: Applause - Internal Team

Action Performed:

1. Access the New Expensify Beta app
2. Sign into a valid account
3. Go to "New Expensify app" in the header > Check for Update > Wait for Update

Expected Result:

User expects the Desktop app to update and receive a message stating that the update is ready

Actual Result:

The user never receives an Update ready message and to update the user needs to access the build link from web

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6519521_1718896885781.Can_not_update_Desktop_app_.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~012dcff6435623d64e
• Upwork Job ID: 1803850854977228837
• Last Price Increase: 2024-06-28
• Automatic offers:
• dukenv0307 | Reviewer | 102945241

Issue Owner

Current Issue Owner: @mallenexpensify

[melvin-bot]

Triggered auto assignment to @neil-marcellini (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[github-actions]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[lanitochka17]

@neil-marcellini FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[neil-marcellini]

@lanitochka17 the description says that this is reproducible on staging and production, therefore it should not be a deploy blocker. Is that true? I'll try testing myself on prod but IDK if I'll be able to reproduce it in a timely manner.

[melvin-bot]

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

[melvin-bot]

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

[lanitochka17]

@neil-marcellini sorry for the confusion
Reproducible in production?: Unable to check

[neil-marcellini]

We decided not to let this be a blocker given that it's unclear if the problem exists on production or not. We're hoping that an external contributor will be able to investigate and figure out the problem. I'm going to bump the price up now given that it's not super easy to reproduce/test this one.

[melvin-bot]

Upwork job price has been updated to $500

[jp928]

Proposal

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

On the Mac OS desktop app updates from the top menu check for update not working.

What is the root cause of that problem?

In the file desktop/main.ts#L142-L153, we only check if there is a update available but not process anything with the download.

autoUpdater
        .checkForUpdates()

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

In the promise chain add function call electronUpdater.init();

What alternative solutions did you explore? (Optional)

Write a separate function to download the update and call autoUpdater.quitAndInstall();
see the offical document

[suneox]

Proposal

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

App not updating after clicking sounds good popup for update

What is the root cause of that problem?

After the user checks the update manual and confirms "sounds good" we don't handle anything

App/desktop/main.ts

Lines 155 to 161 in 4541ce2

	if (downloadPromise) {
	dialog.showMessageBox(browserWindow, {
	type: 'info',
	message: Localize.translate(preferredLocale, 'checkForUpdatesModal.available.title'),
	detail: Localize.translate(preferredLocale, 'checkForUpdatesModal.available.message', {isSilentUpdating}),
	buttons: [Localize.translate(preferredLocale, 'checkForUpdatesModal.available.soundsGood')],
	});

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

We need a UI to show download processing based on this comment or logging, we can implement by listen download-progress event to help the user know the download progress and handle show error

    dialog.showMessageBox(browserWindow, {
        type: 'info',
        message: Localize.translate(preferredLocale, 'checkForUpdatesModal.available.title'),
        detail: Localize.translate(preferredLocale, 'checkForUpdatesModal.available.message', {isSilentUpdating}),
        buttons: [Localize.translate(preferredLocale, 'checkForUpdatesModal.available.soundsGood')],
    }).then(() => {
+       autoUpdater.on(ELECTRON_EVENTS.DOWNLOAD_PROGRESS, (info) => {
+           const { percent, total, transferred, bytesPerSecond } = info;
+           browserWindow.webContents.send(ELECTRON_EVENTS.DOWNLOAD_PROGRESS, { percent, total, transferred, bytesPerSecond });
+       });
+       autoUpdater.on(ELECTRON_EVENTS.Error, (error) => {
+           dialog.showMessageBox(browserWindow, {
+               type: 'error',
+               message: 'Update Error',
+               detail: error.message,
+               buttons: [Localize.translate(preferredLocale, 'checkForUpdatesModal.notAvailable.okay')],
+           })
+       });
+   })

or listen when init

const electronUpdater = (browserWindow: BrowserWindow): PlatformSpecificUpdater => ({
    init: () => {
+       autoUpdater.on(ELECTRON_EVENTS.DOWNLOAD_PROGRESS, (info) => {
+           ....
+       });
+       autoUpdater.on(ELECTRON_EVENTS.Error, (error) => {
+           ....
+       });
+   })

What alternative solutions did you explore? (Optional)

[nkdengineer]

Proposal

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

The user never receives an Update ready message.

What is the root cause of that problem?

When the update is downloaded, we're already attempting to send the push notification to the user here

But this is fragile because the user might not have notifications enabled, or they might have notifications enabled but only allows notification in Notification Center, not allowing to show as Alert. So the user will never see it and assuming it's not working.

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

We should show a system dialog letting user know the update is downloaded. The user can choose to update or just dismiss the modal. This will not cause inconvenience to the user because they just saw the Update Available dialog a few moment earlier so it's natural to see a dialog prompting to update the app.

To enable the behavior, we can update this to

dialog.showMessageBox(browserWindow, {
    type: 'info',
    message: 'Update available',
    detail: 'A new version of this app is available!',
    buttons: ['Update later', 'Update now'],
}).then((data) => {
    if (data.response !== 1) {
        return;
    }
    quitAndInstallWithUpdate();
})

So it will look like this (ignore the icon, running locally so it's Electron icon but on prod should be Expensify icon):

We can also only do this if the update is manually triggered, so it will not show such dialog if the user did not actively request for it. Or we can do the dialog only if Notification permission is not granted/not available by using this check

What alternative solutions did you explore? (Optional)

Update here to directly call

AppUpdate.triggerUpdateAvailable();

(the action that will be done if the notification is clicked, so we skip the notification part and directly show that modal)

We can also only do this if the update is manually triggered.

[nkdengineer]

Proposal updated

[neil-marcellini]

Awesome thanks so much @s77rt. I think this is the PR for the next release

• chore(deploy): Release electron-userland/electron-builder#8377.

[muttmuure]

What's the thinking here with this going in #newdot-quality?

[s77rt]

electron-updater release is published. Raised a PR #46596 for the update

[neil-marcellini]

What's the thinking here with this going in #newdot-quality?

It creates an unreliable experience

[melvin-bot]

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

[melvin-bot]

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

• [No QA] electron-updater: Fix conflicting downloads #46596

If no regressions arise, payment will be issued on 2024-08-13. 🎊

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

• @s77rt requires payment through NewDot Manual Requests
• @dukenv0307 requires payment automatic offer (Reviewer)

[melvin-bot]

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

• [@s77rt / @dukenv0307] The PR that introduced the bug has been identified. Link to the PR:
• [@s77rt / @dukenv0307] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@s77rt / @dukenv0307] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@s77rt / @dukenv0307] Determine if we should create a regression test for this bug.
• [@s77rt / @dukenv0307] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@mallenexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[s77rt]

Checklist provided above ^ For the second bug there is no offending PR as the bug was upstream

[mallenexpensify]

@s77rt we'd still want a regression test for this though, right? I feel like this bug has occurred multiple times already, if it were to happen again, it'd be best to catch it during testing vs waiting for someone tor report.

[s77rt]

@mallenexpensify I would say this requires a regression test but it's not clear if the QA can perform the test. The QA team tests on staging and if they are on staging they can't update the app (they are already on the latest).

[mallenexpensify]

Thanks @s77rt , can you propose the steps and I'll create the TestRail case update? If QA can't test, they can comment on the issue and close if needed.

[s77rt]

@mallenexpensify

Regression Test Proposal
Prerequires: Desktop App that can be updated (i.e. current version is not the latest)

1. Open Desktop App
2. From menu bar click on New Expensify > Check for updates
3. Verify a modal is displayed titled "Update available"
4. Click on "Sounds good"
5. From menu bar verify that New Expensify > Check for updates is disabled
6. Wait while the update is downloaded (this could take a while)
7. Verify you get a desktop notification that the update is ready
8. Click on the notification
9. Verify that the App is updated (go to New Expensify > Check for updates and verify no update is available)

[mallenexpensify]

Thanks @s77rt , TestRail GH created.
https://github.com/Expensify/Expensify/issues/420213