Added Paratext Registration Info form

[tjcouch-sil]

Also reformatted JSON RPC message errors so localized strings can come through and be displayed without any issues

I considered making the new service a data provider instead of just two commands... Would be interested in feedback. Kinda think it would be helpful to listen for updates to that data... But this is kinda a stopgap till we design and implement identity better. 🤔

Resolves #971

---

This change is 

[lyonsil]

Reviewed 62 of 62 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @tjcouch-sil)

---

c-sharp/JsonUtils/RegistrationDataConverter.cs line 26 at r1 (raw file):

        string? lastPropertyName = null;
        while (reader.Read())

Take a look at the other two JsonConverter classes we have. Both are now using the onObjectLevel counter to track if we've gotten to the end of the JSON object being deserialized. I found cases where JSON serialization fails if we don't include that logic because the reader goes past the end of this object and into the next object. It's only a few lines of code but is important.

---

c-sharp/Users/ParatextRegistrationService.cs line 48 at r1 (raw file):

    #endregion

    #region Protected properties and methods

protected seems like an odd choice here. Why not private? You aren't subclassing this anywhere, but making this protected implies you're intending to do so. It isn't a big deal, just wondering. I think we've done it in some other C# code when we subclass inside of tests.

---

c-sharp/Users/ParatextRegistrationService.cs line 157 at r1 (raw file):

                && RegistrationInfo.DefaultUser.Name != newRegistrationData.Name
            )
                try

While what you have works, the convention I'm used to with if statements is that if you have more than 1 line in the block that is inside the if clause, then you wrap the whole thing in curly braces. I think the point is that if you add something to the end of the block, it might not be obvious that the new line isn't part of the if statement anymore if it's too long.

I can't promise that I've always followed this, but usually if I have try/catch I think I do.

---

c-sharp/Users/ParatextRegistrationService.cs line 176 at r1 (raw file):

            // No need to observe this task in any way. We are scheduling a call to restart the application then
            // returning from this method to continue execution and properly return from this method.
#pragma warning disable VSTHRD110 // Observe result of async calls

If you want to get rid of this warning disable/enable, just prepend Task.Delay with _ = . That is:

_ = Task.Delay(...)

It looks a little cleaner and is a clear indication to the compiler that you purposefully aren't doing anything with the returned Task.

---

c-sharp/Users/ParatextRegistrationService.cs line 216 at r1 (raw file):

    /// preparation for switching to a different Paratext user.
    ///
    /// Adapted from `UserSwitchingHelper.PrepareForUserChange

Minor nit - I think you intended to put another back tick mark at the end of this line

---

extensions/src/paratext-registration/src/paratext-registration.web-view.tsx line 134 at r1 (raw file):

    isLoadingCurrentRegistrationData ||
    saveState === SaveState.IsSaving ||
    saveState === SaveState.HasSaved;

What if someone realizes they saved the wrong thing? Should it really be disabled still once the save is complete?

---

extensions/src/paratext-registration/src/paratext-registration.web-view.tsx line 141 at r1 (raw file):

    currentRegistrationData.email !== email ||
    currentRegistrationData.supporterName !== supporter;
  // whether the code seems valid according to a quick check

Is it worth having a simple regex check for email addresses, too?

---

extensions/src/paratext-registration/src/paratext-registration.web-view.tsx line 172 at r1 (raw file):

          </Grid>
        </Section>
        {/* UX said to remove supporter info until we are using it in P10S. Leaving here for uncommenting when the time is right */}

Isn't the supporter info required to authenticate? If it is, then doesn't disabling it mean you can't enter your full credentials to authenticate to the registry?

---

