Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Give unverified users a warning on overview page #4201

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ivarnakken
Copy link
Member

Description

There are currently 966 unverified users out of 1209 in grades 1 to 5 (including PhD). This means only ~20% have verified themselves ..

Result

image

Testing

  • I have thoroughly tested my changes.

The warning only appears if the is a current user and it is not a student (not verified). Closing the card works fine.

@ivarnakken ivarnakken added the review-needed Pull requests that need review label Oct 16, 2023
@ivarnakken ivarnakken requested a review from a team October 16, 2023 15:18
@ivarnakken ivarnakken self-assigned this Oct 16, 2023
@linear
Copy link

linear bot commented Oct 16, 2023

ABA-530 Link to student verification page from overview

I think a card similar to the photo upload card would be nice on the overview page, where it is positioned absolutely at the bottom left corner with a close button. Goes without saying, but only show the card if the user is not verified.

Copy link
Contributor

@falbru falbru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! A very useful feature!

However, this would probably be quite annoying for certain users who can't verify their account. E.g. students who have switched to indøk or students who studies something else but are going to switch to data/komtek. There's no way to manually verify students in the frontend yet, so we should probably add that as well.

@eikhr
Copy link
Member

eikhr commented Oct 16, 2023

I think the text should be changed a bit temporarily as most of the unverified students are simply because the verification system was not in place when they registered. It should inform them that we recently changed the verification system and that they should therefore re-verify, otherwise I think a lot of people will just assume it is a bug.

@ivarnakken
Copy link
Member Author

However, this would probably be quite annoying for certain users who can't verify their account. E.g. students who have switched to indøk or students who studies something else but are going to switch to data/komtek. There's no way to manually verify students in the frontend yet, so we should probably add that as well.

Yes it will be annoying, but then again, they won't be able to attend any events or basically use the site at all so I reckon they won't be regular visitors.
Besides, if they want to use the site, they must verify themselves either way or another.

@ivarnakken
Copy link
Member Author

ivarnakken commented Oct 16, 2023

I think the text should be changed a bit temporarily as most of the unverified students are simply because the verification system was not in place when they registered. It should inform them that we recently changed the verification system and that they should therefore re-verify, otherwise I think a lot of people will just assume it is a bug.

Okay. I just copied the existing text from the verification page. Should that be updated as well?
I don't really want to put too much text in the card - could it be sufficient to add it to the verification page? I suppose they won't think it's a bug when it shows up every time they visit the page, or?

@eikhr
Copy link
Member

eikhr commented Oct 16, 2023

I think the text should be changed a bit temporarily as most of the unverified students are simply because the verification system was not in place when they registered. It should inform them that we recently changed the verification system and that they should therefore re-verify, otherwise I think a lot of people will just assume it is a bug.

Okay. I just copied the existing text from the verification page. Should that be updated as well? I don't really want to put too much text in the card - could it be sufficient to add it to the verification page? I suppose they won't think it's a bug when it shows up every time they visit the page, or?

What if you just add a short sentence like "Vi har nylig endret verifiseringssystem" to the beginning of the popup?

@ivarnakken
Copy link
Member Author

What if you just add a short sentence like "Vi har nylig endret verifiseringssystem" to the beginning of the popup?

I accept

@ivarnakken ivarnakken added the approved Pull requests that have been approved label Oct 16, 2023
@falbru
Copy link
Contributor

falbru commented Oct 16, 2023

However, this would probably be quite annoying for certain users who can't verify their account. E.g. students who have switched to indøk or students who studies something else but are going to switch to data/komtek. There's no way to manually verify students in the frontend yet, so we should probably add that as well.

Yes it will be annoying, but then again, they won't be able to attend any events or basically use the site at all so I reckon they won't be regular visitors. Besides, if they want to use the site, they must verify themselves either way or another.

My point was that there are many people who now go indøk that are apart of the Abakus group (because they were verified with the previous system) who can't verify themselves through FEIDE anymore, and therefore can't remove that annoying popup even though they can attend events etc.

