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

Prevent duplicate employments to same organisation (estimate: 6) #2374

Closed
zzgvh opened this issue Sep 20, 2016 · 7 comments
Closed

Prevent duplicate employments to same organisation (estimate: 6) #2374

zzgvh opened this issue Sep 20, 2016 · 7 comments

Comments

@zzgvh
Copy link
Contributor

zzgvh commented Sep 20, 2016

Do this search: http://rsr.akvo.org/en/admin/rsr/employment/?o=2&q=WaterNet and sort on the User column. We see that Jean-Marie is employed twice. That should not be allowed.

screenshot_132

@zzgvh zzgvh added the Bug label Sep 20, 2016
@nadiagorchakova nadiagorchakova changed the title Prevent duplicate employments to same organisation Prevent duplicate employments to same organisation (estimate: 6) Sep 29, 2016
@nadiagorchakova nadiagorchakova added this to the 3.17 Ulaanbaatar milestone Sep 29, 2016
@punchagan
Copy link
Contributor

The Employment model already has a uniqueness constraint unique_together = ('organisation', 'user', 'group'). So, I'm not sure this is really a bug. The admin interface isn't showing the Group of the user and this is a UX bug, definitely.

zzgvh added a commit that referenced this issue Oct 5, 2016
[#2389] Display group in Employment admin page. Code reviewed, looks fine. May not fix the original problem however, more discussion in #2374
@zzgvh
Copy link
Contributor Author

zzgvh commented Oct 5, 2016

Yep that's a UI/UX bug, good catch. But I think there was an original problem that was solved by removing the "duplicate" employment, meaning that the user was in two groups. So it could be we have a problem when a user is in multiple groups. I don't recall what the original issue was, do you @nadiagorchakova ?

@nadiagorchakova
Copy link
Contributor

@zzgvh @punchagan, as far as I remember, the original problem was with the Up app, here: #2364. That's where the entire discussion about multiple employments started, I think

@nadiagorchakova
Copy link
Contributor

And could this issue be relevant to the discussion? #2122. It was created way earlier but seems to address the same problem of multiple/duplicate employments

@punchagan
Copy link
Contributor

@nadiagorchakova thank you! @zzgvh #2122 does seem like the bug that is triggering this, or atleast one issue. We could either have an Unassigned group as you mentioned in the other issue, or we could just make Users be the default group. Any changes to the group can be made during approval?

@zzgvh
Copy link
Contributor Author

zzgvh commented Oct 7, 2016

I think we have to have a Unapproved group or similar, since we otherwise approve a user implicitly by assigning them to the Users group.

@punchagan
Copy link
Contributor

@zzgvh Doesn't look like it. As far as I could see, Employments have an is_approved flag, and approving is just flipping that flag.

punchagan added a commit that referenced this issue Nov 23, 2016
When an Employment with no group is created, the group is set to 'Users'
in the `post_save` method.  But, if there is already a similar
Employment for that user, the `post_save` fails because there's a
uniqueness constraint on (user, organisation, group)
punchagan added a commit that referenced this issue Nov 23, 2016
Employments are forced to have a non-null group, though the model does
allow null values, by trying to set a default group in the post_save.
But, this can fail if such an employment already exists, thus creating
an employment with a null group.  Multiple such employments with a null
group can get created, too.

This commit moves the setting of the group to the pre_save signal, thus
preventing an invalid employment from getting created.

Closes #2374, Closes #2122
punchagan added a commit that referenced this issue Nov 23, 2016
punchagan added a commit that referenced this issue Nov 23, 2016
punchagan added a commit that referenced this issue Nov 23, 2016
punchagan added a commit that referenced this issue Nov 24, 2016
When an Employment with no group is created, the group is set to 'Users'
in the `post_save` method.  But, if there is already a similar
Employment for that user, the `post_save` fails because there's a
uniqueness constraint on (user, organisation, group)
punchagan added a commit that referenced this issue Nov 24, 2016
Employments are forced to have a non-null group, though the model does
allow null values, by trying to set a default group in the post_save.
But, this can fail if such an employment already exists, thus creating
an employment with a null group.  Multiple such employments with a null
group can get created, too.

This commit moves the setting of the group to the pre_save signal, thus
preventing an invalid employment from getting created.

Closes #2374, Closes #2122
punchagan added a commit that referenced this issue Nov 24, 2016
punchagan added a commit that referenced this issue Nov 24, 2016
zzgvh added a commit that referenced this issue Nov 28, 2016
@MichaelAkvo MichaelAkvo added this to RSR Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

3 participants