rename `source` to `logInActionSource`
ClosedPublic
Actions

Authored by kamil on Nov 2 2022, 5:37 AM.

Details

Reviewers

tomek
atul
ashoat

Commits

rCOMM36084420e00c: rename `source` to `logInActionSource`

Summary

Rename source (connected with session context) to logInActionSource to avoid redacting it from reports.

_task: ENG-2131

Test Plan

Typecheck
Check if logInActionSource is properly passed to keyserver
Trrigger one of logInActionSource actions and trigger app crash in dev tools, check if logInActionSource is present in the action payload in the latest crash report from the database

example:

{
  "type": "SET_NEW_SESSION",
  "payload": {
    "sessionChange": {
      "cookieInvalidated": false,
      "currentUserInfo": {
        "id": "[redacted-3087747816]",
        "username": "[redacted-642983437]"
      }
    },
    "preRequestUserState": null,
    "logInActionSource": "SOCKET_AUTH_ERROR_RESOLUTION_ATTEMPT"
  }
},

Check if existing clients can log in (as described here: https://phab.comm.dev/D5517#163191)

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kamil created this revision.Nov 2 2022, 5:37 AM

kamil held this revision as a draft.

Herald added subscribers: • abosh, atul, ashoat. · View Herald TranscriptNov 2 2022, 5:37 AM

kamil edited the summary of this revision. (Show Details)Nov 2 2022, 5:42 AM

kamil edited the test plan for this revision. (Show Details)

kamil added a reviewer: atul.

kamil published this revision for review.Nov 2 2022, 5:48 AM

kamil edited the test plan for this revision. (Show Details)

Harbormaster completed remote builds in B13126: Diff 18019.Nov 2 2022, 5:51 AM

ashoat requested changes to this revision.Nov 2 2022, 5:54 AM

ashoat added inline comments.

keyserver/src/responders/user-responders.js
193 ↗	(On Diff #18019)	This change will cause literally all existing clients to fail to log in!!
193 ↗	(On Diff #18019)	Can we rename `loginActionSources` to `logInActionSources` for consistency? (capitalization change)
lib/actions/user-actions.js
121 ↗	(On Diff #18019)	We should not change the name of the parameter passed to `keyserver`. We could do this and make `keyserver` look at either `source` or `logInActionSource`, but that seems complicated and unnecessary

This revision now requires changes to proceed.Nov 2 2022, 5:54 AM

fix capitalization, restore source name on keyserver

kamil added inline comments.Nov 2 2022, 7:11 AM

keyserver/src/responders/user-responders.js
193 ↗	(On Diff #18019)	ah right, I missed the `{ strict: true }` mode, thanks for pointing

Harbormaster completed remote builds in B13127: Diff 18020.Nov 2 2022, 7:24 AM

Looks good to me!!

@kamil, can you modify the Test Plan before landing to make sure you're testing existing clients? To do this, I usually checkout master and then build a Release version of the app (so that it won't reload when I change the files in Git), and then checkout my version of the code and run the keyserver with that.

Then I configure the Release build to connect to my local dev server. To do this, you may need to modify some configs:

You'll need to configure the Release build to connect to the local dev server.
- On the iOS simulator, if you've previously deployed a Debug build to the same device, redux-persist should persist the urlPrefix (URL to dev server), so you shouldn't have to do anything
- If you haven't previously deployed a Debug build, then you might have do some more work. Read here for some details
On iOS, you'll need to update native/ios/Comm/Info.release.plist.
- NSAppTransportSecurity should look like it does in native/ios/Comm/Info.debug.plist
On Android, you'll need to update native/android/app/src/release/res/xml/network_security_config.xml.
- It should look like native/android/app/src/debug/res/xml/network_security_config.xml

(Note – no need to test multiple environments... you can just test it once, on iOS or Android.)

Accepting pending test plan update

This revision is now accepted and ready to land.Nov 2 2022, 8:36 AM

@ashoat thank you for a comprehensive explanation - test done.

In D5517#163191, @ashoat wrote:

Looks good to me!!

@kamil, can you modify the Test Plan before landing to make sure you're testing existing clients? To do this, I usually checkout master and then build a Release version of the app (so that it won't reload when I change the files in Git), and then checkout my version of the code and run the keyserver with that.

Then I configure the Release build to connect to my local dev server. To do this, you may need to modify some configs:

You'll need to configure the Release build to connect to the local dev server.

On the iOS simulator, if you've previously deployed a Debug build to the same device, redux-persist should persist the urlPrefix (URL to dev server), so you shouldn't have to do anything

If you haven't previously deployed a Debug build, then you might have do some more work. Read here for some details

On iOS, you'll need to update native/ios/Comm/Info.release.plist.

NSAppTransportSecurity should look like it does in native/ios/Comm/Info.debug.plist

On Android, you'll need to update native/android/app/src/release/res/xml/network_security_config.xml.

It should look like native/android/app/src/debug/res/xml/network_security_config.xml

(Note – no need to test multiple environments... you can just test it once, on iOS or Android.)

It seems like having this process described on Notion might be beneficial. @kamil can you take care of it?

It seems like having this process described on Notion might be beneficial. @kamil can you take care of it?

Done: https://www.notion.so/commapp/Testing-existing-clients-934ee4f593584d35863e53924fed5414 (currently in Dev Notes -> Unsorted, not sure if there is a better place for this).

Closed by commit rCOMM36084420e00c: rename `source` to `logInActionSource` (authored by kamil). · Explain WhyNov 3 2022, 1:11 PM

This revision was automatically updated to reflect the committed changes.

kamil added a commit: rCOMM36084420e00c: rename `source` to `logInActionSource`.

In D5517#163686, @kamil wrote:

It seems like having this process described on Notion might be beneficial. @kamil can you take care of it?

Done: https://www.notion.so/commapp/Testing-existing-clients-934ee4f593584d35863e53924fed5414 (currently in Dev Notes -> Unsorted, not sure if there is a better place for this).

That's ok, thanks!