Closed Bug 389131 Opened 17 years ago Closed 17 years ago

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

Categories

(Other Applications Graveyard :: Reporter, defect)

defect
Not set
major

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.9beta1

People

(Reporter: raccettura, Assigned: raccettura)

References

Details

(Whiteboard: [has patch][has reviews])

Attachments

(2 files, 4 obsolete files)

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?
Status: NEW → ASSIGNED
Depends on: 389128
Flags: blocking1.9?
Severity: normal → major
Summary: Reporter should move away from SOAP → Reporter should move away from SOAP (Reporter fails to send reports on trunk)
Version: unspecified → Trunk
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.
Yea, I'm on... I've started work on it already.
Damon, this should be a blocker
Target Milestone: --- → M9
Target Milestone: M9 → mozilla1.9beta
raccettura, progress so far?
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.
Attached patch Patch v1 rough draft (obsolete) — Splinter Review
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.
Depends on: 400563
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);
};
Attached patch Patch v2 (obsolete) — Splinter Review
Bare with me... lets get this going.
Attachment #282879 - Attachment is obsolete: true
Attachment #286504 - Flags: review?(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";
Attachment #286504 - Flags: review?(dao) → review-
Attached patch Patch v3 (obsolete) — Splinter Review
Addresses comments, also is a complete diff against all changed files (I missed a little big before).
Attachment #286504 - Attachment is obsolete: true
Attachment #286628 - Flags: review?
Attachment #286628 - Flags: review? → review?(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 = ...
Attachment #286628 - Flags: review?(dao) → review+
Robert, if you can get one last rev with Dao's comments I can review today.
Flags: blocking1.9? → blocking1.9+
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.
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.
Attached patch Patch v4 (obsolete) — Splinter Review
As promised
Attachment #286628 - Attachment is obsolete: true
Attachment #286777 - Flags: review?(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!
Attachment #286777 - Flags: review?(mconnor) → review+
Whiteboard: [has patch][need final patch for landing]
Attached patch For CheckinSplinter Review
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.
Attachment #286777 - Attachment is obsolete: true
Whiteboard: [has patch][need final patch for landing] → [has patch][checkin-needed]
Keywords: checkin-needed
Whiteboard: [has patch][checkin-needed] → [has patch]
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch] → [has patch][has reviews]
Reed, you're awesome.

Thanks everyone for the last minute push.

For the curious bug 401816 is for deploying the server side.
Comment on attachment 286777 [details] [diff] [review]
Patch v4

>+  return str.substr(0, str.length-1);
return str.slice(0, -1);
Attachment #286942 - Flags: review?(robert)
Attachment #286942 - Flags: review?(neil)
Attachment #286942 - Flags: review?(neil) → review+
Comment on attachment 286942 [details] [diff] [review]
address neil's nit

simple enough.

r=me
Attachment #286942 - Flags: review?(robert) → review+
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
verified fixed, see Bug 401816
Status: RESOLVED → VERIFIED
Product: Other Applications → Other Applications Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: