search

vevcom / projectNext

Project Next is Omegas new website coming soon
MIT License
6 stars 1 forks source link

Feat/registration #245

Closed theodorklauritzen closed 1 week ago

theodorklauritzen commented 3 months ago

Caution! Merge the feat/notifications #240 branch and the feat/auth folder #228 first!

theodorklauritzen commented 2 months ago

Thanks for the feedback @Paulijuz

Are you supposed to be able to enter any domain name you want into a mail alias. Would it be better to simply enter the name that should be before the @?

Some users should be able to add different domains than the current domain. Maybe we want the alias POSTMASTER@sanctus.omega.ntnu.no for example. But I agree that 99% of cases we just use omega.ntnu.no.

Should the admissions be a database table? This doesn't seem like something that is likely to change in a loooong time so I don't see the benefit of allowing it to be dynamic. Maybe a simple enum would suffice? Then you could reduce the complexity quite a bit.

I had an idea to use this more generally, like admissions to DGR. But after some thought I don't think this will be used, and then it's just extra unnecessary complexity. So I will remove this.

I'm not sure if this is the PR that did it, but the create new user form is broken.

I have some thoughts about this. I think the user form should take in the fields username, firstname, lastname and email. Then when you create a user, the user gets an email with a link that leads to the registrations page. And there they can create a password add a mobile number etc. Do you have any thoughts on this as well @JohanHjelsethStorstad ?

I think turning on a lot of notification channels is very tedious. Especially since you have to press the submit button for each channel.

I totally agree the UI should be a lot better. I was not sure how the profile page design would be. But the best is probably to put the settings on a different page.

Paulijuz commented 2 months ago

Some users should be able to add different domains than the current domain. Maybe we want the alias POSTMASTER@sanctus.omega.ntnu.no for example. But I agree that 99% of cases we just use omega.ntnu.no.

Then I understand your design choice. Ideally, I still think there should be something in the UI to indicate which domains are valid tho. Currently you either have to know which domains are valid or look through the list of used domains. Maybe you could have a drop down of all the valid domains? Though this is not really important, this page will be used by very few people (only admins) so I think how it is now is good enough. We can always improve later on.

JohanHjelsethStorstad commented 2 months ago

I agree that we should have a fixed number of domains allowed

JohanHjelsethStorstad commented 2 months ago

As for the createNewUser form it was meant as an admin-form. So it should probably take as little info as possible then make the new user give the rest

theodorklauritzen commented 2 months ago

I have fixed most of the feedback you gave. I have also created a JWT folder, with all the JWT functionality. The things I haven't fixed yet are:

Do one of you @JohanHjelsethStorstad or @Paulijuz want to give a review of the changes?

theodorklauritzen commented 1 month ago

The latest comments from the review from @Paulijuz should have been resolved by now. Hovever I have not bee enable to reproduce the bug:

If I enable all the channels with the top check button I sometimes get the error message "The methods must a subset of the available methods". I'm guessing this is because the top check button checks the disabled check buttons somehow.

Therefore it's probably not fixed, but I have changed how the page works and that can be why I'm unable to reproduce the bug.

@JohanHjelsethStorstad do you want to give me another review?

theodorklauritzen commented 1 month ago

@JohanHjelsethStorstad fixed your comments.

I'll refactor a few things in this PR while working on push notifications.