Minor refactor and fix test to be more robust

impl/src/main/java/org/jboss/weld/context/conversation/ConversationImpl.java

@@ -139,7 +139,7 @@ public void end()
    public String getId()
    {
       verifyConversationContextActive();
-      if (!isTransient())
+      if (!_transient)
       {
          return id;
       }

impl/src/main/java/org/jboss/weld/jsf/ConversationAwareViewHandler.java

@@ -42,34 +42,38 @@
  * filter, phase listener, etc) to capture the conversation id and restore the
  * long-running conversation.
  * </p>
- * QUESTION should we do the same for getResourceURL?
- * TODO we should enable a way to disable conversation propagation by URL
  * 
  * @author Dan Allen
+ * @author Pete Muir
  */
 public class ConversationAwareViewHandler extends ViewHandlerWrapper
 {
-   private ViewHandler delegate;
+
+   private final ViewHandler delegate;
+   private final Instance<Context> context;
 
    public ConversationAwareViewHandler(ViewHandler delegate)
    {
       this.delegate = delegate;
+      Container container = Container.instance();
+      this.context = container.deploymentManager().instance().select(Context.class);
    }
 
    /**
     * Allow the delegate to produce the action URL. If the conversation is
     * long-running, append the conversation id request parameter to the query
     * string part of the URL, but only if the request parameter is not already
     * present.
-    *
-    * This covers all cases: form actions, link hrefs, Ajax calls, and redirect URLs. 
+    * 
+    * This covers form actions Ajax calls, and redirect URLs (which we want) and
+    * link hrefs (which we don't)
     * 
     * @see {@link ViewHandler#getActionURL(FacesContext, String)}
     */
    @Override
    public String getActionURL(FacesContext facesContext, String viewId)
    {
-      ConversationContext conversationContext = instance().select(HttpConversationContext.class).get();
+      ConversationContext conversationContext = context.select(HttpConversationContext.class).get();
       String actionUrl = super.getActionURL(facesContext, viewId);
       Conversation conversation = conversationContext.getCurrentConversation();
       if (!conversation.isTransient())
@@ -82,18 +86,10 @@ public String getActionURL(FacesContext facesContext, String viewId)
       }
    }
 
-   /**
-    * @see {@link ViewHandlerWrapper#getWrapped()}
-    */
    @Override
    public ViewHandler getWrapped()
    {
       return delegate;
    }
-
-   private static Instance<Context> instance()
-   {
-      return Container.instance().deploymentManager().instance().select(Context.class);
-   }
 
 }

impl/src/main/java/org/jboss/weld/jsf/FacesUrlTransformer.java

@@ -43,15 +43,38 @@ public FacesUrlTransformer(String url, FacesContext facesContext)
    }
 
    public FacesUrlTransformer appendConversationIdIfNecessary(String cidParameterName, String cid)
+   {
+      this.url = appendParameterIfNeeded(url, cidParameterName, cid);
+      return this;
+   }
+
+   private static String appendParameterIfNeeded(String url, String parameterName, String parameterValue)
    {
       int queryStringIndex = url.indexOf(QUERY_STRING_DELIMITER);
-      // if there is no query string or there is a query string but the cid param is absent, then append it
-      if (queryStringIndex < 0 || url.indexOf(cidParameterName + PARAMETER_ASSIGNMENT_OPERATOR, queryStringIndex) < 0)
+      // if there is no query string or there is a query string but the param is
+      // absent, then append it
+      if (queryStringIndex < 0 || url.indexOf(parameterName + PARAMETER_ASSIGNMENT_OPERATOR, queryStringIndex) < 0)
       {
-         url = new StringBuilder(url).append(queryStringIndex < 0 ? QUERY_STRING_DELIMITER : PARAMETER_PAIR_DELIMITER)
-            .append(cidParameterName).append(PARAMETER_ASSIGNMENT_OPERATOR).append(cid).toString();
+         StringBuilder builder = new StringBuilder(url);
+         if (queryStringIndex < 0)
+         {
+            builder.append(QUERY_STRING_DELIMITER);
+         }
+         else
+         {
+            builder.append(PARAMETER_PAIR_DELIMITER);
+         }
+         builder.append(parameterName).append(PARAMETER_ASSIGNMENT_OPERATOR);
+         if (parameterValue != null)
+         {
+            builder.append(parameterValue);
+         }
+         return builder.toString();
+      }
+      else
+      {
+         return url;
       }
-      return this;
    }
 
    public String getUrl()

