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

Automatically update the Expensify.cash desktop app when a new version is released on mac #1011

Closed
michaelhaxhiu opened this issue Dec 18, 2020 · 16 comments · Fixed by #1318
Closed
Assignees

Comments

@michaelhaxhiu
Copy link
Contributor

michaelhaxhiu commented Dec 18, 2020

If you haven’t already, check out our contributing guidelines for onboarding!


Platform - version:

  • Platform: Chat desktop app
  • Version: All versions (tested most recently on v1.0.1-271)
  • This is applicable to Mac computers only

Action Performed (reproducible steps):
Summary: the problem is that when an update is available for the desktop client, you have to manually close/reopen the desktop app for the update to take effect. This is true across all versions, and there are two potential methods to reproduce this:

Testing method - purposely install an older version of the Expensify.cash desktop app
To reproduce this method, you can purposely install an older version of the desktop app. That way, there will always be an update available when you launch the app. To purposely install an older version for this method:

  1. Checkout an older git tag from the Expensify.Cash repo. You can see the full list of release tags here.
  2. Apply the diff outlined in the description of this PR
  3. Run npm run desktop-build. That will run a local production build and save the packaged result to dist/Chat.dmg
  4. Use that .dmg file to install the desktop application.
  5. When it launches, there will be an updated version available and you'll see the desktop app does not automatically update by clicking Expensify.cash > About expensify.com

(Note: this can also be tested passively if you download the desktop app and wait a few hours, since we naturally release frequent version updates for the Expensify.cash)


Expected Result:
Automatically update the Expensify.cash desktop app when an update is released, without requiring refresh or close/reopen.

Actual Result:
When an update is released, the desktop app does not update automatically.

Notes/Photos/Videos:
N/A

Logs - JS/Android/iOS (if applicable):
N/A

@michaelhaxhiu michaelhaxhiu self-assigned this Dec 18, 2020
@michaelhaxhiu michaelhaxhiu changed the title Automatically update the Expensify.cash desktop app when a new version is released on mac [Hold] Automatically update the Expensify.cash desktop app when a new version is released on mac Dec 18, 2020
@roryabraham
Copy link
Contributor

So for a bit more context:

Right now we're utilizing an electron-updater function called autoUpdater.checkForUpdatesAndNotify to update the desktop application, which:

  1. Checks for updates, and if one is available
  2. Downloads the update
  3. Once the download is finished, notifies the user that an update is available
  4. Then, the user must manually close & reopen chat for the update to be installed.

And as a note, we're currently doing that when the app launches, then every hour after that.

What we could do instead is create some custom logic like so:

  1. When the app launches, and every hour after that, check for updates using autoUpdater.checkForUpdates
  2. If an update is available, download the update using autoUpdater.downloadUpdate
  3. Once the download is finished, we can take more control:
    • Maybe once a week or so, or only for major updates, we can notify the user that an update is available by locally triggering a BrowserNotification. In the callback for that BrowserNotification, we can:
      1. Display a confirmation modal like: "Would you like to restart Expensify.Cash to install the latest updates?", and once confirmed
      2. Restart the app with autoUpdater.quitAndInstall
      3. Note, this is basically how slack updates work
    • Alternatively, we can add listeners for if the window is minimized or closed (with the app still running), and if the app is backgrounded when an update is available, restart the app with autoUpdater.quitAndInstall - that way the user potentially won't even notice and the app will update like magic. I think this is our ideal scenario. This also mirrors how it happens on web, except on web we just refresh the page instead of quitAndInstall.
    • Some combination of the two options above???

@puneetlath
Copy link
Contributor

puneetlath commented Dec 24, 2020

This is applicable to Mac computers only

Do we not have a Windows version of our electron app?

@quinthar
Copy link

quinthar commented Dec 25, 2020 via email

@roryabraham
Copy link
Contributor

roryabraham commented Dec 28, 2020

let's create an issue for someone to create a Windows desktop
client

Created an issue here

@jboniface
Copy link

@michaelhaxhiu just wanted to check in on this!

