onc-certification-g10-test-kit icon indicating copy to clipboard operation
onc-certification-g10-test-kit copied to clipboard

FI-3587: Migrate to authinfo

Open Jammjammjamm opened this issue 9 months ago • 1 comments
trafficstars

This branch updates all of the tests to use AuthInfo rather than OauthCredentials.

The preset has been updated and all tests should run as normal.

Jammjammjamm avatar Feb 12 '25 19:02 Jammjammjamm

User experience seems fine to me. I thought that this would show up in more places (e.g.; all launches), but obviously those tests are providing oauth credentials as outputs not inputs.

I walked through both smart v1 and smart v2. Only thing I could nit-pick, which is unrelated, is that it would be nice if default scopes (when not using presets) took into account both version of smart and version of us core. if we have to lock to a single version of us core, would v6 make sense now?

arscan avatar Feb 28 '25 21:02 arscan

User experience seems fine to me. I thought that this would show up in more places (e.g.; all launches), but obviously those tests are providing oauth credentials as outputs not inputs.

It is being used in all launches. I don't think you were using the current version of this branch.

Only thing I could nit-pick, which is unrelated, is that it would be nice if default scopes (when not using presets) took into account both version of smart and version of us core. if we have to lock to a single version of us core, would v6 make sense now?

Yeah, we can update them to use v6 as the default.

Jammjammjamm avatar Mar 03 '25 13:03 Jammjammjamm

not sure how i was working off an old copy. Might have been something like me running using produciton/all docker mode, it failed to build and i didn't notice, and it used an old version. Re-reviewing now.

arscan avatar Mar 03 '25 16:03 arscan

On 2 Limited Access Launch is there a reason why this is all lowercase (see screenshot)? Not obvious why it is doing this after a quick search of the diff.

Screenshot 2025-03-03 at 11 27 15 AM

Also, for clarity, could we change the input instructions to something like (noticed no period before "Enter", and it would be nice to explicitly say why they can't run this thing right out of the gate):

The purpose of this test is to verify that patient app users can restrict access granted to apps to a limited number of resources. Enter which resources the user will grant access to below, and during the launch process only grant access to those resources. Inferno will verify that access granted matches these expectations.

This test can only be run after the Standalone Patient App test, and all other inputs are locked to ensure the same launch configuration in both tests.

arscan avatar Mar 03 '25 16:03 arscan

We seem to mix locked / unlocked inputs more now, and its visually hard to tell them apart. Before, i think, we often dumped all locked things at the bottom (at least we did in this example's case), so this wasn't a problem. With this update I found it to be a bit of work to figure out what I am supposed to fill out.

Screenshot 2025-03-03 at 11 48 52 AM

Can we just turn the labels of locked inputs grey too? Is that a problem? It is particularly confusing to have the checkbox be all grey but not the radio buttons, for example (though, I understand why technically it is setup like that). Maybe a question for @AlyssaWang or @mrnosal

Might be good to also add a tooltip to the lock icon like 'This input is set to a fixed value or is populated by another test.'

arscan avatar Mar 03 '25 16:03 arscan

What do you think about arranging the text like this to avoid three single-sentence paragraphs?

The purpose of this test is to verify that patient app users can restrict access granted to apps to a limited number of resources. This test can only be run after the Standalone Patient App test, and all other inputs are locked to ensure the same launch configuration in both tests.

Enter which resources the user will grant access to below, and during the launch process only grant access to those resources. Inferno will verify that access granted matches these expectations.

Jammjammjamm avatar Mar 03 '25 16:03 Jammjammjamm