Closed Bug 959033 Opened 7 years ago Closed 3 years ago
Don't send X-Confirm-Delete header
59 bytes, text/x-review-board-request
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.
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-]
This is not a big deal.
tracking-fennec: 31+ → ---
Priority: P2 → P3
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.
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?
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.
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?
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
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?
(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 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+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/01ea7fe459c1 Don't send X-Confirm-Delete header with Android Sync requests. r=Grisha
Thanks for the review Grisha. Unless something goes wrong in the test server, is it correct to say I'm done with this bug?
(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.
Awesome, this wasn't as daunting as I thought it would be. Thanks again for the help Mark and Grisha.
You need to log in before you can comment on or make changes to this bug.