codeforpdx / PASS

PASS project - with HMIS module integration
MIT License
24 stars 24 forks source link

598 simplify add contact to use username #600

Closed DionSat closed 3 months ago

DionSat commented 3 months ago

This PR:

Resolves #598

In its current form PASS is still not very user friendly. You have to provide the whole web Id from the https to the #card to lookup the user. This is mainly for cases where the pod servers are different and URL needs to be changed to fit those pods.

This pull request replaces the WebID field with a username and OIDC provider dropdown to make the application more user friendly. The WebID can still be selected by clicking the Other dropdown for entering a custom WebID.

Screenshots (if applicable):

image

Tooltip: image

image

image

Additional Context (optional):

Add any other context about the PR here.

Future Steps/PRs Needed to Finish This Work (optional):

Issues needing discussion/feedback (optional):

DionSat commented 3 months ago

? I just ran all the tests on my side but its still failing the test check. That's weird it passes the tests it says it fails on my side. So I don't know what's going on with the checks.

DionSat commented 3 months ago

I figured it out. There is no environmental variable when GitHub actions it setting up the environment. GitHub actions sets up the dependencies and builds the project and runs the test but doesn't create an .env file. Found a similar issue with GitHub actions. Because my test relies on the .env file we need to either create secret variable in GitHub or add env: parameters to the GitHub actions. Unfortunately this would require changing the workflow file or I would have to change my branch to not use the .env file. But I think this should be addressed now because I assume in the future we'll have tests that rely on the .env file so we should address it now.

timbot1789 commented 3 months ago

I figured it out. There is no environmental variable when GitHub actions it setting up the environment. GitHub actions sets up the dependencies and builds the project and runs the test but doesn't create an .env file. Found a similar issue with GitHub actions. Because my test relies on the .env file we need to either create secret variable in GitHub or add env: parameters to the GitHub actions. Unfortunately this would require changing the workflow file or I would have to change my branch to not use the .env file. But I think this should be addressed now because I assume in the future we'll have tests that rely on the .env file so we should address it now.

In that case, you can mock the ENV in tests.

Add whatever values you need here: https://github.com/codeforpdx/PASS/blob/077756394a32d60c0b2dcde8d4472838a9ca3970/__mocks__/src/constants/index.js#L1

Then add this line after the imports in your test file:

vi.mock('@constants');

And the values will appear in the tests.

Note: This is actually why we wrap ENV as a constant variable in PASS. It lets us treat the env as an external service like a database or a network connection. Then it's more clear how to mock the ENV in tests.

DionSat commented 3 months ago

I tried using that mock of the ENV file in the manual mocks folder. But it doesn't seem to work. I tried to mock('@constants') but it didn't really populate the ENV like you would expect. Now I did find a way using this:

  const actual = await vi.importActual('../../../src/constants/');
  return {
    ...actual,
    ENV: {
      VITE_SOLID_IDENTITY_PROVIDER: 'https://www.testurl.com/',
      VITE_SUGGESTED_OIDC_OPTIONS:
        'http://testurl_1.com/, http://testurl_2.com/, http://testurl_3.com/'
    }
  };
});

This will work but another problem is that you would have to call it each time for the test file that needs the ENV. Why not just set the env value in the setupFile before each test. I found you can just call process.env.[ENV_VARIABLE] = [STRING] in the setupFile to setup the ENV variables before each test. The nice thing about the setupFile I added is that it runs before each test file. This seems a lot simpler and only requires you define the ENV variables once.

andycwilliams commented 3 months ago

Just had a thought. We may want to have a tooltip or a clickable question mark next to the OIDC Provider field that explains what it does. We could have one for each other field as well, though those seem more intuitive.

DionSat commented 3 months ago

As for the tool tip I can add that as well. I've seen a couple of those on some websites and I do think the OIDC provider dropdown should have one to make it easier to understand what we're looking for.

andycwilliams commented 3 months ago

Didn't check this until now. Is the commented out meant to stay in? (Lines 30-38 on the AddContactModal)