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

Need a validation on Identifier assignment screen to avoid duplicate identifiers getting assigned to multiple users #1022

Closed
nupoorkhandelwal opened this issue Aug 10, 2023 · 8 comments
Assignees
Labels
bug Something isn't working user reported

Comments

@nupoorkhandelwal
Copy link
Contributor

nupoorkhandelwal commented Aug 10, 2023

We need a validation on the Identifier assignment screen in order to avoid any duplicate or overriding range of identifiers getting assigned to multiple users.

Steps to reproduce-

  1. Create an Identifier source
  2. Now try to assign a range say 100-200 to a user
  3. Now try assigning the same range of identifiers to another user
  4. See, the app allows to do this and in turn it causes discrepancies in the identifier_assignment table.
@nupoorkhandelwal nupoorkhandelwal added the enhancement New feature or request label Aug 10, 2023
@nupoorkhandelwal nupoorkhandelwal added bug Something isn't working and removed enhancement New feature or request labels Aug 22, 2023
@petmongrels petmongrels self-assigned this Aug 23, 2023
@petmongrels
Copy link
Contributor

Is this applicable to User based identifier generation as well?
@vinayvenu

@petmongrels
Copy link
Contributor

In production we have identifier assignment currently that are overlapping across users.
I think voiding for one of the user may be a good idea.
We should provide voiding in the UI and an SQL that can be run to check where such overlaps are there.

@vinayvenu
Copy link
Member

vinayvenu commented Aug 24, 2023

@petmongrels For User based identifier, we should ensure duplicate ranges are not assigned to the user. We also need to ensure that the prefix for one user is not the same as the one for another user.

@petmongrels
Copy link
Contributor

petmongrels commented Aug 25, 2023

Summarising all the changes made, here.

  • User property IdPrefix cannot be reused within the same organisation. It is case insensitive.
  • Overlapping identifiers not allowed for pooled ids
  • Not handled - cases where the identifier source has a prefix but identifier assignment doesn't have prefix (there are some entries in database like this).

Implementer Doc

@petmongrels
Copy link
Contributor

petmongrels commented Aug 25, 2023

** Query to find existing overlaps (please replace identifier source id and prefix (prefix is H in this example) **

select A.assigned_to_user_id, A.identifier_start, A.identifier_end, B.assigned_to_user_id, B.identifier_start, B.identifier_end, A.id
from identifier_user_assignment A
         join identifier_user_assignment B on A.id <> B.id and A.is_voided = false and B.is_voided = false
where A.identifier_source_id = B.identifier_source_id
  and A.identifier_source_id = 36
  and replace(A.identifier_start, 'H', '')::int < replace(B.identifier_end, 'H', '')::int and replace(A.identifier_end, 'H', '')::int > replace(B.identifier_start, 'H', '')::int
  and A.is_voided = false and B.is_voided = false;

** Query to get two users in same org and same prefix **
Note this may not necessarily be a problem as the IDs assigned may not be overlapping or ID may not have been assigned to them. The above query can be used to find overlaps for user assigned pool as well.

select o.name, a.id, b.id, a.settings->>'idPrefix', b.settings->>'idPrefix' from users a
    join users b on a.organisation_id = b.organisation_id and a.id <> b.id
join organisation o on a.organisation_id = o.id
    where lower(a.settings->>'idPrefix') = lower(b.settings->>'idPrefix') and lower(a.settings->>'idPrefix') <> '' and a.settings->>'idPrefix' is not null
    and a.is_voided = false and b.is_voided = false
order by 1

@vinayvenu
Copy link
Member

avniproject/avni-server@e7540fb#diff-78cbb3dd2b641265df5b13613742034c8b16b0f9871655f2fbc2c8ace3b68a25R10

  • Logger can be private static within WebResponseUtil class instead of being passed in.
  • createInternalServerErrorResponse will swallow the stack trace and log only the message.

Overlapping id sql check is incorrect.
Problem - Verify if range (a, b) overlaps with range (c, d)

Implemented logic - (a between c and d) or b (between c and d)
This will not work if a < c and b > d

Easier logic that works - if a < d and b > c

Verification missing in test method getOverlappingAssignmentForNonPooledIdentifier
assertTrue(hasOverlapUserSpecific(identifierSource, assignmentUserUsingSamePrefix, 50, 210));


This was not identified in the card, but there is an issue where users are not assigned prefixes, which results in bad data getting created.


@petmongrels
Copy link
Contributor

  • Logger is intentionally passed so that we don't lose the where it is happening. the util is only to remove duplication.
  • createInternalServerErrorResponse......... done
  • fixed the overlap logic in code
  • SQL in the comment updated
  • added a check for end > start in the incident ID Assignment
  • added validation check for null id prefix on user

petmongrels added a commit to avniproject/avni-server that referenced this issue Aug 30, 2023
petmongrels added a commit to avniproject/avni-server that referenced this issue Aug 30, 2023
@ashusvnath
Copy link

QA Observations:

  • Cannot create 2 Identifier User Assignments with overlapping ranges for the same user (Error messages show which user assignment overlaps)
  • Cannot create an Identifier User Assignment with start > end (Error message says identifier start cannot be > identifier end)
  • Cannot assign overlapping ranges of Identifier User Assignments to different users (Error messages shows the user with whom a range overlaps).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working user reported
Projects
Archived in project
Development

No branches or pull requests

5 participants