tests-arquillian/src/test/java/org/jboss/weld/tests/contexts/conversation/exceptionInPhaseListener/ClientConversationContextTest.java

@@ -46,7 +46,6 @@
 import org.jboss.shrinkwrap.api.asset.EmptyAsset;
 import org.jboss.shrinkwrap.api.spec.WebArchive;
 import org.jboss.weld.tests.category.Integration;
-import org.junit.Assert;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
 import org.junit.runner.RunWith;
@@ -103,11 +102,12 @@ public void testExceptionPhaseListener() throws Exception
       String cloudName = getFirstMatchingElement(cloud, HtmlSpan.class, "cloudName").getTextContent();
       assertEquals(Cloud.NAME, cloudName);
 
-      // Now start a conversation and access the page that throws an exception again
+      // Now start a conversation
       HtmlPage thunderstorm = getFirstMatchingElement(cloud, HtmlSubmitInput.class, "beginConversation").click();
-      // This page will error
-
       String cid = getCid(thunderstorm);
+
+      //  and access the page that throws an exception again
+      getFirstMatchingElement(cloud, HtmlSubmitInput.class, "thunderstorm").click();
 
       cloud = client.getPage(getPath("/cloud.jsf", cid));
 

tests-arquillian/src/test/java/org/jboss/weld/tests/contexts/conversation/exceptionInPhaseListener/Cloud.java

@@ -21,7 +21,6 @@
 import javax.annotation.PreDestroy;
 import javax.enterprise.context.Conversation;
 import javax.enterprise.context.ConversationScoped;
-import javax.enterprise.inject.Default;
 import javax.inject.Inject;
 import javax.inject.Named;
 
@@ -85,11 +84,16 @@ public void setName(String name)
 
    public String thunderstorm()
    {
-      conversation.begin();
-      setName("gavin");
       return "thunder";
    }
 
+   public String beginConversation()
+   {
+      setName("gavin");
+      conversation.begin();
+      return "conversationBegun";
+   }
+
    public String hailstorm()
    {
       conversation.begin();

tests-arquillian/src/test/resources/org/jboss/weld/tests/contexts/conversation/exceptionInPhaseListener/cloud.jsf

@@ -16,7 +16,10 @@
    <f:view>
       <h:outputText value="#{cloud.name}" id="cloudName" />
       <h:form>
-         <h:commandButton action="#{cloud.thunderstorm}" value="Start Conversation" id="beginConversation" />
+         <h:commandButton action="#{cloud.beginConversation}" value="Start Conversation" id="beginConversation" />
+      </h:form>
+      <h:form>
+         <h:commandButton action="#{cloud.thunderstorm}" value="Start Conversation" id="thunderstorm" />
       </h:form>
    </f:view>
   </body>

tests-arquillian/src/test/resources/org/jboss/weld/tests/contexts/conversation/exceptionInPhaseListener/faces-config.xml

@@ -15,5 +15,14 @@
          <to-view-id>/thunderstorm.jspx</to-view-id>
       </navigation-case>
    </navigation-rule>
+
+   <navigation-rule>
+      <from-view-id>/cloud.jspx</from-view-id>
+      <navigation-case>
+         <redirect/>
+         <from-action>#{cloud.beginConversation}</from-action>
+         <to-view-id>/cloud.jspx</to-view-id>
+      </navigation-case>
+   </navigation-rule>
 
 </faces-config>