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

Curator inbox add document #348

Merged
merged 23 commits into from Apr 28, 2017
Merged

Curator inbox add document #348

merged 23 commits into from Apr 28, 2017

Conversation

jgoley
Copy link
Contributor

@jgoley jgoley commented Apr 18, 2017

PT Story
Changes:

  • update how we associate incidents to their sources: switch from storing the sources URL to its _id
  • Allows curators to add a source with URL or text but only when either the User Added or Current User's feed is chosen
  • Switches to using the incident collection rather than the local collection when suggesting events (this is probably not necessary)
  • Updates UI related to adding the add document button

@jgoley jgoley force-pushed the curator-inbox-add-document branch 3 times, most recently from dad7134 to a3b8e34 Compare April 18, 2017 18:12
@jgoley
Copy link
Contributor Author

jgoley commented Apr 18, 2017

I noticed article documents, now that we are store the enhancements on them, can get pretty large. Are we okay with this?

@jgoley jgoley force-pushed the curator-inbox-add-document branch from 986162a to 67f60f8 Compare April 18, 2017 18:58
@nathanathan
Copy link
Contributor

There are some bugs:

  • Removing the local incidents collection breaks the extract-incidents page.
  • The event page add incident modal doesn't allow submission.
  • Incidents added by highlighting article snippets do not appear in the incident table.
  • The view original document link of the event page goes nowhere for articles with text content.
  • When the apps starts there are many "url is not allowed by the schema" errors.
  • Toggling to the pasted text tab after selecting a suggested article causes the web page to get really laggy.
  • I'm seeing some buggy behavior when I use the inbox date selector to try to select dates from several years ago. Sometimes a different date is used when I hit apply or valid dates are rendered unselectable.
  • A console exception occurs when incidents are added to events, and the incident is not added:
m._d.getTime is not a function
    at valid__isValid (moment.js:131)
    at Moment.moment_valid__isValid [as isValid] (moment.js:3254)
    at Moment.toJSON (moment.js:3250)
    at JSON.stringify (<anonymous>)
    at Object.DDPCommon.stringifyDDP (utils.js:62)
    at Connection._send (livedata_connection.js:1041)
    at MethodInvoker.sendMessage (livedata_connection.js:437)
    at Connection.apply (livedata_connection.js:965)
    at Connection.call (livedata_connection.js:729)
    at Object.click .add-to-event (addToEvent.coffee:61)
  • I'm getting these server side exceptions:
I20170419-03:36:28.518(-4)? Internal exception while processing message { msg: 'method',
I20170419-03:36:28.518(-4)?   method: 'retrieveProMedArticle',
I20170419-03:36:28.519(-4)?   params: [ 4374873 ],
I20170419-03:36:28.519(-4)?   id: '5849' } Maximum call stack size exceeded RangeError: Maximum call stack size exceeded
I20170419-03:43:20.086(-4)? Exception while invoking method 'queryForSuggestedArticles' Error: Match error: Expected string, got null
I20170419-03:43:20.086(-4)?     at exports.check (packages/check/match.js:34:1)
I20170419-03:43:20.087(-4)?     at [object Object].queryForSuggestedArticles (server/dbUtils.coffee:1:1)
I20170419-03:43:20.087(-4)?     at maybeAuditArgumentChecks (packages/ddp-server/livedata_server.js:1711:12)
I20170419-03:43:20.089(-4)?     at packages/ddp-server/livedata_server.js:711:19
I20170419-03:43:20.089(-4)?     at [object Object]._.extend.withValue (packages/meteor/dynamics_nodejs.js:56:1)
I20170419-03:43:20.090(-4)?     at packages/ddp-server/livedata_server.js:709:40
I20170419-03:43:20.090(-4)?     at [object Object]._.extend.withValue (packages/meteor/dynamics_nodejs.js:56:1)
I20170419-03:43:20.091(-4)?     at packages/ddp-server/livedata_server.js:707:46
I20170419-03:43:20.091(-4)?     at Session.method (packages/ddp-server/livedata_server.js:681:23)
I20170419-03:43:20.091(-4)?     at packages/ddp-server/livedata_server.js:551:43
I20170419-03:43:20.099(-4)? Sanitized and reported to the client as: Match failed [400]

