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

Attempting to update rubocop #822

Merged
merged 2 commits into from Jan 10, 2018
Merged

Attempting to update rubocop #822

merged 2 commits into from Jan 10, 2018

Conversation

mdbenjam
Copy link
Contributor

@mdbenjam mdbenjam commented Jan 9, 2018

Our version of Rubocop had a vulnerability. Upgrading causes new rules to be added. I mimicked most of what was done in: https://github.com/department-of-veterans-affairs/caseflow/pull/4312/files. Where only one thing was flagged by a rule, I fixed that one instance, and called it out in the PR.

Test Plan

  • The only way to really test this is to rely on automated tests. Quick smoke test also passed.

@@ -9,7 +9,7 @@ def self.run(command)
puts(line)
end

success = thread.value == 0
success = thread.value.zero?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed by hand. Tested by running rake commands which all rely on it.

@@ -1,13 +1,14 @@
class DocumentCreator
include ActiveModel::Model

attr_accessor :manifest_source, :external_documents
attr_accessor :manifest_source
attr_writer :external_documents
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed by hand. Should work if tests pass since external_documents is written to in several jobs.

@@ -2,14 +2,15 @@ class CssAuthenticationSession
include ActiveModel::Model
include ActiveModel::Serializers::JSON

attr_accessor :id, :css_id, :email, :name, :roles, :station_id
attr_accessor :id, :name, :roles, :station_id
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed by hand.

@@ -22,7 +22,7 @@ def document_download_failed(error_kind)
status = 500
message = "Caseflow eFolder failed to fetch document contents."

if error_kind == :vva_error || error_kind == :vbms_error
if [:vva_error, :vbms_error].include?(error_kind)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed by hand. Tested in documents_spec.rb test: returns 502 if there is a VBMS error

@mdbenjam mdbenjam changed the title [WIP] Attempting to update rubocop Attempting to update rubocop Jan 9, 2018
Copy link
Contributor

@lowellrex lowellrex left a comment

Choose a reason for hiding this comment

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

LGTM!

@mdbenjam mdbenjam merged commit 628fb30 into master Jan 10, 2018
@pkarman pkarman deleted the mdbenjam-update-rubocop branch April 11, 2019 20:53
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

2 participants