Uploaded image for project: 'OFBiz'
  1. OFBiz
  2. OFBIZ-5279

Wrong to use !jQuery('#content-messages') in \framework\images\webapp\images\selectall.js line #354

    Details

      Description

      I think we should use

      if(jQuery('#content-messages').length==0)

      insead of

      if(!jQuery('#content-messages'))

      in framework\images\webapp\images\selectall.js line #354.

      1. patches.zip
        1 kB
        Wei Zhang

        Issue Links

          Activity

          Hide
          tzngvi Wei Zhang added a comment -
          Show
          tzngvi Wei Zhang added a comment - in addition, please refer to http://www.paulund.co.uk/check-to-see-if-a-element-exists-in-jquery
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          I totally agree with you, I missed this when I committed for OFBIZ-4022. It's now committed in trunk at revision: 1506274, and will backport in releases when OFBIZ-4794 will de done (so I don't close now), thanks!

          Show
          jacques.le.roux Jacques Le Roux added a comment - I totally agree with you, I missed this when I committed for OFBIZ-4022 . It's now committed in trunk at revision: 1506274, and will backport in releases when OFBIZ-4794 will de done (so I don't close now), thanks!
          Hide
          tzngvi Wei Zhang added a comment -

          In addition the hideErrorContainer function in line #349 cannot hide the error message. Maybe we should use jQuery('#content-messages').html('');

          Show
          tzngvi Wei Zhang added a comment - In addition the hideErrorContainer function in line #349 cannot hide the error message. Maybe we should use jQuery('#content-messages').html('');
          Hide
          tzngvi Wei Zhang added a comment -

          Hi Jacques,

          There is a Syntax Error in line #345, you missed "{".

          Regards,

          Wei

          Show
          tzngvi Wei Zhang added a comment - Hi Jacques, There is a Syntax Error in line #345, you missed "{". Regards, Wei
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Oops, indeed, thanks Wei Zhang, fixed at revision 1506418.

          Show
          jacques.le.roux Jacques Le Roux added a comment - Oops, indeed, thanks Wei Zhang, fixed at revision 1506418.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Wei Zhang,

          I did not test, about your 1st comment

          In addition the hideErrorContainer function in line #349 cannot hide the error message. Maybe we should use jQuery('#content-messages').html('');

          is it still true after the typo fix?

          Show
          jacques.le.roux Jacques Le Roux added a comment - Wei Zhang, I did not test, about your 1st comment In addition the hideErrorContainer function in line #349 cannot hide the error message. Maybe we should use jQuery('#content-messages').html(''); is it still true after the typo fix?
          Hide
          tzngvi Wei Zhang added a comment - - edited

          Yes, the statement below in line #350 cannot hide message.

          jQuery('#content-messages').removeClass('errorMessage').fadeIn('fast')

          Show
          tzngvi Wei Zhang added a comment - - edited Yes, the statement below in line #350 cannot hide message. jQuery('#content-messages').removeClass('errorMessage').fadeIn('fast')
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Did you test
          jQuery('#content-messages').html('');
          ?

          Show
          jacques.le.roux Jacques Le Roux added a comment - Did you test jQuery('#content-messages').html(''); ?
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          BTW there is the same line at line 371, I guess what you suggest should work, but I have no time to test, please confirm, thanks!

          Show
          jacques.le.roux Jacques Le Roux added a comment - BTW there is the same line at line 371, I guess what you suggest should work, but I have no time to test, please confirm, thanks!
          Hide
          tzngvi Wei Zhang added a comment -

          Yes, I tested. and I think we also need to change line #371. So the correct function should be

          function ajaxSubmitFormUpdateAreas(form, areaCsvString) {
             waitSpinnerShow();
             hideErrorContainer = function() {
          		jQuery('#content-messages').html('');
                 jQuery('#content-messages').removeClass('errorMessage').fadeIn('fast');
             }
             updateFunction = function(data) {
                 if (data._ERROR_MESSAGE_LIST_ != undefined || data._ERROR_MESSAGE_ != undefined) {
                     if (!jQuery('#content-messages').length) {
                        //add this div just after app-navigation
                        if(jQuery('#content-main-section')){
                            jQuery('#content-main-section' ).before('<div id="content-messages" onclick="hideErrorContainer()"></div>');
                        }
                     }
                     jQuery('#content-messages').addClass('errorMessage');
                    if (data._ERROR_MESSAGE_LIST_ != undefined && data._ERROR_MESSAGE_ != undefined) {
                        jQuery('#content-messages' ).html(data._ERROR_MESSAGE_LIST_ + " " + data._ERROR_MESSAGE_);
                    } else if (data._ERROR_MESSAGE_LIST_ != undefined) {
                        jQuery('#content-messages' ).html(data._ERROR_MESSAGE_LIST_);
                    } else {
                        jQuery('#content-messages' ).html(data._ERROR_MESSAGE_);
                    }
                    jQuery('#content-messages').fadeIn('fast');
                 }else {
                     if(jQuery('#content-messages').length) {
          			   jQuery('#content-messages').html('');
                         jQuery('#content-messages').removeClass('errorMessage').fadeIn("fast");
                     }
                     ajaxUpdateAreas(areaCsvString);
                 }
                 waitSpinnerHide();
             }
          
             jQuery.ajax({
                 type: "POST",
                 url: jQuery("#" + form).attr("action"),
                 data: jQuery("#" + form).serialize(),
                 success: function(data) {
                         updateFunction(data);
                 }
             });
          }
          
          Show
          tzngvi Wei Zhang added a comment - Yes, I tested. and I think we also need to change line #371. So the correct function should be function ajaxSubmitFormUpdateAreas(form, areaCsvString) { waitSpinnerShow(); hideErrorContainer = function() { jQuery('#content-messages').html(''); jQuery('#content-messages').removeClass('errorMessage').fadeIn('fast'); } updateFunction = function(data) { if (data._ERROR_MESSAGE_LIST_ != undefined || data._ERROR_MESSAGE_ != undefined) { if (!jQuery('#content-messages').length) { //add this div just after app-navigation if (jQuery('#content-main-section')){ jQuery('#content-main-section' ).before('<div id= "content-messages" onclick= "hideErrorContainer()" ></div>'); } } jQuery('#content-messages').addClass('errorMessage'); if (data._ERROR_MESSAGE_LIST_ != undefined && data._ERROR_MESSAGE_ != undefined) { jQuery('#content-messages' ).html(data._ERROR_MESSAGE_LIST_ + " " + data._ERROR_MESSAGE_); } else if (data._ERROR_MESSAGE_LIST_ != undefined) { jQuery('#content-messages' ).html(data._ERROR_MESSAGE_LIST_); } else { jQuery('#content-messages' ).html(data._ERROR_MESSAGE_); } jQuery('#content-messages').fadeIn('fast'); } else { if (jQuery('#content-messages').length) { jQuery('#content-messages').html(''); jQuery('#content-messages').removeClass('errorMessage').fadeIn( "fast" ); } ajaxUpdateAreas(areaCsvString); } waitSpinnerHide(); } jQuery.ajax({ type: "POST" , url: jQuery( "#" + form).attr( "action" ), data: jQuery( "#" + form).serialize(), success: function(data) { updateFunction(data); } }); }
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Could you please rather provide a patch based on trunk HEAD?

          Except if you confirme that I just have to add the line

          jQuery('#content-messages').html('');

          above the 2
          jQuery('#content-messages').removeClass('errorMessage').fadeIn("fast");
          lines

          right?

          Show
          jacques.le.roux Jacques Le Roux added a comment - Could you please rather provide a patch based on trunk HEAD? Except if you confirme that I just have to add the line jQuery('#content-messages').html(''); above the 2 jQuery('#content-messages').removeClass('errorMessage').fadeIn("fast"); lines right?
          Hide
          tzngvi Wei Zhang added a comment -

          Please see attached patch

          Show
          tzngvi Wei Zhang added a comment - Please see attached patch
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Thanks Wei Zhang,

          I have completed by hand using your patch as a basis. Completed: At revision: 1506499

          Next time please follow https://cwiki.apache.org/confluence/display/OFBADMIN/OFBiz+Contributors+Best+Practices

          Thanks!

          Show
          jacques.le.roux Jacques Le Roux added a comment - Thanks Wei Zhang, I have completed by hand using your patch as a basis. Completed: At revision: 1506499 Next time please follow https://cwiki.apache.org/confluence/display/OFBADMIN/OFBiz+Contributors+Best+Practices Thanks!
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          I committed a typo fix at revision 1506504. Another reason to follow the normal way for patches

          Show
          jacques.le.roux Jacques Le Roux added a comment - I committed a typo fix at revision 1506504. Another reason to follow the normal way for patches
          Hide
          tzngvi Wei Zhang added a comment -

          Hi Jacques,

          The line below in line #371 was not changed correctly

          if (jQuery('#content-messages').length == 0)
          

          It should be(It should be opposite of line #355)

          if (jQuery('#content-messages').length)
          

          or

          if (jQuery('#content-messages').length>0)
          

          Thanks,

          Wei

          Show
          tzngvi Wei Zhang added a comment - Hi Jacques, The line below in line #371 was not changed correctly if (jQuery('#content-messages').length == 0) It should be(It should be opposite of line #355) if (jQuery('#content-messages').length) or if (jQuery('#content-messages').length>0) Thanks, Wei
          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          Thanks Wai Zhang,

          I should have taken this fix more seriously (I was focused on smthg else). BTW really next time use a real patch, but yes my bad, thanks for review!

          Committed at revision 1506828.

          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited Thanks Wai Zhang, I should have taken this fix more seriously (I was focused on smthg else). BTW really next time use a real patch, but yes my bad, thanks for review! Committed at revision 1506828.
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          I put as resolved/incomplete to facilitate searches with "My resolved pending" own filter

          Show
          jacques.le.roux Jacques Le Roux added a comment - I put as resolved/incomplete to facilitate searches with "My resolved pending" own filter
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Backported in
          R13.07 r1533495 + r1533500 + r1533508
          R12.04 r1533496 + r1533501 + r1533509
          R11.04 r1533497 + r1533502 + r1533510

          Show
          jacques.le.roux Jacques Le Roux added a comment - Backported in R13.07 r1533495 + r1533500 + r1533508 R12.04 r1533496 + r1533501 + r1533509 R11.04 r1533497 + r1533502 + r1533510
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Need to clean the Jira situation here

          Show
          jacques.le.roux Jacques Le Roux added a comment - Need to clean the Jira situation here
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Done!

          Show
          jacques.le.roux Jacques Le Roux added a comment - Done!

            People

            • Assignee:
              jacques.le.roux Jacques Le Roux
              Reporter:
              tzngvi Wei Zhang
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development