I noticed article documents, now that we are store the enhancements on them, can get pretty large. Are we okay with this?

I don't think the size will be a problem as long as we make sure to do projections on any server-side queries that return lots of articles.

@@ -63,8 +77,14 @@ Template.curatorInbox.onRendered ->

@autorun =>
if @ready.get()
if @selectedFeedId.get() in getCustomFeedArray()
Copy link
Contributor

Choose a reason for hiding this comment

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

When the ready variable is false, the autorun won't see the reactive-variables inside this if statement and so won't react to them.

total = instance.incidentCollection.find().count()
count = instance.incidentCollection.find(accepted: true).count();
total = Incidents.find().count()
count = Incidents.find(accepted: true).count();
Copy link
Contributor

Choose a reason for hiding this comment

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

semicolon

Meteor.publish 'ArticleIncidentReports', (articleUrl) ->
Incidents.find {url: $regex: new RegExp("#{regexEscape(cleanUrl(articleUrl))}$")},
Meteor.publish 'ArticleIncidentReports', (articleId) ->
Incidents.find articleId: articleId,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should type check the articleId

@jgoley
Copy link
Contributor Author

jgoley commented Apr 19, 2017

@nathanathan I've fixed some of the issues. Will pick this back up in the morning and fix the rest.

@jgoley
Copy link
Contributor Author

jgoley commented Apr 20, 2017

@nathanathan I've cleared up most of the issues/errors.
I'm getting an error that says Text must be a string while documents are being processed.
Initially I thought it was related to changes around the content property of sources in this PR but its occurring in master as well. Was there a change to GRITS recently that would cause this?

I moved back to the local collection for storing the incidents on the extract-incidents route.

A console exception occurs when incidents are added to events, and the incident is not added

I couldn't replicate this. Do you mean incidents are added but the document is not? Within the inbox?

@nathanathan
Copy link
Contributor

The "Text must be a string" error was caused by a change to the grits api. The issue has been fixed, but it might occur intermittently until we deploy the fix to Kirby as well.

On the latest version, I couldn't replicate the error when adding incidents. However, I noticed a couple problems I didn't see before. When I added incidents from an article based on text content to an event, the article was duplicated in the inbox. Additionally, the incidents on the event page had 'http://' in their document property, and could not be edited due to the empty document URL field.

@jgoley jgoley force-pushed the curator-inbox-add-document branch from c276fc4 to bff7234 Compare April 21, 2017 12:59
@Broham
Copy link
Contributor

Broham commented Apr 21, 2017

looks good 👍

@jgoley
Copy link
Contributor Author

jgoley commented Apr 21, 2017

@nathanathan I've fixed the two bugs you mentioned.

  • I added a new method called associateWithEvent which is now called instead of adding the source/document (I suppose this was a remnant of presenting CuratorSources in the inbox)
  • Solution for the URL field issue: If the incident is based on pasted text content, the document url is not shown. Instead, an annotated snippet is shown. (this snippet will also appear if there is a url and a case annotation)

@nathanathan
Copy link
Contributor

Saving edits to incidents on the curator inbox page for text-input articles is not working for me. I'm seeing this console error:

app.js?hash=7691f76…:15659 Uncaught TypeError: Cannot read property '_id' of undefined
    at Object.incidentReportFormToIncident (app.js?hash=7691f76…:15659)
    at Object.click .save-modal (suggestedIncidentModal.coffee:89)
    at blaze.js:3718
    at Function.Template._withTemplateInstanceFunc (blaze.js:3687)
    at Blaze.View.<anonymous> (blaze.js:3717)
    at blaze.js:2560
    at Object.Blaze._withCurrentView (blaze.js:2214)
    at Blaze._DOMRange.<anonymous> (blaze.js:2559)
    at HTMLButtonElement.<anonymous> (blaze.js:835)
    at HTMLBodyElement.dispatch (jquery.js:4723)

