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

[publication] Lead investigator issue #7471

Merged

Conversation

pierre-p-s
Copy link
Contributor

@pierre-p-s pierre-p-s commented Jun 10, 2021

Brief summary of changes

As it currently stands, when a publication is proposed (tab: propose a project) the user is asked, amongst other things, for a lead investigator name and email if these values are not already in the database, they get inserted into the collaborator table, if they do already exist they get linked to the new project.

The issue here is when 2 or more users share a name and/or an email address (project address like cbigr@mcin.ca for example) Since the SQL query currently uses and OR operator, the query returns the first name that matches or the first email that matches without checking the other value causing in some instances to select the wrong investigator and displaying the incorrect name or email in the menu filter.

Solves issue from CCNA: https://github.com/aces/CCNA/issues/4555

Testing instructions (if applicable)

  1. Create publication with your name as lead investigator and your email
  2. Create another publication with someone else's name as lead investigator but with the same email
  3. Check that the 2 publications lead investigators have saved properly

@pierre-p-s pierre-p-s changed the base branch from main to 23.0-release June 10, 2021 14:40
@pierre-p-s pierre-p-s changed the title 2021 06 10 publication investigator issue [publication] Lead investigator issue Jun 10, 2021
@pierre-p-s pierre-p-s closed this Jun 11, 2021
@pierre-p-s pierre-p-s reopened this Jun 11, 2021
@pierre-p-s pierre-p-s added the "Help! I don't understand Travis!" PR is having a beef with TRAVIS. Someone needs to help label Jun 14, 2021
@driusan
Copy link
Collaborator

driusan commented Jun 15, 2021

This is failing the tests and can you please update the description to be more descriptive.

@ridz1208 ridz1208 removed the "Help! I don't understand Travis!" PR is having a beef with TRAVIS. Someone needs to help label Jun 16, 2021
@ridz1208
Copy link
Collaborator

@pierre-p-s tests are now passing but I have no idea why this PR is making that change. can you please add a description with a detail issue and fix ?

modules/publication/ajax/FileUpload.php Show resolved Hide resolved
@ridz1208
Copy link
Collaborator

@pierre-p-s I just tested on demo and I cannot reproduce the error.

@ridz1208
Copy link
Collaborator

@pierre-p-s I updated the description to the best of my capacity

Now that I understand the issue a bit more and given the possibility or emails not being unique in this usecase, I think the collaborator table should have a combined primary key of name and email which should low significantly the risk of an overlap and an incorrect display

@pierre-p-s
Copy link
Contributor Author

@ridz1208 I changed the OR to an AND.

@ridz1208 ridz1208 dismissed their stale review June 18, 2021 19:40

not approving yet until we discuss the intended functionality

@driusan driusan merged commit 1be38d0 into aces:23.0-release Aug 19, 2021
@ridz1208 ridz1208 added this to the 23.0.6 milestone Aug 19, 2021
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.

None yet

3 participants