search

freeCodeCamp / publish

> Content backend platform for /news
https://publish.freecodecamp.org
BSD 3-Clause "New" or "Revised" License
10 stars 9 forks source link

test: improve and cleanup invitation tests #368

Closed ojeytonwilliams closed 7 months ago

ojeytonwilliams commented 7 months ago

Checklist:

As I mentioned in https://github.com/freeCodeCamp/publish/pull/359, there were some improvements I wanted to make. This PR is my attempt!

I refactored the users page tests to use a Page Object Model. This keeps some of the implementation details encapsulated and the tests themselves can be very terse. In principle we can use fixtures to further simplify the setup/teardown, but I feel like our tests are simple enough already that we don't need that abstraction (not yet, anyway).

Finally, I dropped the api requests (where possible) so that the tests behave more like users. Only one api call is necessary during the testing - it sets the provider to local, so that we can test signing in. Aside from that, the cleanup removes the new user via api call (simply because that's faster than via the UI)

sidemt commented 7 months ago

Thank you for this improvement, @ojeytonwilliams! I think it tests usecases more realistic than the previous test. LGTM 👍

(I'm just leaving notes of what I understood for future reference:) At first, I thought this was another operation we shouldn't allow through API for security reasons:

Only one api call is necessary during the testing - it sets the provider to local, so that we can test signing in.

But we have the check of process.env.EMAIL_PASSWORD_AUTHENTICATION === "true" in the auth flow so it should prevent the email/password login in production. So I think it's okay to use this for tests.

ojeytonwilliams commented 7 months ago

But we have the check of process.env.EMAIL_PASSWORD_AUTHENTICATION === "true" in the auth flow so it should prevent the email/password login in production. So I think it's okay to use this for tests.

Yep, that's correct. We should also disable the email provider in the strapi backend, just to be sure.

At first, I thought this was another operation we shouldn't allow through API for security reasons:

Only one api call is necessary during the testing - it sets the provider to local, so that we can test signing in.

We can visit this closer to launch, but the main thing we need to worry about is what non-Editors can do. Editors do need to be able to do some quite destructive things (delete users, for example) and there's not a lot we can do to prevent that.