389131 - Reporter should move away from SOAP (Reporter fails to send reports on trunk)

Status: VERIFIED FIXED

(Other Applications Graveyard :: Reporter, defect)

Description

[Robert Accettura [:raccettura]]

As a result of bug 332174, we should move the client/server away from soap.  It was on soap since the code was based on another project I was working on at the time (and was soap based).  Now is a good time to move away.

My proposed change was to an xmlHttpRequest sending POST data.  Same data.  Any binary data (such as screenshots, which I still intend to add) would be base64 encoded and sent POST.

Anyone see any caveats int his proposal?

Comment 2

[Carsten Book [:Tomcat]]

Because of this change in Bug 332174 Reporter does not work on Trunk anymore. 

Reporter could be very helpful for Firefox 3 Beta 1 for Feedback of broken/problems with Websites (like as example Banking sites), so i think we should find a way to bring reporter back to working soon.

I think this also something for the Release Notes for the next M8 Release, if reporter is then still broken.

Comment 3

[Robert Accettura [:raccettura]]

Yea, I'm on... I've started work on it already.

Comment 4

[Robert Sayre]

Damon, this should be a blocker

Comment 5

[Damon Sicore (:damons)]

raccettura, progress so far?

Comment 6

[Robert Accettura [:raccettura]]

I've got most of the server side done, and about half of this bug (the client side).

If someone wants to pickup this one to speed things along... awesome, I'll post my current work and they can take it from there.  

I'm in a bad spot the next 1-2 weeks or so limiting my time, hence no patch thus far.

Comment 8

[Robert Accettura [:raccettura]]

Attachment: Patch v1 rough draft

This is a working patch which converts the entire thing to using POST via xmlHttpRequest.  It doesn't work against the current reporter production instance.  The server is bug 389131 (also getting close).  I'll try to get reporter-dev up and running with the updated service shortly for testing purposes.

There's very likely some cleanup still needed in this patch.  If someone wants to drive and either a) just clean it up, or b) review for clean up... cool.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 282879 [details] [diff] [review]
Patch v1 rough draft

Some quick cleanup proposals:

>   var param = new Array();
>+  param['method']               = 'submitReport';
>+  param['rmoVers']              = gRMOvers;
...
>+  param['sysid']                = sysId;

var param = {
  method:   'submitReport',
  rmoVers:  gRMOvers,
...
  sysid:    sysId
}

>+  sendReporterServerData(serviceURL, param, 'onSendReportDataLoad');

sendReporterServerData(param, onSendReportDataLoad);

