MHV Activity Logging Client #442

saneshark · 2016-11-04T05:07:52Z

This is just the client part of MHV activity logging. The integration with vets.gov session will happen in a separate PR with accompanying integration tests.

mphprogrammer · 2016-11-04T14:30:01Z

@saneshark LGTM, I see that @ayaleloehr is also assigned. Shall I wait for his input, or upmerge and merge now?

aub · 2016-11-04T14:38:04Z

.rubocop.yml

@@ -78,3 +78,4 @@ Style/AccessorMethodName:
  Exclude:
    - 'lib/rx/**/*'
    - 'lib/sm/**/*'
+    - 'lib/mhv_logging/**/*'


If I remember correctly, rubocop doesn't like methods named get_something..., post_something.... However, that naming style was adopted for the client apis.

You can tell it to ignore those specific lines if you want.

# rubocop:disable RULE

We could also disable that rule, which IMHO makes no sense whatsoever.

Yes, i like using get_ and put_ for methods that are making those types of http calls and this rule prevents me from doing so.

I guess the idea of the rule is that you shouldn't put get in the name of a getter method (just call it x, not get_x), but it makes no sense not to say "get" in the context of making an HTTP client.

aub · 2016-11-04T14:40:14Z

lib/mhv_logging/api/audits.rb

+# frozen_string_literal: true
+module MHVLogging
+  module API
+    # This module defines the prescription actions


Bad copy/paste?

yea, i will fix this.

aub · 2016-11-04T14:45:10Z

lib/mhv_logging/api/audits.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true


I'm not sure I understand why this module is separate from the Sessions one or really why they're modules at all rather than being inside client.rb directly. Seems like a strategy to factor the code out, but if it's not being reused then I think we're adding complexity with little gain.

@aub I did it this way very simply because I wanted to have an MVC style concept I guess, where the api methods correlate to the rails controllers calling them. By separating these out into modules that get mixed in to the client, the specific modules are kind of the equivalent of a Controller for the client. The idea was that later, with minor refactoring I could mixin those same modules into the Model itself, and for example a Message object would be able to lookup values via the client in an ActiveRecord like way. I ultimately opted against doing this.

However, this allowed us to write specs for each of the apis sections in an organized manner, it's more a matter of separating out related MHV endpoints into distinct units. It's true, they could all be on client, but its one big jumbled mess if I did it that way. Take for example Secure messaging, the api modules map directly to rails controllers, specs are organized in a similar fashion. There is actually also a rubocop rule related to length of a class, and secure messaging would easily exceed that limit without this variety of separation. I strongly feel it helps with organizing the methods.

Until I have a chance to DRY up clients and remove some of the repetitive cruft into a parent client class, I prefer to keep this separation / mixin structure even if it only serves to keep things organized.

aub · 2016-11-04T14:46:52Z

lib/mhv_logging/client.rb

+
+    def initialize(session:)
+      @config = MHVLogging::Configuration.instance
+      @session = MHVLogging::ClientSession.find_or_build(session)


Having @session and session is pretty confusing. Should it be @mhv_session?

honestly, I would like to change the DSL for these initialize altogether, but that will happen in a separate refactor.

I want to make the clients such that they are invoked in the following manner.

MHVLogging::Client.user(user_id) -- this sets up a new session given the user id.
MHVLogging::Client.existing_session(session) -- this sets up an existing session

Both of these would return an instance of Client, and calling .new would require you to pass one or the other.

MHVLogging uses the same redisstore as prescription because those sessions do not need to be unique. Currently this is the same DSL used by the other two clients, so I did more for consistency between those than anything else. What's confusing about it? session can be passed in as a hash of values, which will be used by the find_or_build method to either build a new session if none exists in redis or deserialize a session based on the input and persist it to redis upon authentication.

ClientSession is a type of RedisStore.

I'm just confused by the naming of the variables. You have a session variable that gets passed in and is used to create an instance variable of the same name but that is a different type of thing. Note that this is not a big deal.

I see what you mean now. Especially since one is a hash and the other is an object federalized from hash having the same name. I will fix this in a larger refactor though if that's OK.

I will need to do that anyway as part of building a generic client class to out in common::client

aub · 2016-11-04T14:52:05Z

lib/mhv_logging/configuration.rb

+require 'rx/configuration'
+module MHVLogging
+  # Configuration class used to setup the environment used by client
+  class Configuration < Rx::Configuration


How can it be a subclass of Rx::Configuration when things like the base path are pretty different between the two?

The basepath for Rx and MhvLogging are identical. In fact everything about it is, and it uses the same ClientSession redis store. I just made the clients distinct from one another rather than using Rx, because audit log can also be invoked by secure messaging, and it has distinctly different base path and different client session for that.

So the requests go to URLs like #{host}/mhv-api/patient/v1/activity/auditlogout? If that's true, then we still have a problem with breakers for this. It has to be able to distinguish requests to the different services in order to tell them apart. Take a look at the matcher in Rx::Configuration.

