Feature/update user-info in settings #939
Conversation
| .min(2, "Navn må være minst 2 bokstaver") | ||
| .max(50, "Navn kan maks være 50 bokstaver") | ||
| .regex(/^[a-zA-ZæøåÆØÅ\s'-]+$/, "Navn kan ikke inneholde tall eller spesialtegn bortsett fra - og '") |
There was a problem hiding this comment.
I don't think we should have a limitation on names here, first of all because regex can cause issues with names from e.g. FEIDE not being valid.
We actually don't need name-validation since we now always set the name from FEIDE, so we have access to users' legal names. Accounts without membership/usernames should probably only show their emails and we are likely fine with them not being able to change their names
There was a problem hiding this comment.
Agree, I don't think we should allow self-name changing since we're also storing the names in two places. More place for error
There was a problem hiding this comment.
since we're also storing the names in two places.
What do you mean by this? The single source of truth for us should be Auth0
There was a problem hiding this comment.
I agree that this can cause issues, but what about users not registering with FEIDE? Can we force name inputs on account registration?
There was a problem hiding this comment.
Even social members will have their name set through FEIDE (and if not that is HS badly modifying data / some desperate actions related to ITEX). This is then only relevant for what are essentially guest-users, which I think has only been used for utmatrikulering/immball (?).
tl;dr: we will eventually not have "active" users where we do not have their name from FEIDE
any users who actively want to set their name to something else could be something we support, but that should be dealt with on an individual basis if so, and not something I would spend time implementing before it is a feature request.
| "clsx": "^2.0.0", | ||
| "cva": "npm:class-variance-authority@^0.7.0", | ||
| "react": "^18.2.0", | ||
| "react-dom": "^18.2.0", | ||
| "react-phone-number-input": "^3.4.5", |
There was a problem hiding this comment.
| "react-phone-number-input": "^3.4.5", |
Just use <input type="tel">? https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/tel
(I would actively not bother to have country-code validation)
There was a problem hiding this comment.
We can also get phone-numbers from FEIDE, though we might need another scope (and not sure if we get it for exchange students)
There was a problem hiding this comment.
-
If i'm a user who used FEIDE for my account creation, i would not like for the website to get my phone number and show it (this can be a toggle of course). I would rather want to be able to type it in myself, who knows, i might want to use a different phone number than the one registered at FEIDE.
-
By having the country code, not only does it look good in design, but also solves issues with possible members (exchange students) having non-norwegian phone numbers.
I would like to keep it, but that's my opinion
There was a problem hiding this comment.
also solves issues with possible members (exchange students) having non-norwegian phone numbers.
Which problems? This dependency seems quite unnecessary, if we wanted to validate that the input data is good we would actually need to send a test-sms with a some kind of code akin to 2FA, but that costs money.
If i'm a user who used FEIDE for my account creation, i would not like for the website to get my phone number and show it (this can be a toggle of course). I would rather want to be able to type it in myself, who knows, i might want to use a different phone number than the one registered at FEIDE.
I agree you might want to register your phone number as something different from FEIDE, but my issue was with adding a whole new dependency which seems unnecessary and just gives us technical debt. I think the feature is fine, just remove the extra JS
There was a problem hiding this comment.
Regardless of outcome this PR should remove the @fadi-ui/react-country-flag dependency
| .replace(/[\s,]+/g, ",") | ||
| .replace(/(^\w|,\w)/g, (match) => match.toUpperCase()) |
There was a problem hiding this comment.
Allergies has to be consistently either a single string-field or a list as presented to users, I don't think we should split it based on commas/spaces, users will just give input that does not turn well into a list
There was a problem hiding this comment.
- I'm not sure if i understand the problem? If the allergies are used as a string, then most likely all users allergies will be unique and there will be no way of using them for analytics, for example knowing how many attendees in an event are allergic to nuts.
However, it might be a suitable solution to give the user an input-field for ONE allergy, then hitting a+to get another input-field for the next allergy. - What do you mean by users will just give input that does not turn well into a list, could you give an example.? In theory, a user can always give bad inputs even if the input is a string or a list
There was a problem hiding this comment.
users allergies will be unique and there will be no way of using them for analytics, for example knowing how many attendees in an event are allergic to nuts.
You would need to perform some kind of text-search w/ synonyms. There is native support for that in e.g. postgres. Also, we don't do such kinds of analytics / statistics, this information is generally only useful on an individual level.
AFAIK allergies are dealt with on an individual level, not by performing some grouping/staking the field before ordering something for everyone
However, it might be a suitable solution to give the user an input-field for ONE allergy, then hitting a + to get another input-field for the next allergy.
Yes exactly, but from the images in the PR-description it is shown as a text-field, but stored as a list
could you give an example.?
Look at e.g. the values people have for allergies at old events like https://old.online.ntnu.no/dashboard/events/2268/, there is mix of description and extra contextual information, not just "nuts" / "peanuts" / "pork", and full sentences with weird splits.
IMO: trying to make a regex/split this from free text is a lost cause, just let it be a free text-field, and allow event organizers to sort by non-empty values just read through all of them individually. That is how it currently works, and I think most other features for this is over-engineering or complicating it without giving us any gains. A potential alternative would be that we have a specific list of allergies we can accomodate for, and not allow user-input as text, but IMO that sounds like a lot of work with
- gathering the specific allergies
- makes the user have to actively hunt for their allergies
- maintaining/updating the list
Most of our attendees do not have allergies, that might be a bias/registration problem, but I would focus on other features related to monoweb / figure out concrete issues users/organizers have with it before we do anything more
| @@ -68,6 +68,37 @@ module.exports = { | |||
| borderRadius: { | |||
| md: "4px", | |||
| }, | |||
| minHeight: { | |||
| 1: "0.25rem", | |||
Closes: #...
Checklist
Changelog
ADD: PhoneInput component
ADD: react-phone-number-input + hookresolver dependencies
ADD: types/node@* + postcss@^8.3.3 peer dependencies
ADD: submit form for updating user-info in settings
- Force format allergies field
- Show error on submitting of wrong format
NOTE: Because the session does not currently give a full user, i've populated some fields with fake data such as the id of the user. Therefore the only user currently being updated is attached to mads.barnes@hotmail.com
FIX: Small bugs in Button component and SettingsMenuItem
How to test
settingsDefault
Errors on wrong format
Success on update user
Check Auth0 and Neon for updated user