Improve/api variants #594
Open
Improve/api variants #594
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If something is off with a given variant, a dialogue will now open to inform the user of these issues and propose a possible fix. Also: fixed issue with outdated curly brackets as used to index a string.
The isHGVS function is created to increase readability when doing an HGVS check. Before this commit, this was done using lovd_getVariantInfo($sVariant, false, true); not very readable. Now this can simply be done using lovd_isHGVS($sVariant).
This large commit stores the outlines of the approach on how to open the dialogue, perform tests, do the mapping and make sure only validated variants are passed onto the database.
To sum, this commit: - simplifies and cleans up parts of the code; - reduces the amount of output sent to the user, by using images instead of sentences to update the user on the status of the steps; - makes sure the form receives images to communicate status updates as well, and blocks all options that should be blocked after some input fields have been automatically filled.
We noticed that flush() actually does not fully make sure the output is immediately shown to the users. Instead, what happens is this: When flush() is being called, the buffered output is duly sent to the browser. However, the browser (at least as tested on Chrome) only starts to execute the sent in code once the script of origin has stopped running. This means that flush() cannot be used to dynamically update the HTML webpage while processes are active on the background.
Because we realised that the browser does not execute code that was sent in before the end of the script using flush(), we now use recursive calls instead. This way, we made sure that the user's screen will already show the dialogue, even when the mapping is not done yet.
If VariantValidator returns false, if we use an unknown build, or if syntax was found which VV does not support, we now make sure these ARE allowed into the database anyway, since these issues are outside of the user's control.
To give more credits to VariantValidator and its crucial role in the mapping of the variants, we have added its name to the title of our dialogue.
The md5 translation of validated variants will later serve to check whether the received input was indeed validated, and can thus be sent to the database.
The md5 translation of validated variants will later serve to check whether the received input was indeed validated, and can thus be sent to the database.
We want the code to use the fields of the chosen genome build similarly to how the code handled the position fields at first. To ensure that this is done correctly, we simply name our chosen position fields to how the original fields are named, so without a suffix.
ifokkema
requested changes
Apr 8, 2022
Instead of continuously using ternary IFs to concatenate column suffixes to the fields of a specific genome build, now this is done directly in the SQL query that builds $aActiveGBs. This way, all code needing column suffixes is shorter and more readable.
Instead of continuously using ternary IFs to concatenate column suffixes to the fields of a specific genome build, now this is done directly in the SQL query that builds $aActiveGBs. This way, all code needing column suffixes is shorter and more readable.
Because the SQL query has been rewritten for this branch to now process aliases dynamically depending on which genome builds are active, the SQL query needed loads of refactoring. During the rewriting, the line that is deleted with this commit was wrongfully refactored to retrieve the main DNA field instead of the one on hg38. In reality, this line should have been removed altogether, because later in the code, where this concatenated string in exploded, it no longer needs and thus no longer expects the DNA item.
When going through the aliases of each build, we can find different aliases of the same build for one and the same variant on the main build. Before, these multiple aliases were returned in a concatenated format such as <alias1>;<alias2>. Now, each alias gets their own array.
This commit fixes the code that caused each panel to contain more aliases than just those of a particular variant ID.
Previously, we simply took the suffixes from the database and added these in $aActiveGBs. Back then, $sGBSuffix was a correct variable name. Now we have rebuilt $aActiveGBs to contain complete column names instead of just the suffixes, so the variable names used in loops through $aActiveGBs needed to be updated as well.
ifokkema
requested changes
May 25, 2022
| $aTranscripts = (empty($_REQUEST['transcripts'])? array() : explode('|', $_REQUEST['transcripts'])); | ||
|
|
||
| // Retrieving the name of the input field. | ||
| $sFieldName = $_REQUEST['fieldName']; |
There was a problem hiding this comment.
This variable hasn't been sanitized everywhere. It's generally a problem to take fields out of $_GET or $_POST and rename them as $variable if you're not sanitizing them. You'll quickly forget that this variable ($sFieldName in this case) can contain exploits. That is unlikely to happen when you would have left it as $_REQUEST['fieldName'], as those variables "look" dangerous.
So, either sanitize (check and/or correct) it here, or sanitize it where you use it and leave it named $_REQUEST['fieldName'].
|
|
||
|
|
||
| // Retrieving the variant and the transcripts to map to. | ||
| $sVariant = $_REQUEST['var']; |
There was a problem hiding this comment.
I can not properly follow this variable everywhere, to see if it's sanitized everywhere. Better not rename it, or sanitize it here. See also the two comments below.
|
|
||
| // Retrieving the variant and the transcripts to map to. | ||
| $sVariant = $_REQUEST['var']; | ||
| $aTranscripts = (empty($_REQUEST['transcripts'])? array() : explode('|', $_REQUEST['transcripts'])); |
There was a problem hiding this comment.
This variable hasn't been sanitized and is used unsanitized (danger!). Better sanitize it here or don't rename it. See also the comment below.
src/class/api.ga4gh.php
Outdated
| } | ||
| } | ||
| ?> | ||
| <?php |
There was a problem hiding this comment.
Please fix the line endings; it's CRLF again!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This branch updates the code that allows variants to be viewed using LOVD APIs. The code will be refactored and extended to dynamically handle different active genome builds.
Closes #589. Related to #550.