I might have misunderstood though. Does the isStudent property check if the user is apart of Abakus or if the student is verified through FEIDE? Because if it's the last one, my point above still applies. Also, we can't verify students in the frontend (IIRC), so if they send us an email asking for us to verify them, we have to do that through the shell. Doing that for 100+ students is a lot of manual labor. But that's separate to this PR, so I'll just create an issue about it.

@norbye
Copy link
Member

norbye commented Oct 16, 2023

but then again, they won't be able to attend any events or basically use the site at all so I reckon they won't be regular visitors.

Am I missing something here? As far as I've seen, the isStudent attribute is not used at all

I mean yes it is used for the student verification, but as soon as you have verified yourself, the only permission that is used for events are group memberships - and if you verified your user previously you will still be a member of the groups "Abakus" and whichever class you're in..
And the only place users can really see if they have the isStudent attribute set or not is if you go into your settings to see if you're a member of abakus, but nobody does that and it doesn't change any functionality or permissions.

If I'm not mistaken, it would make quite a lot of sense that so few users are validated as students (with the isStudent attribute that is), because everyone that registered this year would be verified - and everyone that verified previous years are already part of the groups so it doesn't really make a difference for them..

@norbye norbye added discussion Pull requests that needs discussions, e.g. regarding controversial/big changes do-not-merge/hold Pull request is on hold (e.g. waiting for something else to be merged), and should not be merged and removed do-not-merge/hold Pull request is on hold (e.g. waiting for something else to be merged), and should not be merged labels Oct 16, 2023
@norbye
Copy link
Member

norbye commented Oct 16, 2023

So I guess my main question is - why do we want to do this?
Especially if it has no practical implications for the individual users. If we do want to force them through the revalidation each year even if they have previously verified their student status - we should have some sort of mechanism in place to not use your membership in the class groups (1. klasse komtek, etc.), so you can't go to events that are limited to students if you're not registered as a student..

@ivarnakken ivarnakken added the do-not-merge/hold Pull request is on hold (e.g. waiting for something else to be merged), and should not be merged label Oct 17, 2023
@ivarnakken
Copy link
Member Author

ivarnakken commented Oct 26, 2023

@haukkagu @norbye

Yes you're both right. isStudent currently does nothing. However, I suppose we should add a check for this during event registration, no? If you're not actually allowed to register for events, you shouldn't be able to either.
I sort of blindly assumed this was already done based on the text given to users, but you're right, previously verified users who have been given a grade (Abakus group) are able to register for events.

Edit: As a sort of temporary, but final, solution we could only give the warning to users created after the new verification system was deployed.


This is especially counterproductive given that users must verify themselves every year. ABA-534 would give more meaning to it though.

@norbye
Copy link
Member

norbye commented Oct 26, 2023

However, I suppose we should add a check for this during event registration, no?

I agree, but maybe it should probably be optional in some way right? I would assume that some events would be open to alumni as well (genfors(?),etc.) - so I guess it's ideal if it's either linked to membership of the student groups or a checkable field in the event admin page (yayy more clutter, but at least I guess it could be applied by default)

I think the user-experience could be sorta smooth if we require isStudent to be set for memberships of subgroups of the Student group to count. That could give the same experience and usage as we have now just with the added step of validation.
Would also give event admins the freedom of still choosing the members with quite a lot flexibility. Miight be a bit messy code-wise tho, idk about that yet

This is especially counterproductive given that users must verify themselves every year. ABA-534 would give more meaning to it though.

Hmm I don't immediately see what difference this would make.. would decrease the amount of students we have to manually bump yeah, but after the initial error the years are mostly correct by bumping every summer

Copy link
Contributor

@ch0rizo ch0rizo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@danielyanghansen
Copy link
Member

This popped into my notifications. Are we re-opening this discussion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Pull requests that have been approved discussion Pull requests that needs discussions, e.g. regarding controversial/big changes do-not-merge/hold Pull request is on hold (e.g. waiting for something else to be merged), and should not be merged review-needed Pull requests that need review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants