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.

Sign up for GitHub

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

Jump to bottom

Issue #27 - Search new stock name, ignore column and email support. #30

Merged
merged 6 commits into from
Apr 6, 2018
Merged

Issue #27 - Search new stock name, ignore column and email support. #30

merged 6 commits into from
Apr 6, 2018

Conversation

reynoldtan
Copy link
Member

@reynoldtan reynoldtan commented Feb 6, 2018 •
edited
Loading

NOTE
1. this update requires an update to KP nodes to include all
hook_alter() for this update to apply (Issue #6 - Rawphenotypes hook_alter() implementations).
2. Should update code when - Issue #12 Stock names look-up should be restricted to an organism is merged.

  • Upload functionality can now search new name (using a token eg. AGL)
    to a specified project name.
  • Ignore or skip a set column names.
  • Provide a support email for data collectors to email for support.

To test:
A. Support email:
In the homepage of you dev, in rawphenotypes dashboard, click need help? link and test

  1. help topic select jump menu and
  2. support email (please click if it starts a blank email or right click on the link to copy the email address to manually compose an email)

B. Origin

  1. In standard spreadsheet file, add a column Origin.
  2. Upload the spreadsheet file. Ensure to upload file into AGILE project.
    In all upload stages, it should skip this column.
  3. When done, download dataset to verify.

C. New stock names (with AGL).

  1. Upload to AGILE project a spreadsheet file where the name column contains stock names without AGL token.
  2. When complete, download dataset to verify the use of new stock names.

@reynoldtan
Issue #27 - Search new stock name, ignore column and email support.
8885ae9 
NOTE: this update requires an update to KP nodes to include all
hook_alter() for this update to apply.

- Upload functionality can now search new name (using a token eg. AGL)
to a specified project name.
- Ignore or skip a set column names.
- Provide a support email for data collectors to email for support.
@carolyncaron
Copy link
Member

carolyncaron commented Mar 1, 2018 •
edited
Loading

A. Support Email

  • I do see the help topic dropdown, and all the options appear to work as expected. However, I can't seem to find a link for support email - I assume I have to set this up through administration but can't find an option for it.
  • Also, should we consider making the email appear only on the upload page? I feel like having it in the Raw pheno panel on the homepage will either get missed entirely by those who need it or spammed by those who haven't even tried...
    Update: We decided to leave the email link where it is in addition to adding it to the upload page. If it starts getting abused, we'll revisit this.

B. Origin

  • I verified that an 'Origin' column is ignored when uploaded to AGILE. Plus, 'Origin' appears as a new trait for AGILE: Preliminary Trials 👍
  • One thing I noticed is that when running the job manually, the terminal is extremely verbose. Every stock ID and stock name is printed, in addition to progress and whenever a column is skipped (one print statement for each row). If this is for debugging purposes, we probably don't need all of the print statements anymore!
  • Another thing is when uploading a file, the initial check for stock IDs appears to take a long time, thus the progress bar during Step 3 remains at "Waiting..." for half the duration of the job. Update: This bug is not reproducible. Lacey suggests that another job was waiting in the queue, thus I was seeing output to the terminal from the prior job before my intended upload could run.

C. New Stock Names (with AGL)

  • I verified that when uploading to AGILE, stock names gained the AGL suffix when it was missing (gained nothing if it was already there).
  • For a non-AGILE project, AGL was accepted when present but not added when missing.
  • This functionality looks ready to me :)

Regarding the merge conflicts, we will need @laceysanderson's help with addressing this, as we suspect it has to do with recent updates to the module for Tripal 3 support.
Update: Not Lacey's fault! :) Reynold has resolved the conflicts in commit fad6958

@reynoldtan
Merge branch 'master' into Issue-#27---Hook-to-search-new-stock-name,…
fad6958 
…-ignore-header-and-provide-a-support-email
@reynoldtan
Added support email in upload page.
5a6abca 
- Support email and video demo link added to Upload page Need Help?
- Removed print command that prints stock_ids to the terminal.
@reynoldtan
Copy link
Member Author

  • Removed print command that prints stock_id to terminal
  • Added link to support email and video demo in Upload Page
    support_email

@carolyncaron
Copy link
Member

carolyncaron commented Mar 13, 2018 •
edited
Loading

  • I am still seeing excessive print statements when I run a job. My test case is the entire LDP panel (3 reps) with an origin column (value = Mars). Here is a snippet of what I see (the actual output is hundreds of lines):
    image
  • I couldn't see the support email on either the front page or upload form, but after some digging I noticed I was an additional commit ahead on the relevant branch for kp_nodes as compared to the last time I tested (see commit: 0a9ed89) It would have been helpful to know that I needed to set the email address! Lacey suggests perhaps setting a default email to be the same as admin_0 user 1? We then need to update the documentation to explain how to set the email for support (I think this needs a new issue - this reminds me that the docs also need to be updated regarding the update to handle environmental data)

laceysanderson
laceysanderson requested changes Mar 15, 2018
View reviewed changes
Copy link
Member

@laceysanderson laceysanderson left a comment

Choose a reason for hiding this comment

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

A cursory review of the code looks good except for the one comment mentioned regarding a default support email.

rawpheno.module
@@ -326,6 +326,10 @@ function rawpheno_preprocess_rawpheno_upload_form_master(&$variables, $hook) {
$variables['rel_url'] = url('phenotypes/raw/instructions');
// Page Title
$variables['page_title'] = variable_get('rawpheno_upload_title');
// Support Email.
$support_email = '';
drupal_alter('rawpheno_support_email', $support_email);
Copy link
Member

Choose a reason for hiding this comment

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

There should be an API function for retrieving the support email. Furthermore, the code in kp_nodes determining the default support email (https://github.com/UofS-Pulse-Binfo/kp_nodes/pull/7/files#diff-ea83b3e1886aaaeac403bd6114856bd6R553) should be in this rawphenotypes API function --otherwise only we are given a default ;-)

@laceysanderson
Copy link
Member

Note: I added a pull request for Kp Nodes as well: UofS-Pulse-Binfo/kp_nodes#7 (review)

There are suggested changes on it too :-)

@laceysanderson
Copy link
Member

Thus, I'm calling this PR Approved 🎉

@laceysanderson laceysanderson merged commit 1aef528 into master Apr 6, 2018
@laceysanderson laceysanderson deleted the Issue-#27---Hook-to-search-new-stock-name,-ignore-header-and-provide-a-support-email branch April 6, 2018 19:19
@reynoldtan reynoldtan mentioned this pull request Apr 25, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laceysanderson laceysanderson laceysanderson requested changes

Assignees

@carolyncaron carolyncaron

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@reynoldtan @carolyncaron @laceysanderson