extensions/src/paratext-registration/src/types/paratext-registration.d.ts line 31 at r1 (raw file):

    'paratextRegistration.showParatextRegistration': () => Promise<string | undefined>;
    /**
     * Gets information about user's current Paratext Registry user information in

Is it important to call out that the registration code isn't actually returned?

---

src/main/main.ts line 261 at r1 (raw file):

      app.quit();
    },
    'platform.restart': async () => {

I'm a little surprised this works reliably.

[tjcouch-sil]

Reviewable status: 58 of 62 files reviewed, 4 unresolved discussions (waiting on @lyonsil)

---

c-sharp/JsonUtils/RegistrationDataConverter.cs line 26 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Take a look at the other two JsonConverter classes we have. Both are now using the onObjectLevel counter to track if we've gotten to the end of the JSON object being deserialized. I found cases where JSON serialization fails if we don't include that logic because the reader goes past the end of this object and into the next object. It's only a few lines of code but is important.

Done - weird :)

---

c-sharp/Users/ParatextRegistrationService.cs line 48 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

protected seems like an odd choice here. Why not private? You aren't subclassing this anywhere, but making this protected implies you're intending to do so. It isn't a big deal, just wondering. I think we've done it in some other C# code when we subclass inside of tests.

Was just copying things over from the S/R service... 😬 *whistles* changed

---

c-sharp/Users/ParatextRegistrationService.cs line 157 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

While what you have works, the convention I'm used to with if statements is that if you have more than 1 line in the block that is inside the if clause, then you wrap the whole thing in curly braces. I think the point is that if you add something to the end of the block, it might not be obvious that the new line isn't part of the if statement anymore if it's too long.

I can't promise that I've always followed this, but usually if I have try/catch I think I do.

Done

---

c-sharp/Users/ParatextRegistrationService.cs line 176 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

If you want to get rid of this warning disable/enable, just prepend Task.Delay with _ = . That is:

_ = Task.Delay(...)

It looks a little cleaner and is a clear indication to the compiler that you purposefully aren't doing anything with the returned Task.

Oops, forgot to come back to this and change to use ThreadingUtils.RunTask. Fixed

---

c-sharp/Users/ParatextRegistrationService.cs line 216 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Minor nit - I think you intended to put another back tick mark at the end of this line

Done - thanks!

---

extensions/src/paratext-registration/src/paratext-registration.web-view.tsx line 134 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

What if someone realizes they saved the wrong thing? Should it really be disabled still once the save is complete?

Once the C# side completes, it sets up a restart for shortly after submission. So this web view will be back up and unlocked for additional changes once the application restarts. Think that's all right?

---

extensions/src/paratext-registration/src/paratext-registration.web-view.tsx line 141 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Is it worth having a simple regex check for email addresses, too?

Done :)

---

extensions/src/paratext-registration/src/paratext-registration.web-view.tsx line 172 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Isn't the supporter info required to authenticate? If it is, then doesn't disabling it mean you can't enter your full credentials to authenticate to the registry?

Supporter info is optional and isn't needed for authenticating. I have never had it filled out even from starting here years ago. According to Sebastian, it's just a helper field to coordinate between a user and some primary supporter in Paratext 9. UX suggested removing the field. I figured we might want to add it back in, so I left the code commented. Feel free to discuss with UX, and maybe we can add it back in the follow-up if that's the decision. Seems reasonable to me to have it.

---

extensions/src/paratext-registration/src/types/paratext-registration.d.ts line 31 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Is it important to call out that the registration code isn't actually returned?

Good idea. Done.

---

src/main/main.ts line 261 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I'm a little surprised this works reliably.

Why? What would you expect would be better? I just put what Electron said to do to restart. I am definitely open to adjustment.

[lyonsil]

I wouldn't suggest we hold releasing due to the restart discussion. I would just suggest we keep this in mind and maybe have Roopa stress test it a bit if possible.

I don't think we need to make the new service a data provider. I mean I guess we could, but it feels overly complicated for something that 1) is still pretty early in our implementation (i.e., identity concepts) and 2) is data that really ought to change VERY infrequently as-is (i.e., how often are people going to change their PT registration details?).

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

---

c-sharp/Users/ParatextRegistrationService.cs line 48 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Was just copying things over from the S/R service... 😬 *whistles* changed

No worries

---

c-sharp/Users/ParatextRegistrationService.cs line 157 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Done

Thanks

---

extensions/src/paratext-registration/src/paratext-registration.web-view.tsx line 134 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Once the C# side completes, it sets up a restart for shortly after submission. So this web view will be back up and unlocked for additional changes once the application restarts. Think that's all right?

Got it - makes sense

---

extensions/src/paratext-registration/src/paratext-registration.web-view.tsx line 141 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Done :)

thanks

---

extensions/src/paratext-registration/src/paratext-registration.web-view.tsx line 172 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Supporter info is optional and isn't needed for authenticating. I have never had it filled out even from starting here years ago. According to Sebastian, it's just a helper field to coordinate between a user and some primary supporter in Paratext 9. UX suggested removing the field. I figured we might want to add it back in, so I left the code commented. Feel free to discuss with UX, and maybe we can add it back in the follow-up if that's the decision. Seems reasonable to me to have it.

Ok, thanks for confirming

---

src/main/main.ts line 261 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Why? What would you expect would be better? I just put what Electron said to do to restart. I am definitely open to adjustment.

This kind of thing (i.e., an application triggering its own restart) has almost always been flaky in my experience. Maybe relaunch has a solid approach. There are various race conditions that I've seen hit with programmatic restarting in general, such as:

• If your process has a start-up mutex of some kind to enforce you can't run more than one instance at a time, how can you ensure the old process gives up the mutex before the new one tries to take it?
• If your old process is slow to die for some reason, does the OS kill it before it actually launches the new one?
• If your process doesn't have something like a mutex but does hold system-level resources (e.g., holds a port open, holds a file open for read/write, etc.), what happens if the new process tries to grab the resource before the old process gives it up?
• The OS will hold onto ports after a process dies based on the TIME_WAIT settings in the TCP stack. If you don't use the SO_REUSEADDR option then a new process can't bind to the port immediately. Usually SO_REUSEADDR is fine to use, but if it isn't for some reason then restarting could be a problem.
• The OS might be slow to flush data from in memory caches (e.g., files, DBs, etc.) tied to the old process compared to when the new process tries to read from shared data locations. How can you tell for certain that the old process wrote everything in time for the new process to see it?

I'm sure there are other cases, but these are ones I've run into in the past. I think the only semi-foolproof approach has been to run some "sentinel" process whose entire purpose is to start/stop/restart the "real" process. The sentinel manages ensure the old process is closed and all critical resources are ready before starting the new process. Then you still have to worry about the sentinel dying or not killing itself when you want, but usually it's small and a lot less complex.

I'm not suggesting we need a sentinel here. I just would expect a "naive" restart API that just reruns your application's command line to start up a new instance to run into some of the race conditions (sometimes) that I mentioned above.

[tjcouch-sil]

Reviewable status: 60 of 65 files reviewed, all discussions resolved (waiting on @lyonsil)

---

src/main/main.ts line 261 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

This kind of thing (i.e., an application triggering its own restart) has almost always been flaky in my experience. Maybe relaunch has a solid approach. There are various race conditions that I've seen hit with programmatic restarting in general, such as:

• If your process has a start-up mutex of some kind to enforce you can't run more than one instance at a time, how can you ensure the old process gives up the mutex before the new one tries to take it?
• If your old process is slow to die for some reason, does the OS kill it before it actually launches the new one?
• If your process doesn't have something like a mutex but does hold system-level resources (e.g., holds a port open, holds a file open for read/write, etc.), what happens if the new process tries to grab the resource before the old process gives it up?
• The OS will hold onto ports after a process dies based on the TIME_WAIT settings in the TCP stack. If you don't use the SO_REUSEADDR option then a new process can't bind to the port immediately. Usually SO_REUSEADDR is fine to use, but if it isn't for some reason then restarting could be a problem.
• The OS might be slow to flush data from in memory caches (e.g., files, DBs, etc.) tied to the old process compared to when the new process tries to read from shared data locations. How can you tell for certain that the old process wrote everything in time for the new process to see it?

I'm sure there are other cases, but these are ones I've run into in the past. I think the only semi-foolproof approach has been to run some "sentinel" process whose entire purpose is to start/stop/restart the "real" process. The sentinel manages ensure the old process is closed and all critical resources are ready before starting the new process. Then you still have to worry about the sentinel dying or not killing itself when you want, but usually it's small and a lot less complex.

I'm not suggesting we need a sentinel here. I just would expect a "naive" restart API that just reruns your application's command line to start up a new instance to run into some of the race conditions (sometimes) that I mentioned above.

As discussed, let's see if we need to make anything more sophisticated in the future :)

[lyonsil]

Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil)

---

src/main/services/rpc-server.ts line 103 at r3 (raw file):

    requestParams: RequestParams,
  ): Promise<JSONRPCResponse> {
    const requestId = this.createNextRequestId();

These top two lines need to be part of the retry. Otherwise, we could send multiple requests with the same ID which isn't allowed by the JSON-RPC protocol. I think I saw one of the JSON-RPC stacks (.NET or TS, don't remember which) just repeated the same response that had previously been sent when I accidentally reused a request ID during early development.

[lyonsil]

Looks great! Thanks for making the adjustments.

Reviewed all commit messages.
Reviewable status: 64 of 65 files reviewed, all discussions resolved

[lyonsil]

Reviewed 1 of 1 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved