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

TechDebt: System User Enhancements/Fixes #1316

Merged
merged 20 commits into from
Jul 25, 2024
Merged

Conversation

NickPhura
Copy link
Collaborator

@NickPhura NickPhura commented Jun 26, 2024

Links to Jira Tickets

https://apps.nrs.gov.bc.ca/int/jira/browse/SIMSBIOHUB-599

Description of Changes

Copy existing database functions to procedures folder, where they can be migrated and maintained more easily.
Update api_patch_system_user.ts to do case-insensitive comparisons when finding a system user.

  • Remove the ability to request a role when requesting access
  • Migration to prevent duplicates in system_user table
  • Removing references to duplicated system_user_ids

Testing Notes

Should not be able to add a user with the same username as an existing system user (case-insensitive).

  • When adding a user through the request access flow
  • When adding a user manually (through the admin manage users page -> add users button)

New migration to merge duplicate system_user records.

  • The duplicate records will still remain in the system_user table, they will be end-dated, and a comment is added to the so-far-unused notes column to indicate they were merged into another system user record.

Other

Ran this query in Prod to see if there are any duplicates.

WITH w_duplicates AS (
  SELECT 
    array_remove(
      array_agg(
        DISTINCT(system_user.system_user_id)
      ), 
      NULL
    ) AS system_user_ids, 
    LOWER(user_identifier) AS user_identifier, 
    user_identity_source_id, 
    COUNT(
      LOWER(user_identifier)
    ) AS count 
  FROM 
    system_user 
  WHERE 
    record_end_date IS NULL 
  GROUP BY 
    LOWER(user_identifier), 
    user_identity_source_id 
  HAVING 
    COUNT(
      LOWER(user_identifier)
    ) > 1
) 
SELECT 
  w_duplicates.*, 
  array_remove(
    array_agg(
      DISTINCT(project.project_id)
    ), 
    NULL
  ) AS project_ids 
FROM 
  w_duplicates 
  JOIN project_participation ON project_participation.system_user_id = ANY(w_duplicates.system_user_ids) 
  JOIN project ON project.project_id = project_participation.project_id 
GROUP BY 
  w_duplicates.system_user_ids, 
  w_duplicates.user_identifier, 
  w_duplicates.user_identity_source_id, 
  w_duplicates.count;
system_user_ids user_identifier user_identity_source_id count project_ids
{92,111} rstaplet 2 2 {43,46,49,50,55,56,58,67,79,80,89,90,99}
{93,108} eroorda 2 2 {39,46,49,50,55,56,58,80,87,89,99,100,101}
{95,110} gemery 2 2 {95,96}
{96,107,109,113,114} jbudzins 2 5 {40,41,47,87}

That jbudzins one was created today, so there might be more appearing if they keep trying and more records get added.

… be migrated and maintained more easily.

Update api_patch_system_user.ts to do case-insensitive comparisons when finding a system user.
Copy link

codecov bot commented Jun 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (dev@9113f6a). Learn more about missing BASE report.

Current head ae58f54 differs from pull request most recent head 94fef77

Please upload reports for the commit 94fef77 to get more accurate results.

Additional details and impacted files
@@          Coverage Diff           @@
##             dev    #1316   +/-   ##
======================================
  Coverage       ?   49.21%           
======================================
  Files          ?      639           
  Lines          ?    17707           
  Branches       ?     2787           
======================================
  Hits           ?     8714           
  Misses         ?     8409           
  Partials       ?      584           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mauberti-bc
Copy link
Collaborator

mauberti-bc commented Jun 26, 2024

system_user_ids user_identifier user_identity_source_id count project_ids
{92,111} rstaplet 2 2 {43,46,49,50,55,56,58,67,79,80,89,90,99}
{93,108} eroorda 2 2 {39,46,49,50,55,56,58,80,87,89,99,100,101}
{95,110} gemery 2 2 {95,96}
{96,107,109,113,114} jbudzins 2 5 {40,41,47,87}
That jbudzins one was created today, so there might be more appearing if they keep trying and more records get added.

I'm working on a migration that should remove duplicate users based on (user_identifier, user_identity_source_id) as a unique key and then patch any references to those removed system_user_ids.

@NickPhura
Copy link
Collaborator Author

select t.table_schema,
       t.table_name
from information_schema.tables t
inner join information_schema.columns c on c.table_name = t.table_name 
                                and c.table_schema = t.table_schema
where c.column_name = 'last_name'

@NickPhura NickPhura self-assigned this Jul 18, 2024
Update migration to combine queries into one pieces of sql.
@NickPhura NickPhura requested a review from MacQSL July 22, 2024 21:54
@NickPhura NickPhura marked this pull request as ready for review July 23, 2024 00:02
@NickPhura NickPhura added the Ready For Review PR is ready for review label Jul 23, 2024
Copy link
Collaborator

@mauberti-bc mauberti-bc left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Copy link

sonarcloud bot commented Jul 24, 2024

@NickPhura NickPhura merged commit 62ba230 into dev Jul 25, 2024
20 checks passed
@NickPhura NickPhura deleted the npTechDebt-System-User branch July 25, 2024 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants