acquia_agent's call flow is confusing and suffers from a lack of consistency and sensible expectations. This issue is for working on that consistency a bit.

CommentFileSizeAuthor
#8 2054113-acquia_agent-consistency.patch17.35 KBcoltrane
#6 2054113-acquia_agent-consistency.patch16.28 KBcoltrane
#5 2054113-acquia_agent-consistency.patch10.79 KBcoltrane
#3 2054113-acquia_agent-consistency-d6-do-not-test.patch2.59 KBcoltrane
#1 2054113-acquia_agent-consistency.patch12.09 KBcoltrane

Comments

coltrane’s picture

coltrane
he/him
commented
StatusFileSize
new 2054113-acquia_agent-consistency.patch12.09 KB

Patch cleans up acquia_agent_get_subscription() to return FALSE or an array of active subscription data. Also removes unnecessary variable delete in acquia_agent_check_subscription() and limits variable acquia_subscription_data from containing the xmlrpc error number.

coltrane’s picture

coltrane
he/him
commented
Status: Active » Needs review

I'm considering letting an expired subscription's data live in the variable and moving most subscription checks to use _is_active() but that may be more changes than I want to do.

coltrane’s picture

coltrane
he/him
commented
StatusFileSize
new 2054113-acquia_agent-consistency-d6-do-not-test.patch2.59 KB

D6 patch of #1

coltrane’s picture

coltrane
he/him
commented
Status: Needs review » Needs work

This approach breaks acquia_search_acquia_subscription_status() since it relies on the subscription integers from acquia_agent_get_subscription().

coltrane’s picture

coltrane
he/him
commented
Status: Needs work » Needs review
StatusFileSize
new 2054113-acquia_agent-consistency.patch10.79 KB

Well, to better catch that dependency we need tests! Attached patch adds test for subscription being an int.

coltrane’s picture

coltrane
he/him
commented
StatusFileSize
new 2054113-acquia_agent-consistency.patch16.28 KB

Now testing # of outgoing HTTP requests and not replacing subscription data when server returns timeout (-110) or server error (503).

damiankloip’s picture

damiankloip commented

Changes look sensible to me.

+++ b/acquia_agent/acquia_agent.moduleundefined
@@ -169,11 +169,12 @@ function acquia_agent_theme() {
+  $current_subscription = acquia_agent_settings('acquia_subscription_data');

+++ b/acquia_agent/tests/acquia_agent.testundefined
@@ -154,6 +164,110 @@ class AcquiaAgentTestCase extends DrupalWebTestCase {
+    $this->assertNotIdentical($subscription, '503', 'Subscription is not storing 503.');
+    $this->assertTrue(is_array($subscription), 'Storing subscription array data.');

Do we have data that we can use to test against? So we can also assert that the previous data has remained unchanged?

Oh, and nitpicking...A couple of files need newlines at the end :)

coltrane’s picture

coltrane
he/him
commented
StatusFileSize
new 2054113-acquia_agent-consistency.patch17.35 KB

Yep, I can test that!

coltrane’s picture

coltrane
he/him
commented
Status: Needs review » Fixed

Considering this fixed for now. http://drupalcode.org/project/acquia_connector.git/commit/b8453cb and http://drupalcode.org/project/acquia_connector.git/commit/d99b990

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.