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 Module #3695

Merged
merged 206 commits into from
Mar 15, 2019
Merged

Publication Module #3695

merged 206 commits into from
Mar 15, 2019

Conversation

davidblader
Copy link
Contributor

@davidblader davidblader commented May 28, 2018

Module for proposing & uploading publication projects
Test plan

$pubUploadInsert = array(
'PublicationID' => $pubID,
'PublicationUploadTypeID' => $pubTypeID,
'URL' => basename($fileName),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem to be a URL, since basename is just the filename

`PublicationUploadID` int(10) unsigned NOT NULL AUTO_INCREMENT,
`PublicationID` int(10) unsigned NOT NULL,
`PublicationUploadTypeID` int(2) unsigned NOT NULL,
`URL` varchar(255) NOT NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

If URL is unique, there can't be 2 version of the same file. Is that intended? If yes, what is version for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the version field was included in the original design mock-ups. i interpreted it to be a sort of crude version control feature, where perhaps somebody could upload different versions of the same or similar publication drafts for collaborative purposes.

Copy link
Contributor

@christinerogers christinerogers left a comment

Choose a reason for hiding this comment

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

Great module!

Main comment is that the Readme/nomenclature is a little unexpected:

  • "Publications module" suggests it's for tracking publications, while Readme says it's instead purely for Proposal applications

# Publication

## Purpose
The publication module allows users to propose research projects based on LORIS
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @mohadesz @cmadjar per Monday meeting -

"Publication" strongly suggests this module is for tracking Papers underway/assigned/published.

However says it's for Project Proposals -- i.e. researchers request to perform a series of Analyses, via access to specific datasets (like genomics).

  • This extra sense of "project" confuses our documentation -> suggest "Analysis Proposal" or "Research Proposal"?

Typically, one research analysis project can generate multiple publications. And/or one publication and result from several analyses.

CONSTRAINT `PK_publication_status` PRIMARY KEY(`PublicationStatusID`)
) ENGINE=InnoDB DEFAULT CHARSET='utf8mb4';
INSERT INTO publication_status (`Label`) VALUES ('Pending');
INSERT INTO publication_status (`Label`) VALUES ('Approved');
Copy link
Contributor

Choose a reason for hiding this comment

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

Publication status "Approved" strongly suggests that a manuscript was approved/accepted for publication by a publisher.

If this means just "Project/Access approved" - then consider renaming

`Version` varchar(255),
`Citation` text,
CONSTRAINT `PK_publication_upload` PRIMARY KEY (`PublicationUploadID`),
CONSTRAINT `UK_publication_upload_URL` UNIQUE (URL),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this an external URL (Pubmed) or the URL on (e.g.) *.loris.ca where the paper will be hosted/downloadable (prob relative to a Config setting?) ?
Adding an External URL field is not a bad idea for preprints, the official publisher's site, etc.

  • per my comment in the Readme, this is a very different sense of "publication" from "project proposal"

'publication_parameter_type_rel',
'publication_test_names_rel',
'publication_collaborator_rel',
'publication_keyword_rel',
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be done with foreign key cascading.

@xlecours also suggests wrapping the script in a transaction and rolling back if an exception is caught instead of using a cleanup function.

@davidblader davidblader added the State: Needs adoption PR whose author is no longer involved in, awaiting someone else to pick it up to proceed label Mar 1, 2019
Copy link
Contributor

@zaliqarosli zaliqarosli left a comment

Choose a reason for hiding this comment

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

file upload still not working. im failing to find what the bug is. could someone else please try test?

CONSTRAINT `FK_publication_UserID` FOREIGN KEY(`UserID`) REFERENCES `users` (`ID`),
CONSTRAINT `FK_publication_RatedBy` FOREIGN KEY(`RatedBy`) REFERENCES `users` (`ID`),
CONSTRAINT `FK_publication_PublicationStatusID` FOREIGN KEY(`PublicationStatusID`) REFERENCES `publication_status` (`PublicationStatusID`),
CONSTRAINT `FK_publication_LeadInvestigatorID` FOREIGN KEY(`LeadInvestigatorID`) REFERENCES `publication_collaborator` (`PublicationCollaboratorID`),
Copy link
Contributor

Choose a reason for hiding this comment

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

table publication_collaborator needs to be created first before it is referenced

Copy link
Collaborator

Choose a reason for hiding this comment

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

solved

@zaliqarosli
Copy link
Contributor

on testing: 2 errors no longer an issue. the only error remaining is that on editing an existing publication, file upload throws a DB->update error

@driusan
Copy link
Collaborator

driusan commented Mar 15, 2019

Despite the above concerns and objections, I'm going to merge this as a beta state in the interest of it not falling behind and becoming unmergeable/unmaintainabe.. if anyone wants to tackle any of the above bugs or reviews (or even just log them as a github issue), feel free.

@driusan
Copy link
Collaborator

driusan commented Mar 15, 2019

..except the merge button seems to be broken on my computer :(

@driusan driusan merged commit 0f39ef6 into aces:minor Mar 15, 2019
@ridz1208 ridz1208 added this to the 20.3.0 milestone Mar 25, 2019
kchatpar pushed a commit to kchatpar/Loris that referenced this pull request Apr 15, 2019
This adds a new module called "the publication module."

The publication module allows users to propose research projects based on LORIS 
data, upload publication media, and allows study PIs to review the project 
proposals.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Feature PR or issue that aims to introduce a new feature Priority: Projects PR or issue is a priority for at least one project and should be a higher priority for LORIS State: Needs adoption PR whose author is no longer involved in, awaiting someone else to pick it up to proceed
Projects
None yet
Development

Successfully merging this pull request may close these issues.