search

Pigmice2733 / peregrine-frontend

The future of FRC scouting
https://peregrinefrc.com/
The Unlicense
23 stars 4 forks source link

frontend page for creating a new realm #1521

Open varCepheid opened 1 year ago

varCepheid commented 1 year ago

closes #1510

varCepheid commented 1 year ago

@calebeby At line 37 in new-realm, the server gets a request to POST a new realm, even if that realm is already in the database. Is there a check that should happen in the frontend code to make sure that doesn't happen, or will the backend take care of it?

varCepheid commented 1 year ago

I don't totally understand what the test files are doing. I messed around with them a little bit, but I couldn't get the check to pass. I only changed the two under components. Can you take a look at those and either recommend something to do or fix it yourself?

calebeby commented 1 year ago

@varCepheid The test fails because the login form is broken. The test is intended to catch bugs like this, so the test is doing its job perfectly!

If you go to the preview for this PR: https://deploy-preview-1521--peregrine.netlify.app/leaderboard

Once you type a valid username and password, the "submit" button should stop being disabled, so you can click it. (That's what happens on https://dev.peregrinefrc.com/leaderboard)

I am not sure exactly what you changed in your PR that caused this behavior to break, but a good starting point would be to try undoing each of your changed files one by one until it is fixed, so that you can narrow down what caused the problem.

calebeby commented 1 year ago

@varCepheid I am guessing that this change is what's breaking it: https://github.com/Pigmice2733/peregrine-frontend/pull/1521/files#diff-7f29dc5f7895e5965f3a69c6f27d41be530b11108dba3c035c8287ec491b4cf2R22

I am not sure the intended behavior of that change, but because of the change, the ref never gets passed in to the form element, so the ref never gets set, so there is no way for the component to check whether its form is valid. The way that the code was before you made a change was correct.

calebeby commented 1 year ago

Also @varCepheid once you fix the bug, if you could undo the unrelated parts of this PR that'd be great! Smaller PR's are much easier to review and merge, and they'll make it easier to track down changes in the git history once we merge them too!

varCepheid commented 1 year ago

A lot of the changes that aren't connected to this issue are fixing errors that make the checks fail. Is it better to fix them in separate PRs and put this one on hold?

calebeby commented 1 year ago

@varCepheid The checks are passing on the dev branch, and if this branch is up to date with that branch, then the checks should be passing here too. I think at least some of the changes in this PR aren't needed to make checks pass.

calebeby commented 10 months ago

@varCepheid I have a question about how this flow works for users. If a team was to create a new realm, now that realm has no users. So in order for them to start making approved users for their realm, they'd need to make an unverified user in that realm, and still go through the manual process of contacting pigmice and getting their account verified and set to admin. Then they could go and make other users.

Is it intended to still have that manual step there or were you envisioning something different?

calebeby commented 10 months ago

@varCepheid it looks like newly-created realms have shareReports set to false, while all the previous realms have it set to true. Is that intentional?

varCepheid commented 9 months ago

The shareReports bit was unintentional and I fixed it. My solution to the no-admins problem was to make the new-realm form have all the same fields as the signup form, so it creates the first account in the same action in the new realm and sets it to be an admin.

varCepheid commented 8 months ago

I realized that making a user automatically an admin A. is a bad idea and B. gives us no oversight of new realms. I think we should just indicate that a new user in a new realm needs to join the Slack to become an admin.

calebeby commented 4 months ago

@varCepheid Good point. However, if the backend already allows it, then people still could technically do it themselves (not a big fan of "security by obscurity"). Maybe we should make the backend have a constraint on creating new admin users/realms.

varCepheid commented 4 months ago

There actually is already a backend constraint, which meant the user creation request wasn't going through, because a non-admin user (in this case not even logged in) can't create users. I think the ideal way to do this is to create only the realm and then force the would-be admin to communicate with us about creating their user. The alternative is to create a dummy super-admin and log in as that user for only the time it takes to create the new user.

varCepheid commented 4 months ago

After talking with Elijah, I realized that of course a non-user can create a user because the signup page does it. The actual problem was in the return type of the createRealm method, but that's largely unimportant. Currently, the new-realm form creates the realm with an unverified user in it and directs the user to email me and get connected to the Slack. From there, we can verify them and make them an admin. I think this system puts the least burden on us but still allows us oversight of who is running realms.