The document URL field is still being shown when editing incidents from text-input articles on the event page.

Also, when a document is added on the event page, the title field is deleted when the user types something in the "Via Pasted Test" field.

{ startingIndex, endingIndex, selectedAnnotationId } = options
export annotateContent = (content, annotations, options={}) ->
{ startingIndex, endingIndex, selectedAnnotationId, pointerEvents} = options
pointerEvents ?= true
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding the selected class and pointer events class could be done in a more versatile way by adding a "classes" property to the annotations (similar to the attributes property) before they passed into the annotateContent function. It will also allow us to keep call-site specific logic out of the function.

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 call. I created an array called classNames

Copy link
Contributor

Choose a reason for hiding this comment

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

@jgoley The classNames arrays should come from the annotations like the attributes property. The selectedAnnotationId and pointerEvents parameters can be replaced by adding the required classNames before the annotations are passed in. The goal is to make it so the sites where the annotation functions are called from have more control over how the content is annotated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Looks good

@@ -96,7 +97,7 @@ Meteor.startup ->
id: "userSpecifiedDisease:#{incident.disease}"
text: "Other Disease: #{incident.disease}"

promedFeed = Feeds.findOne(url: 'promedmail.org/post/')
promedFeed = Feeds.findOne(url: $regex: /promedmail.org/)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this being reverted to a regex?


incidentContent: ->
articleContent = Articles.findOne(@articleId).enhancements.source.cleanContent.content
Spacebars.SafeString(getIncidentSnippet(articleContent, @, 100))
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is currently the only call site, it would make sense to set the default padding characters value to 100.

@jgoley jgoley force-pushed the curator-inbox-add-document branch from 7be8ce9 to d656a45 Compare April 25, 2017 22:22
@jgoley jgoley force-pushed the curator-inbox-add-document branch from d656a45 to 8b45cb2 Compare April 25, 2017 22:25
@jgoley
Copy link
Contributor Author

jgoley commented Apr 25, 2017

@nathanathan I ran out of time to test all the changes on Tue. so I'll take care of that Wed morning and let you know when its ready for hopefully a final review.
I'm still getting an error when processing a new document:
Exception in delivering result of invoking 'getArticleEnhancementsAndUpdate': TypeError: Cannot read property 'source' of undefined
Do you think its related to this PR? Seems like there was a version of this recently on the server?

@nathanathan
Copy link
Contributor

How are you adding the document? There was a recent fix for a similar issue here:
b499a0f

@jgoley
Copy link
Contributor Author

jgoley commented Apr 26, 2017

@nathanathan I'm not seeing the issue in master so I'll look into why its happening in this branch.

@jgoley
Copy link
Contributor Author

jgoley commented Apr 26, 2017

Also about the title being lost. I figured if a user selects a suggested article then enters custom text they would want to enter a new title?

@nathanathan
Copy link
Contributor

The title field being cleared is an issue when the user enters a title before pasting in the article or when the user wants to make some edits to the pasted content after they've entered the title field.

@jgoley jgoley force-pushed the curator-inbox-add-document branch from 4096a52 to 6dcec95 Compare April 26, 2017 17:13
Update other areas affected by lack of url on articles obj
@jgoley jgoley force-pushed the curator-inbox-add-document branch 2 times, most recently from 597e574 to 040f4a5 Compare April 27, 2017 20:23
@jgoley jgoley force-pushed the curator-inbox-add-document branch from 040f4a5 to 7d2d2bc Compare April 27, 2017 20:25
@nathanathan
Copy link
Contributor

👍 Looks good

@nathanathan nathanathan merged commit 6b338ae into master Apr 28, 2017
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