OFBiz
  1. OFBiz
  2. OFBIZ-4822

eCommerce Profile Improve Button Naming Consistency

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: SVN trunk
    • Fix Version/s: SVN trunk
    • Labels:
      None
    • Environment:

      demo-trunk

      Description

      This follows from OFBIZ-4814 bug fix.

      In eCommerce the naming and display of back buttons is inconsistent across the Profile Editing screens.
      Example:
      Changes Password label should be "Go Back" but is "[Go Back]"

        Activity

        Hide
        Tom Burns added a comment -

        Jacques,

        I did test viewprofile without authview and I did not see an impact.
        I'll submit a new issue as an improvement.

        Tom

        Show
        Tom Burns added a comment - Jacques, I did test viewprofile without authview and I did not see an impact. I'll submit a new issue as an improvement. Tom
        Hide
        Jacques Le Roux added a comment -

        Tom,

        authview quickly fixed for now at r1327562
        Interesting:

        That said the authview is used inconsistently in eCommerce and only in the ftl files in the Customer folder.
        It calls the authview request in the controller which sets https before calling main view.
        The donePage call is set to viewprofile, which sets https, from all of the edit forms off viewprofile.
        So authview does not appear to be adding any value. The code would be clearer without it.

        I tend to agree from what yo are saying (no review I mean). Would you mind to provide a patch to remove it, did you test it?

        It is not completely clear to me where donePage is picking up is value.

        I had a look before committing, since this is in the profile screen, and at the top of it there is a call to the editperson screen, then it's intially set there with default-value="viewprofile"

        I agree with your 3 points, good catches. I saw also that you put the buttons at top and bottom where they were missing (maybe it was you say in point 3. BTW...)

        Show
        Jacques Le Roux added a comment - Tom, authview quickly fixed for now at r1327562 Interesting: That said the authview is used inconsistently in eCommerce and only in the ftl files in the Customer folder. It calls the authview request in the controller which sets https before calling main view. The donePage call is set to viewprofile, which sets https, from all of the edit forms off viewprofile. So authview does not appear to be adding any value. The code would be clearer without it. I tend to agree from what yo are saying (no review I mean). Would you mind to provide a patch to remove it, did you test it? It is not completely clear to me where donePage is picking up is value. I had a look before committing, since this is in the profile screen, and at the top of it there is a call to the editperson screen, then it's intially set there with default-value="viewprofile" I agree with your 3 points, good catches. I saw also that you put the buttons at top and bottom where they were missing (maybe it was you say in point 3. BTW...)
        Hide
        Tom Burns added a comment -

        Jacques,

        I did not intentionally remove it.
        Likely it was a sloppy cut an paste of the "Go Back" line from one of the other files.

        That said the authview is used inconsistently in eCommerce and only in the ftl files in the Customer folder.
        It calls the authview request in the controller which sets https before calling main view.
        The donePage call is set to viewprofile, which sets https, from all of the edit forms off viewprofile.
        So authview does not appear to be adding any value. The code would be clearer without it.

        I could be missing something in the logic.
        It is not completely clear to me where donePage is picking up is value.
        I see it set in Customer Screens > editperson, ChangePassword.groovy and some of the form tags but nothing consistent.

        Some things that were intentional:

        1. For consistency I changed the behavior of editperson form. It returned to viewprofile on save click while all other edit forms required clicking on the done button.

        2. Placed a paragraph after the buttons on the top line to separate them from the horizontal table lines. The connection to the top horizontal line made the form look like a tab form which is misleading.

        3. For consistency modified the newcustomer form buttons, though it is outside of the Profile feature, since they serve the same function.

        Tom

        Show
        Tom Burns added a comment - Jacques, I did not intentionally remove it. Likely it was a sloppy cut an paste of the "Go Back" line from one of the other files. That said the authview is used inconsistently in eCommerce and only in the ftl files in the Customer folder. It calls the authview request in the controller which sets https before calling main view. The donePage call is set to viewprofile, which sets https, from all of the edit forms off viewprofile. So authview does not appear to be adding any value. The code would be clearer without it. I could be missing something in the logic. It is not completely clear to me where donePage is picking up is value. I see it set in Customer Screens > editperson, ChangePassword.groovy and some of the form tags but nothing consistent. Some things that were intentional: 1. For consistency I changed the behavior of editperson form. It returned to viewprofile on save click while all other edit forms required clicking on the done button. 2. Placed a paragraph after the buttons on the top line to separate them from the horizontal table lines. The connection to the top horizontal line made the form look like a tab form which is misleading. 3. For consistency modified the newcustomer form buttons, though it is outside of the Profile feature, since they serve the same function. Tom
        Hide
        Jacques Le Roux added a comment -

        Tom,

        double checking I found

        Modified: ofbiz/trunk/specialpurpose/ecommerce/webapp/ecommerce/customer/changepassword.ftl
        URL: http://svn.apache.org/viewvc/ofbiz/trunk/specialpurpose/ecommerce/webapp/ecommerce/customer/changepassword.ftl?rev=1327269&r1=1327268&r2=1327269&view=diff
        ==============================================================================
        --- ofbiz/trunk/specialpurpose/ecommerce/webapp/ecommerce/customer/changepassword.ftl (original)
        +++ ofbiz/trunk/specialpurpose/ecommerce/webapp/ecommerce/customer/changepassword.ftl Tue Apr 17 20:12:11 2012
        @@ -17,9 +17,10 @@ specific language governing permissions 
         under the License.
         -->
         <div class="screenlet">
        -  <h2>${uiLabelMap.PartyChangePassword}</h2>
        -  <a href="<@ofbizUrl>authview/${donePage}</@ofbizUrl>" class="button">[${uiLabelMap.CommonGoBack}]</a>
        -  <a href="javascript:document.getElementById('changepasswordform').submit()" class="button">[${uiLabelMap.CommonSave}]</a>
        +  <h2>${uiLabelMap.PartyChangePassword}</h2>  
        +  &nbsp;<a href="<@ofbizUrl>${donePage}</@ofbizUrl>" class="button">${uiLabelMap.CommonGoBack}</a>
        +  &nbsp;<a href="javascript:document.getElementById('changepasswordform').submit()" class="button">${uiLabelMap.CommonSave}</a>
        

        I guess the authview/ removing was not intended, right?

        Show
        Jacques Le Roux added a comment - Tom, double checking I found Modified: ofbiz/trunk/specialpurpose/ecommerce/webapp/ecommerce/customer/changepassword.ftl URL: http: //svn.apache.org/viewvc/ofbiz/trunk/specialpurpose/ecommerce/webapp/ecommerce/customer/changepassword.ftl?rev=1327269&r1=1327268&r2=1327269&view=diff ============================================================================== --- ofbiz/trunk/specialpurpose/ecommerce/webapp/ecommerce/customer/changepassword.ftl (original) +++ ofbiz/trunk/specialpurpose/ecommerce/webapp/ecommerce/customer/changepassword.ftl Tue Apr 17 20:12:11 2012 @@ -17,9 +17,10 @@ specific language governing permissions under the License. --> <div class= "screenlet" > - <h2>${uiLabelMap.PartyChangePassword}</h2> - <a href= "<@ofbizUrl>authview/${donePage}</@ofbizUrl>" class= "button" >[${uiLabelMap.CommonGoBack}]</a> - <a href= "javascript:document.getElementById('changepasswordform').submit()" class= "button" >[${uiLabelMap.CommonSave}]</a> + <h2>${uiLabelMap.PartyChangePassword}</h2> + &nbsp;<a href= "<@ofbizUrl>${donePage}</@ofbizUrl>" class= "button" >${uiLabelMap.CommonGoBack}</a> + &nbsp;<a href= "javascript:document.getElementById('changepasswordform').submit()" class= "button" >${uiLabelMap.CommonSave}</a> I guess the authview/ removing was not intended, right?
        Hide
        Jacques Le Roux added a comment -

        Thanks Tom,

        Your patch is in trunk at revision: 1327269.

        You should not have included the editcontactmech.ftl changes since you already included them in the button fix. Anyway not a big deal I easily bypassed the conflict

        Show
        Jacques Le Roux added a comment - Thanks Tom, Your patch is in trunk at revision: 1327269. You should not have included the editcontactmech.ftl changes since you already included them in the button fix. Anyway not a big deal I easily bypassed the conflict
        Hide
        Tom Burns added a comment -

        Jacques,

        Please review carefully. I found some errors in the earlier patch where the javascript submits were not correct.

        Thanks,

        Tom

        Show
        Tom Burns added a comment - Jacques, Please review carefully. I found some errors in the earlier patch where the javascript submits were not correct. Thanks, Tom

          People

          • Assignee:
            Jacques Le Roux
            Reporter:
            Tom Burns
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development