@michaelhaxhiu michaelhaxhiu changed the title [Hold] Automatically update the Expensify.cash desktop app when a new version is released on mac Automatically update the Expensify.cash desktop app when a new version is released on mac Jan 8, 2021
@michaelhaxhiu
Copy link
Contributor Author

Posted to Upwork last week.

@michaelhaxhiu
Copy link
Contributor Author

michaelhaxhiu commented Jan 19, 2021

Going to assign to Nikhil from Upwork as soon as he responds and I add him to this GH.

@nbhargava
Copy link
Contributor

I've started to take a stab at this. Here are my thoughts/progress so far:

  • I've decided to go with the second approach to start (i.e. add listeners and fire quitAndInstall when detecting that we're in the background)
  • I'm able to reliably detect that the app has been minimized/closed/hidden and can trigger a callback on those events.
  • I'm currently having trouble actually triggering the update due to code signature validation issues, but I can validate that the download does happen correctly in the background

Next steps:

  • It seems like by pointing the autoupdater at a local minio instance, I may be able to overcome these issues, but I'm not confident on this point
  • If that doesn't work, I can put up a PR (which certainly won't cause any regressions) to see whether it's just code-sign issues holding us back. But, I recognize that may be unsatisfactory and I'm open to other thoughts.

@michaelhaxhiu
Copy link
Contributor Author

I'll review your solution proposal and get back to you on that shortly!

@nbhargava can you please send me your email address so I can add you as a guest to our #expensify-contributors Slack channel?

@roryabraham
Copy link
Contributor

@nbhargava Not 100% certain if this will resolve your code signing issues you're running into, but you might try temporarily applying these changes in config/electron.config.js:

module.exports = {
    appId: 'com.expensifyreactnative.chat',
    productName: 'Expensify.cash',
    extraMetadata: {
        main: './desktop/main.js',
    },

    // Comment out notarizing
    // afterSign: 'desktop/notarize.js',
    mac: {
        category: 'public.app-category.finance',
        icon: './android/app/src/main/res/mipmap-xxxhdpi/ic_launcher_round.png',
        hardenedRuntime: true,
        entitlements: 'desktop/entitlements.mac.plist',
        entitlementsInherit: 'desktop/entitlements.mac.plist',
        type: 'distribution'
    },
    dmg: {
        title: 'Expensify.cash',
        artifactName: 'Expensify.cash.dmg',
        internetEnabled: true
    },
    // Also comment out publishing step
    // publish: [{
    //     provider: 's3',
    //     bucket: 'chat-test-expensify-com',
    //     channel: 'latest'
    // }],
    files: [
        './dist/**/*',
        './desktop/*.js',
        './src/libs/checkForUpdates.js',
    ],
};

It's worth a shot to see if it will help you test this. Just make sure you don't commit those changes or try to include them in your PR :)

@nbhargava
Copy link
Contributor

Unfortunately I tried that and it doesn't solve the issue for me. The problem seems to be that if the local version isn't signed the same way as the downloaded version, electron-updater rejects it (though I'm not 100% that this is the source of the problem).

@michaelhaxhiu
Copy link
Contributor Author

@nbhargava Your proposal looks good from a high level, but let's continue to have check-ins as you go to ensure the approach is working. Feel free to keep asking questions!

You can get started on the PR and link it here.

@AndrewGable
Copy link
Contributor

@nbhargava - I like the idea of investigating a way to test this using mino or some other way that doesn't use production signed versions. I don't think it's realistic that we are going to be able to test this using production signed versions as we already have users using the desktop app.

@nbhargava
Copy link
Contributor

Quick update -- I have this mostly working. There are a couple of weird quirks I'm trying to work around before I post a PR. Namely, when the updated version downloads and boots, it creates a second instance of an Electron app (both of which are the correct version). I'm not exactly sure why this is happening, but once it's sorted I should be good to go.

@nbhargava
Copy link
Contributor

^ The above PR should include fixes for the mentioned issues. Let me know if there's anything else I need to do before it can be reviewed.

@michaelhaxhiu
Copy link
Contributor Author

The payment was sent today. Thanks for your contribution @nbhargava!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants