WikiEducationFoundation / WikiEduDashboard

Wiki Education Foundation's Wikipedia course dashboard system
https://dashboard.wikiedu.org
MIT License
387 stars 623 forks source link

NoMethodError when PlagiabotImporter query does not return results #2485

Open ragesoss opened 5 years ago

ragesoss commented 5 years ago

https://sentry2.wikiedu.org/wikiedu/p-e-dashboard/issues/2602/

NoMethodError: undefined method `each' for nil:NilClass
  lib/importers/plagiabot_importer.rb:24:in `find_recent_plagiarism'
    suspected_diffs.each do |rev|
  lib/data_cycle/constant_update.rb:62:in `update_revisions_and_articles'
    PlagiabotImporter.find_recent_plagiarism
  lib/data_cycle/constant_update.rb:37:in `run_update'
    update_revisions_and_articles
  lib/data_cycle/batch_update_logging.rb:30:in `run_update_with_pid_files'
    run_update # implemented by each update class
  lib/data_cycle/constant_update.rb:30:in `initialize'
    run_update_with_pid_files(:constant)
...
(36 additional frame(s) were not displayed)

NoMethodError: undefined method `each' for nil:NilClass
Aeropio commented 5 years ago

Hello I am current outreachy applicant and wants to work on this issue, please know the steps to reproduce this in my local

Aeropio commented 5 years ago

@ragesoss : if we check suspected_diffs.present? then this mentioned error can be avoided; incase if suspected_diffs is not present then what the method has to return?

ragesoss commented 5 years ago

Something like that seems reasonable, although if there's anywhere earlier in the data flow to handle this failure mode, that would be better.

A patch for this should also include a test for a mocked query that would lead to this error.

Aeropio commented 5 years ago

@ragesoss in api_get method of PlagiabotImporter class if we check if response is present then it would be better, but if response is not present then what the method has to return?

Aeropio commented 5 years ago

Something like that seems reasonable, although if there's anywhere earlier in the data flow to handle this failure mode, that would be better.

A patch for this should also include a test for a mocked query that would lead to this error.

@ragesoss where should I write the test for the mocked query? in which file?

ragesoss commented 5 years ago

Probably plagiabot_importer_spec.rb

Aeropio commented 5 years ago

@ragesoss : My understanding is that api_get method returns nil in the find_recent_plagiarism method , but i am not sure how to reproduce this error?please share the Steps to reproduce this behavior

bwreid commented 5 years ago

@Aeropio I think it will be easiest to just build the test so that the Net::HTTP.get returns nil. I think the problem at the moment is we're not entirely sure what yet is causing the return of nil but still want to be able to handle it.

Aeropio commented 5 years ago

@bwreid okay sure