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

Issue #1980: Evaluation Summaries not populating with data #2083

Merged
merged 5 commits into from
Jan 25, 2020

Conversation

markmandell
Copy link
Contributor

@markmandell markmandell commented Nov 22, 2019

Issue #1980 :
Here are the steps to recreate the bug and test this issue -

  1. In a local instance without these changes, log in to VCI and curate a new variant. Create an interpretation, and evaluate some criteria to gain access to the Evaluation Summary. Save the initial form until you have saved the interpretation as 'Approved'.
  2. Navigate to the Approved Summary, and open the browser console. Navigate to the network tab and choose either 'Fast 3G' or 'Slow 3G' (have been able to recreate using both, but slow option seems to be the most reliable method to get the error)
  3. It may take a couple of tries but you should see an error along the lines of:
    Screen Shot 2019-11-18 at 3 18 07 PM

After browsing Stackoverflow/Github, specifically here, there was a suggestion that an inconsistency with dates may be an issue when receiving the above error. Also, the date is the only data that is being rendered here, with it being the current time, not the date the classification was saved.

If you comment out line 65 (the call to getClassificationSavedDate in render()) in /src/clincoded/static/components/variant_interpretation_summary/header.js, and keep the same network throttling in step 2, the error should not occur. If you pull in the changes from this branch, you will get the same result.

I assigned the desired date to a conditional in which getClassificationSavedDate will only be called if last_modified exists in the classification object. This is because the moment library was returning the current time upon the initial render (due to classification being undefined), and if the users connection was too slow to resolve this inconsistency, the Summary would error out. This is why some users reported this issue while others may not have encountered the bug at all. I set the else condition to be null, because if undefined, moment will return the current time, instead of 'Invalid Date'.

@h-tong h-tong self-requested a review December 13, 2019 18:22
Copy link
Contributor

@h-tong h-tong left a comment

Choose a reason for hiding this comment

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

This is a great catch! From my testing, this fixes the bug.

@@ -62,7 +63,7 @@ class VariantInterpretationSummaryHeader extends Component {
{classification ?
<div>
<dt>Date interpretation saved:</dt>
<dd className="classificationSaved">{moment(getClassificationSavedDate(classification)).format("YYYY MMM DD, h:mm a")}</dd>
<dd className="classificationSaved">{moment(lastSavedDate).format("YYYY MMM DD, h:mm a")}</dd>
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about conditionally rendering this, so that the user doesn't see "Invalid Date"?

{lastSavedDate ?
  <dd>.......</dd>
  : null}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion! I'll implement and push the change

Copy link
Contributor

@shaungc-su shaungc-su left a comment

Choose a reason for hiding this comment

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

The logic fix looks good to me.

@markmandell markmandell merged commit 3da736c into dev Jan 25, 2020
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.

3 participants