Does breakers need to distinguish it? Rx and MHVLogging are technically the same server is my understanding, so if one is down, it would follow that the other is down. Perhaps I should just use the same Rx breaker then maybe?

Only reason why this client is not just using Rx is that it didn't make a whole lot of sense for me to invoke a client for prescriptions from secure messaging, and of course the fact that the sessions, for whatever reason are distinct between SM and Rx+MHVLogging

Yeah, breakers needs to distinguish if it's going to report the things as being from separate services. If they should really be counted as the same, which is fine with me, I think the simplest solution would be to not add the MHVConfiguration one in the breakers initializer.

I'll change this then and possibly look at naming the configuration something like MHVConfiguation vs Rx::Configuration.

saneshark · 2016-11-04T19:25:42Z

@aub can you give this a second pass, wherever possible I changed it to just use the mixins, and classes from Rx, and put a comment at the top to notify that this is only a separate client to properly separate out concerns between what Rx, since this is also used by SM

…eterans-affairs/vets-api into mhv_activity_api

aub · 2016-11-04T19:29:24Z

LGTM

@ayal

* mvi cert and key are figaro required keys, but should not be read in dev (#399) * mvi cert and key are figaro required keys, but faked values should return nil * added initializer to load mvi cert and key * removing unused vcr_cassettes (#412) * Changed default rx sort to prescription_name (#403) * Changed default rx sort to prescription_name * Doh. spec test heading * Prune applications that were processed > 1 month ago (#364) * EVSS: Queue requests for decision (#407) * EVSS: Queue requests for decision * Spec fixes * Finish refactor of common client part 2 (#411) * adding secure messaging * adding spec to improve test coverage issue * stylistic improvements * refactoring prescriptions * rubocop style change * removing old rx parser * removing unused artifacts * adding new prescriptions factory * Allow Education Forms to be sent to a different facility (#400) * Allow Education Forms to be sent to a different facility * PR Feedback * Swallow exception on MVI missing (#413) * swallow exception on MVI missing * remove cruft * Add authorization to SM (#396) * add authorization to SM * revert cassettes * revert cassettes * fix specs without cassettes * fix one more * rubocop * fix coverage * Log validation errors in sentry (#404) * send validation error to sentry * test raven capture exception * return the user (#418) * EVSS: Remove intent to file (#414) Wasn't being used by us, so remove it. * re-enabling auth for rx (#417) * re-enabling auth for rx * fixing stupid * DRY up MHV concerns * add in the concerns * bypass simplecov stupidness * fixing issue from code review * Instrument sidekiq (#410) * Instrument sidekiq * Add sidekiq stats job spec * Cron format for stats job prevents duplicate runs * Adds notes on usage to sidekiq scheduler * EVSS: Add an old claim reaper (#419) * EVSS: Add an old claim reaper We don't have the ATO to store claims indefinitely, so we need to delete old ones periodically. * Update name and schedule * Return empty vaprofile if mvi nil (#422) * updating specs for nil va_profile * va_profile return as null if mvi is down or record not found * don't need profile in user.mvi nil spec * rubocop * Update mock mvi users (#354) * Changes to reflect updated GeoServices VHA attributes (#425) * finish daily report job (#392) * add govdelivery server to app yml * use govdelivery server in staging email method * use govdelivery server in config helper * fix server name * add year to date report to scheduler * perform doesnt require argument * fix statds for spring, add govdelivery server to figaro * add govdelivery server to travis * add va stakeholders * lint * rename report mailer * rename constants * Adding MHV accounts for some mock users (#427) * flush specific redis namespaces via rake (#421) * flush specific redis namespaces via rake * minor fix * allow rake task to invoke environment for models * Application Controller: Send exception to Sentry/Raven (#424) * EVSS: Add a new field/serializer for the list view (#423) * EVSS: Add a new field/serializer for the list view * Add some tests * Add NotImplementedError * Upload nil tracked item (#429) * Increase timeout for EVSS * Remove breakers for testing EVSS * Add some nocov to get up to coverage while breakers are disabled * Omit tracked item id if not present * Set document filename to sanitized name * Clear workers before each spec * Thread pagination (#431) * Removed pagination from threads * new schema for thread * added optional draft id in creates (#432) * added optional draft id in creates * Added draft id to sm documentation * Allow a blank gender in User model validation (#428) * allow a blank gender in User model validation * ask alastair if this is preferable * gender is more likely to be nil than blank. added specific test for it * per alastairs recommendation, put this check back in * Fix folder create (#441) * Update VA stakeholders list (#433) * update va stakeholders * lint * handle routing errors with JSONAPI (#426) * handle routing errors with JSONAPI * routing errors fix * commented out action_missing method, added stylistic improvements * EVSS: Add receivedFromOthersList to events timeline (#436) Fixes https://github.com/department-of-veterans-affairs/sunsets-team/issues/183 * update va_profile, return null for mvi down, 'not found' for no record (#438) * added mvi status to user va_profile * EVSS: Return sync status in json meta (#439) fixes https://github.com/department-of-veterans-affairs/sunsets-team/issues/193 * EVSS: Add other documents to events timeline (#444) * EVSS: Add other documents to events timeline Refs https://github.com/department-of-veterans-affairs/sunsets-team/issues/197 * Add date to objects * Clearer spec * Add SLO Logout (#443) * start putting together SLO * remove junk, add junk, fix signing * finalize SLO * set the logout relay * less dopiness * delete the user too * do not delete the user * implement suggestions from Bill * change the action name * Use EVSS ID for disability claims lookup (#446) * Use EVSS ID for disability claims lookup * Add comment on ID aliasing * MHV Activity Logging Client (#442) * initial commit of mhv activity client * implementing the client for mhv audit logging * adding the breakers service to breakers initializer * dryed up the common issues and addressed comments from code review * remove unnecessary comma * change spool filenames (#445) * EVSS: Set a flag when a user requests a decision (#449) * EVSS: Set a flag when a user requests a decision Solves an issue where the job might not be processed for a while * Rubocop * simplify spec * Sidekiq stats job on critical queue (#435) * Make mvi processing code env var i451 (#453) * added mvi service handling for 200 from vaafi but 500 from mvi (#454) * stop using a singleton for SAML::SettingsService (#447) * stop using a singleton for SAML::SettingsService * do the whole SSL thing * fix spec * MHV Login/Logout facility Integration (#452) * adding the logout logging facility for mhv as an integration, specs to follow * remove unused redis_store change * only do it mhv_correlation_id exists * allow specs to pass but still need to add specs to test service * get test coverage to 100% * addressing comments from @ayal * oops * fixing linter issues * Enable breakers for EVSS services (#457) * Pv facloc pagination (#437) * Error logging on GIS errors * Pagination functional. * Sorting by distance from bbox center * use breakers for MVI (#448)

saneshark added 3 commits October 30, 2016 10:59

initial commit of mhv activity client

3f4ed6e

Merge branch 'master' into mhv_activity_api

6635b9a

implementing the client for mhv audit logging

1167af3

saneshark added in progress in progress and removed in progress labels Nov 4, 2016

saneshark assigned ayaleloehr and mphprogrammer Nov 4, 2016

adding the breakers service to breakers initializer

2195e8f

saneshark changed the title ~~Mhv activity api~~ MHV Activity Logging Client Nov 4, 2016

aub suggested changes Nov 4, 2016

View reviewed changes

saneshark added 2 commits November 4, 2016 15:24

dryed up the common issues and addressed comments from code review

1f5acf0

Merge branch 'master' into mhv_activity_api

f2c2a25

saneshark added 2 commits November 4, 2016 15:26

remove unnecessary comma

1de10de

Merge branch 'mhv_activity_api' of https://github.com/department-of-v…

32e4287

…eterans-affairs/vets-api into mhv_activity_api

aub approved these changes Nov 4, 2016

View reviewed changes

saneshark merged commit f0fec83 into master Nov 4, 2016

saneshark deleted the mhv_activity_api branch November 4, 2016 19:36

saneshark removed the in progress label Nov 4, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MHV Activity Logging Client #442

MHV Activity Logging Client #442

saneshark commented Nov 4, 2016 •

edited

Loading

mphprogrammer commented Nov 4, 2016

aub Nov 4, 2016

mphprogrammer Nov 4, 2016 •

edited

Loading

aub Nov 4, 2016

saneshark Nov 4, 2016

earthboundkid Nov 4, 2016

aub Nov 4, 2016

saneshark Nov 4, 2016

aub Nov 4, 2016

saneshark Nov 4, 2016

aub Nov 4, 2016

saneshark Nov 4, 2016 •

edited

Loading

saneshark Nov 4, 2016 •

edited

Loading

aub Nov 4, 2016

saneshark Nov 4, 2016

aub Nov 4, 2016

saneshark Nov 4, 2016

aub Nov 4, 2016

saneshark Nov 4, 2016

aub Nov 4, 2016

saneshark Nov 4, 2016

saneshark commented Nov 4, 2016

aub commented Nov 4, 2016

MHV Activity Logging Client #442

MHV Activity Logging Client #442

Conversation

saneshark commented Nov 4, 2016 • edited Loading

mphprogrammer commented Nov 4, 2016

Choose a reason for hiding this comment

mphprogrammer Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saneshark Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

saneshark Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saneshark commented Nov 4, 2016

aub commented Nov 4, 2016

saneshark commented Nov 4, 2016 •

edited

Loading

mphprogrammer Nov 4, 2016 •

edited

Loading

saneshark Nov 4, 2016 •

edited

Loading

saneshark Nov 4, 2016 •

edited

Loading