>+function sendReporterServerData(serviceURL, params, callback) {

function sendReporterServerData(params, callback) {
  var serviceURL = getCharPref("serviceURL", "http://reporter.mozilla.org/service/");

>+  params = serializeParams(params);
>+
>+  var request = new XMLHttpRequest();
>+  request.onprogress = onSendReportDataProgress;
>+  request.open("POST", serviceURL, true);
>+
>+  request.onreadystatechange = function (aEvt) {
>+    if (request.readyState == 4) {
>+      eval(callback + '(request);');
>+    }
>+  };

request.onreadystatechange = function () {
  if (request.readyState == 4)
    callback(request);
};

Comment 11

[Robert Accettura [:raccettura]]

Attachment: Patch v2

Bare with me... lets get this going.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 286504 [details] [diff] [review]
Patch v2

>Index: resources/content/reporter/reportWizard.js
> var gPrefBranch;
>+var gStatusIndicator

lacks a semicolon

>+  var param = {
>+    'method':             'submitRegister',
>+    'language':           gParamLanguage
>+  };

You don't need the apostrophes ahead of the colons; doesn't matter though.

>+function onRegisterSysIDLoad(req){
>+  if (req.status == 200) {
>+    gParamSysID = req.responseXML.getElementsByTagName('result').item(0).firstChild.data;
>+
>+    // saving
>+    if (gParamSysID != undefined){
>+      var prefs = getReporterPrefBranch();
>+      prefs.setCharPref("sysid", gParamSysID);

gParamSysID doesn't need to be global, does it?
The |undefined| check looks wrong to me, |.firstChild| will throw if there's no 'result' element.

How about:

>    var paramSysID = req.responseXML.getElementsByTagName('result').item(0);
>
>    // saving
>    if (paramSysID) {
>      var prefs = getReporterPrefBranch();
>      prefs.setCharPref("sysid", paramSysID.textContent);

>+    // Invalid Response Error
>+    showError('An Invalid response was recieved from the server.  Please try again later.');
>+    return;

something to localize?

>+  // On error
>+  var errorStr = extractError(req);
>+  showError(errorStr);
>+  return;
> }

>+  request.send(params);
>+  return;
>+}

superfluous |return| in both cases

>+function serializeParams(params) {
>+  var str = '';
>+  for (var key in params) {
>+    str += key + '=' + escape(params[key]) + '&';

I don't think |escape| is right here, use encodeURIComponent for UTF-8.

>+function onSendReportDataLoad(req) {
>+  var error = false;
>+
>+  var reportWizard = document.getElementById('reportWizard');
>   var finishSummary = document.getElementById('finishSummary');
>   var finishExtendedFailed = document.getElementById('finishExtendedFailed');
>   var finishExtendedSuccess = document.getElementById('finishExtendedSuccess');
>+  var statusDescription = document.getElementById('sendReportProgressDescription');
> 
>+  if (req.status != 200) {
>+    var errorStr = extractError(req);
>+    showError(errorStr);
>+    return;

You're not using the |error| variable, and the other declarations should probably go beneath the |if|.

>+function extractError(req){
>+  var strbundle = document.getElementById("strings");
>+    // Check the response
>+    if(req.responseXML.getElementsByTagName('errorString').item(0)){
>+      return req.responseXML.getElementsByTagName('errorString').item(0).firstChild.data;
>+    }

-->
>    var error = req.responseXML.getElementsByTagName('errorString').item(0)    
>    if (error)
>      return error.textContent;

and the indention isn't quite right.

>+function showError(errorStr){

>+  // If there was an error from the server
>+  finishExtendedSuccess.setAttribute("class", "hide");

finishExtendedSuccess.className = "hide";

Comment 13

[Robert Accettura [:raccettura]]

Attachment: Patch v3

Addresses comments, also is a complete diff against all changed files (I missed a little big before).

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 286628 [details] [diff] [review]
Patch v3

Looks good to me.


> // Globals
>+var gParamSysID;

This declaration should go away.

>+  // If successful
>+  finishExtendedFailed.setAttribute("class", "hide");

You can also use className here. (Better yet: Set .hidden = true and remove the class from the stylesheet.)
Same for .setAttribute("value",...) vs .value = ...

Comment 15

[Mike Connor [:mconnor]]

Robert, if you can get one last rev with Dao's comments I can review today.

Comment 16

[Robert Accettura [:raccettura]]

I can't hit this until tonight as I'm behind a firewall that isn't cvs friendly.  It is my intention to get this tonight.  Then if you want, you can review and checkin on my behalf.  It doesn't have to wait for 389128 since it's busted both ways.

I'll also update reporter-dev with the patch from 389128 so you can test against that.

Comment 17

[Reed Loden [:reed]]

Once you have final review from mconnor, you can just add the "checkin-needed" keyword, and somebody (probably me) will commit the final reviewed patch for you.

Comment 18

[Robert Accettura [:raccettura]]

Attachment: Patch v4

As promised

Comment 19

[Mike Connor [:mconnor]]

Comment on attachment 286777 [details] [diff] [review]
Patch v4

>+++ locales/en-US/chrome/reportWizard.properties	30 Oct 2007 01:17:59 -0000
>@@ -8,3 +8,6 @@
> 
> successfullyCreatedReport=Successfully Transmitted Report
> failedCreatingReport=There was an error creating the report, and so no information was sent to mozilla.org
>+defaultError=Unable to make a successful connection to the server.
>+invalidResponse=An Invalid response was recieved from the server.  Please try again later.

An invalid response was received from the server.  Please try again later.

> function sendReport() {
>+

random whitespace. whoever lands this, please fix on checkin

>+function sendReporterServerData(params, callback) {
>+  var serviceURL = getCharPref("serviceURL", "http://reporter.mozilla.org/service/");

this should be /service/0.3/ right?

>+function serializeParams(params) {
>+  var str = '';
>+  for (var key in params) {
>+    str += key + '=' + encodeURIComponent(params[key]) + '&';
>+  }
>+  return str.substr(0, str.length-1);
>+}

I hate doing stuff like that, there's never a really clean solution. :)

>+function onSendReportDataLoad(req) {
>+
>+  if (req.status != 200) {
>+

more weird whitespace :)

please make those changes and attach a new patch, and hang a checkin-needed keyword if you can't land yourself.  thanks again!

Comment 20

[Robert Accettura [:raccettura]]

Attachment: For Checkin

This should do it.

As a last note, this will obviously have no real effect until bug 389128 is checked in (to happen now), and pushed live (in a bug to be filed).

I don't have the time to be on the hook ATM and likely won't until Friday evening.  If someone wants to be a pal and checkin, feel free (noting above caveat).  Otherwise I'll take care of it then.

Comment 21

[Reed Loden [:reed]]

Checking in extensions/reporter/locales/en-US/chrome/reportWizard.properties;
/cvsroot/mozilla/extensions/reporter/locales/en-US/chrome/reportWizard.properties,v  <--  reportWizard.properties
new revision: 1.7; previous revision: 1.6
done
Checking in extensions/reporter/resources/content/prefs/reporter.js;
/cvsroot/mozilla/extensions/reporter/resources/content/prefs/reporter.js,v  <--  reporter.js
new revision: 1.4; previous revision: 1.3
done
Checking in extensions/reporter/resources/content/reporter/reportWizard.js;
/cvsroot/mozilla/extensions/reporter/resources/content/reporter/reportWizard.js,v  <--  reportWizard.js
new revision: 1.29; previous revision: 1.28
done
Checking in extensions/reporter/resources/skin/classic/reporter/reportWizard.css;
/cvsroot/mozilla/extensions/reporter/resources/skin/classic/reporter/reportWizard.css,v  <--  reportWizard.css
new revision: 1.5; previous revision: 1.4
done

Comment 22

[Robert Accettura [:raccettura]]

Reed, you're awesome.

Thanks everyone for the last minute push.

For the curious bug 401816 is for deploying the server side.

Comment 23

[[e-mail]]

Comment on attachment 286777 [details] [diff] [review]
Patch v4

>+  return str.substr(0, str.length-1);
return str.slice(0, -1);

Comment 24

[Reed Loden [:reed]]

Attachment: address neil's nit

Comment 26

[Robert Accettura [:raccettura]]

Comment on attachment 286942 [details] [diff] [review]
address neil's nit

simple enough.

r=me

Comment 27

[Reed Loden [:reed]]

Checked-in nit fix.

Checking in extensions/reporter/resources/content/reporter/reportWizard.js;
/cvsroot/mozilla/extensions/reporter/resources/content/reporter/reportWizard.js,v  <--  reportWizard.js
new revision: 1.30; previous revision: 1.29
done

Comment 28

[Carsten Book [:Tomcat]]

verified fixed, see Bug 401816