Closed
Bug 959033
Opened 11 years ago
Closed 7 years ago
Don't send X-Confirm-Delete header
Categories
(Firefox for Android Graveyard :: Android Sync, defect, P3)
Firefox for Android Graveyard
Android Sync
Tracking
(firefox57 fixed)
RESOLVED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: rfkelly, Assigned: kimsaehun, Mentored)
References
Details
(Whiteboard: [qa-][good first bug][lang=java])
Attachments
(1 file)
Per the proposed sync1.5 protocol docs in Bug 951986, the sync storage servers will no longer require an "X-Confirm-Delete" header on delete requests. Android sync client should avoid sending this header if it knows it's talking to a sync1.5 server.
Note that it's safe to send the header anyway if you need to, as it will be ignored.
Updated•11 years ago
|
Whiteboard: [qa?]
Comment 1•11 years ago
|
||
N.B., given that the backend client code is shared, this removal either needs to be conditional or to only occur when Sync 1.1 is deprecated.
Marking qa-, 'cos this'll have unit tests and won't be user-facing.
Whiteboard: [qa?] → [qa-]
Updated•11 years ago
|
tracking-fennec: --- → 30+
Priority: -- → P2
Updated•11 years ago
|
tracking-fennec: 30+ → 31+
Comment 3•7 years ago
|
||
This is a great first bug -- delete a line and fix the test.
android/services/src/main/java/org/mozilla/gecko/sync/net/SyncStorageRequest.java
162: request.addHeader("x-confirm-delete", "1");
android/tests/background/junit4/src/org/mozilla/android/sync/net/test/TestSyncStorageRequest.java
253: assertNotNull(request.getValue("x-confirm-delete"));
254: assertEquals("1", request.getValue("x-confirm-delete"));
We only support Sync 1.5 now, so we can just remove this functionality.
Mentor: gkruglov
Summary: Don't send X-Confirm-Delete header to sync1.5 storage servers → Don't send X-Confirm-Delete header
Whiteboard: [qa-] → [qa-][good first bug][lang=java]
Hi, I wanted to start contributing to open source projects and found this bug from BugsAhoy(https://www.joshmatthews.net/bugsahoy/?java=1&unowned=1&simple=1). May I take this task and also receive some guidance on how I can complete this task?
Comment 5•7 years ago
|
||
Hi kimsaehun,
Thanks for taking an interest in this bug. You should follow the instructions at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction (and in this case you will be following the instructions for Android), and get a build of Firefox for Android working. Once you have that working, see comment 3 for the specific changes that need to be made - you then rebuild Firefox with your changes, run the tests to ensure they work, then put your patch up for review.
Please let us know if you run into problems, and consider joining the #introduction channel on IRC where you can ask all kinds of introductory questions.
Comment hidden (mozreview-request) |
Hi Mark, Thanks for the guidance. I believe I followed the instructions correctly and have submitted a patch to this bug. It seems the next step is to ask for a review. To whom should I request this review to? Bugzilla recommends Grisha Kruglov. Would he be the correct person to request this to?
Comment 8•7 years ago
|
||
Thanks for the patch! Grisha is the right person to request review from, and the easiest way to do that is to re-push your patch with a different commit message. If you make it, say, "Bug 959033 - Don't send X-Confirm-Delete header with Android Sync requests. r?grisha", you should find it automatically flags him for review, and if he gives r+, will also be ready to land.
Thanks!
Assignee: nobody → kimsaehun
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
Looks like it worked! Thanks for your help Mark. Is it safe to presume that this is how I can keep on contributing from now on? Find a bug, ask for it to be assigned to me, work on it, and push a patch?
Comment 11•7 years ago
|
||
(In reply to kimsaehun from comment #10)
> Looks like it worked! Thanks for your help Mark. Is it safe to presume that
> this is how I can keep on contributing from now on? Find a bug, ask for it
> to be assigned to me, work on it, and push a patch?
Yep, that sounds great! In general though, instead of immediately asking for it to be assigned to you, you should probably start with either a comment indicating how you intend tackling the bug, or just by uploading a patch - or if you aren't sure, just asking for guidance as you did here. As you have already seen, bugsahoy is a great way to find bugs.
Let me know if I can be more help, and thanks for contributing to Firefox!
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8893619 [details]
Bug 959033 - Don't send X-Confirm-Delete header with Android Sync requests.
https://reviewboard.mozilla.org/r/164704/#review170186
Looks good! Thanks for the patch, kimsaehun! I've pushed it to our test servers, once everything passes there it should be good to land.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b36b0c31e1e
Attachment #8893619 -
Flags: review?(gkruglov) → review+
Comment 13•7 years ago
|
||
Pushed by gkruglov@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/01ea7fe459c1
Don't send X-Confirm-Delete header with Android Sync requests. r=Grisha
Assignee | ||
Comment 14•7 years ago
|
||
Thanks for the review Grisha. Unless something goes wrong in the test server, is it correct to say I'm done with this bug?
Comment 15•7 years ago
|
||
(In reply to kimsaehun from comment #14)
> Thanks for the review Grisha. Unless something goes wrong in the test
> server, is it correct to say I'm done with this bug?
Indeed, all is good. Tests looked good, and I've "landed" your patch yesterday. Comment 13 indicates that it's been pushed into our integration branch. It should land on main mozilla-central branch sometime later today, at which point this bug will be officially marked as FIXED.
Updated•7 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 16•7 years ago
|
||
Awesome, this wasn't as daunting as I thought it would be. Thanks again for the help Mark and Grisha.
Comment 17•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•7 years ago
|
Product: Android Background Services → Firefox for Android
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•