Skip to content

Conversation

@elisescu
Copy link
Contributor

@elisescu elisescu commented Nov 27, 2024

This adds support for registering users to the different campaigns we might have, and at the same time add them to a proejct as well, if needed. The registration collects information about the user which can be different from the campaign to campaign. That information is very unlikely to be used on actual site features, hence it was added it as a JSON: flexible and unlikely to cause issues with queries (either because of complexity or performance).

users/models.py Outdated
Comment on lines 132 to 133
def __str__(self):
return f"{self.user.username}({self.key})"
Copy link
Contributor

Choose a reason for hiding this comment

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

optional:
make the string include the Model type unless the keys always make it super obvious

Copy link
Contributor

@lsabor lsabor left a comment

Choose a reason for hiding this comment

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

I looked over the front end and didn't see anything glaring, though not my area of expertise.

Back end looks good to me.

Copy link
Contributor

@ncarazon ncarazon left a comment

Choose a reason for hiding this comment

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

Left a comment to double-check. Though, I assume you've tested and redirect actually works fine, so I'm approving

</>
);
} else if (user) {
redirect("register");
Copy link
Contributor

Choose a reason for hiding this comment

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

Double-checking, is this a valid redirect? As far as I can see, we on't have /register route

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested and works, it's a relative path

Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies, I see the register route now. I must have missed it during my previous review for some reason.

users/models.py Outdated
key = models.CharField(
max_length=200,
blank=True,
null=True,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use null=False for required Char values to avoid headache with null vs empty string states

Comment on lines 145 to 150
ProjectUserPermission.objects.create(
user=user, project=project, permission=ObjectPermission.FORECASTER
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have any checks whether user is allowed to join given project? Otherwise, you can specify id of a private project and enroll yourself into

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I will limit this to non-private projects

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please also check signup_api_view as well? It also has add_to_project

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am using that too, but this is for users that already have an account and they need to register to a campaign

@elisescu elisescu force-pushed the userRegistrationCampaigns branch from 78d2fa9 to a10db73 Compare November 28, 2024 13:35
@elisescu elisescu merged commit f35c04a into main Nov 28, 2024
2 checks passed
@elisescu elisescu deleted the userRegistrationCampaigns branch November 28, 2024 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants