Closed Bug 1151847 Opened 6 years ago Closed 6 years ago

[Network Alerts][Refactoring] Use notification "data" attribute instead of icon URL query string parameters

Categories

(Firefox OS Graveyard :: Gaia::Network Alerts, defect)

ARM
Gonk (Firefox OS)
defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
2.6 S5 - 1/15

People

(Reporter: azasypkin, Assigned: ilphrin, Mentored)

Details

(Whiteboard: [good first bug][lang=js][mentor-lang=ru])

Attachments

(1 file, 2 obsolete files)

After patch for bug 899585, notification options have dedicated "data" attribute to associate custom data with notification object. So we should stop abusing icon URL attribute to store custom data and start using "data" instead.

Please, see [1] for the place where fix is needed.

[1] https://github.com/mozilla-b2g/gaia/blob/5f4c9281b425f3fdb21e6cd9d29e2ca0a21bffba/apps/network-alerts/js/attention/attention.js#L44
Hi,

I want to work on it, but it is the very first time I try to work on B2G, can I be assigned to this bug?
Assignee: nobody → ilphrin
Flags: needinfo?(azasypkin)
(In reply to Kevin Pellet (Ilphrin) from comment #1)
> Hi,
> 
> I want to work on it, but it is the very first time I try to work on B2G,
> can I be assigned to this bug?

Hey Kevin,

Sure and thanks for your interest!

Since it's your first time contributing to FxOS, I'd recommend you to read through [1] ("The Basics" section) and [2] (for the basic information about "Network Alerts" app). If you have any questions you can ask here or ping me on IRC channels (#gaia-messaging or #fxos).

As mentioned in [2] you'll either need to use WebIDE (with device or built-in simulator) [3] or emulator [4] to see your code in action.

P.S. Please change bug status NEW -> ASSIGNED once you start working on this :)

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Developing_Gaia#The_basics
[2] https://wiki.mozilla.org/Gaia/Network_Alerts
[3] https://mdn.io/webide
[4] https://developer.mozilla.org/en-US/Firefox_OS/Using_the_B2G_emulators
Flags: needinfo?(azasypkin)
Hi Oleg,

Thank you for your links!

I am going to start working on all of this today, so I change the bug status. For now I do not have a lot of questions, but I am pretty sure I will have a lot of question, when I will start using the simulator.
Status: NEW → ASSIGNED
Keywords: checkin-needed
This needs a r+ patch first ;)
Keywords: checkin-needed
if you're looking for a review request, you can display the attachment's page by clicking "Details" besides the attachment. Then you can flip the review flag on the right to "?" with a name besides, like ":azasypkin" in this case.
Oh, I jumped a step sorry x) Thanks I am gonna do that!
Attachment #8673224 - Flags: review?(azasypkin)
Comment on attachment 8673224 [details] [review]
[gaia] Ilphrin:Bug-151847 > mozilla-b2g:master

Thanks for the patch! I've left some comments/guidance at GitHub. You mostly need to update unit tests.

In addition to review request (r?) you can also ask feedback (f?). In Messages we usually use r? when we're sure that patch is ready for review and Treeherder is green :) But if you unsure, or just want to have early feedback on the patch before doing tests and so on - you can ask f? anytime :)
Attachment #8673224 - Flags: review?(azasypkin)
Ok, I saw your comments on Github, I'm currently working on notification.js to understand how all these things works.

And then I will try to fix the unit tests. How can I launch manually these two tests?

Thanks for all your help and patience :D
(In reply to Kevin Pellet (Ilphrin) from comment #9)
> Ok, I saw your comments on Github, I'm currently working on notification.js
> to understand how all these things works.
> 
> And then I will try to fix the unit tests. How can I launch manually these
> two tests?

Please, see [1]. Once browser is launched you'll see the list of available gaia unit tests, your tests are under "Network Alerts" section, you can run all test files or just pick one.

> Thanks for all your help and patience :D

No worries, take you time!

Thanks!

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Automated_testing/Gaia_unit_tests#Launching_the_test_runner_in_Mulet_or_Firefox
Attachment #8673224 - Flags: review?(azasypkin)
Comment on attachment 8673224 [details] [review]
[gaia] Ilphrin:Bug-151847 > mozilla-b2g:master

Left few more comments at GitHub, please ask r? once you're ready!

Thanks!
Attachment #8673224 - Flags: review?(azasypkin)
Hey!

Ok now everything seems (I hope) to be ok =) There is no more unit tests failure on Treeherder, and the notification is showed correctly.
Attachment #8673224 - Flags: review?(azasypkin)
(In reply to Kevin Pellet (Ilphrin) from comment #12)
> Hey!
> 
> Ok now everything seems (I hope) to be ok =) There is no more unit tests
> failure on Treeherder, and the notification is showed correctly.

Hey, sorry for the delay, just returned from PTO. So now everything looks good! Just one nit in unit test left.

Since the nit is tiny you can now rebase your PR on the latest master [1], squash all your commit into one, so that the final commit message will be "Bug 1151847 - [Network Alerts][Refactoring] Use notification "data" attribute instead of icon URL query string parameters. r=azasypkin" and then push PR to GitHub.

I'm keeping r? for now, so just ping me once this nit is handled and we're done :)

Thanks!

[1] https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Submitting_a_Gaia_patch#Tips_on_Gaia_Rebasing
Comment on attachment 8673224 [details] [review]
[gaia] Ilphrin:Bug-151847 > mozilla-b2g:master

Cleaning up my review queue, please set r? once you're back and ready :)
Attachment #8673224 - Flags: review?(azasypkin)
Hi!

I've tried to resolve my git problem but at last I've created a new branch on Github, from where I would ask for a pull request. Can I cancel my last pull request and add a new one? :)
(In reply to Kevin Pellet (Ilphrin) from comment #15)
> Hi!
> 
> I've tried to resolve my git problem but at last I've created a new branch
> on Github, from where I would ask for a pull request. Can I cancel my last
> pull request and add a new one? :)

Hey! Heh, no worries! So just do:
* Close your old PR;
* Mark current bugzilla attachment as "obsolete";
* Create new Pull Request - autolander should automatically attach it to this bug as long as PR name contains bug number;
* Ask r? for newly attached PR :)

Thanks!
Hi!

Thanks for your help! And sorry for the delay, I didn't figured out that you answered ^^"
I've pulled the new branch, in one commit with all the modification of the previous branch.
Attachment #8697763 - Flags: review+
Attachment #8697763 - Flags: review+ → review?(azasypkin)
(In reply to Kevin Pellet (Ilphrin) from comment #18)
> Hi!
> 
> Thanks for your help! And sorry for the delay, I didn't figured out that you
> answered ^^"
> I've pulled the new branch, in one commit with all the modification of the
> previous branch.

Hey! No worries, thanks for the patch :)
(In reply to Oleg Zasypkin [:azasypkin][⏰UTC+1] from comment #19)
> (In reply to Kevin Pellet (Ilphrin) from comment #18)
> > Hi!
> > 
> > Thanks for your help! And sorry for the delay, I didn't figured out that you
> > answered ^^"
> > I've pulled the new branch, in one commit with all the modification of the
> > previous branch.
> 
> Hey! No worries, thanks for the patch :)

If everything is ok, can I put the autoland keyword? ^^
Hey Kevin, we were all traveling during the week-end :) please be a little more patient, I'm sure Oleg will be able to review your patch soon !
Comment on attachment 8697763 [details] [review]
[gaia] Ilphrin:CorrectionBug > mozilla-b2g:master

Sorry, but it seems you have one failing unit test and Treeherder didn't manage to run any test suite for your PR as well + one nit at github.

Could you please fix that (in the same commit, no need for additional one), rebase your PR on the latest master and push again so that Treeherder can pick it up?

Once Treeherder is green please, r? me again (ni? you this time so that you don't miss it:)).

Thanks!
Flags: needinfo?(ilphrin)
Attachment #8697763 - Flags: review?(azasypkin)
(In reply to Oleg Zasypkin [:azasypkin][⏰UTC+1] from comment #22)
> Comment on attachment 8697763 [details] [review]
> [gaia] Ilphrin:CorrectionBug > mozilla-b2g:master
> 
> Sorry, but it seems you have one failing unit test and Treeherder didn't
> manage to run any test suite for your PR as well + one nit at github.
> 
> Could you please fix that (in the same commit, no need for additional one),
> rebase your PR on the latest master and push again so that Treeherder can
> pick it up?
> 
> Once Treeherder is green please, r? me again (ni? you this time so that you
> don't miss it:)).
> 
> Thanks!


Sorry I was very busy these days, I have sent a fix for this. Treeherder says there is still one error but it looks to have nothing to do with this (It is about node).
Flags: needinfo?(ilphrin)
Attachment #8697763 - Flags: review?(azasypkin)
Comment on attachment 8697763 [details] [review]
[gaia] Ilphrin:CorrectionBug > mozilla-b2g:master

> Sorry I was very busy these days, I have sent a fix for this.

No problem, now it looks perfect! Just one tinyyyy nit left. So r+.

> Treeherder says there is still one error but it looks to have
> nothing to do with this (It is about node).

Looks like the reason is that you have very old gaia that depends on old Node version, we recently migrated to Node v4, so here is what is needed to land your patch:

1. Switch to master branch and pull the latest gaia upstream master branch (e.g. git pull upstream master);

2. Switch to your branch with the fix and rebase your PR on top of the master (git rebase -i master). There won't be any conflicts, I've just tried :) 

3. While rebasing please squash your commits into one and set correct commit message for it to reflect what has been fixed and who reviewed the patch - "Bug 1151847 - [Network Alerts][Refactoring] Use notification "data" attribute instead of icon URL query string parameters. r=azasypkin";

4. Push your branch to GitHub so that Treeherder can pick it up and run final set of tests (you should see that a bunch of tests are run, like for any other gaia PR https://treeherder.mozilla.org/#/jobs?repo=gaia).

5. Once Treeherder is green, add "checkin-needed" keyword to the bug and it will be merged for you!

Ping me if you have any questions/doubts or just need some more help.

Thanks a lot for you contribution!
Attachment #8697763 - Flags: review?(azasypkin) → review+
Comment on attachment 8673224 [details] [review]
[gaia] Ilphrin:Bug-151847 > mozilla-b2g:master

Marking old PR as obsolete.
Attachment #8673224 - Attachment is obsolete: true
Attachment #8703328 - Flags: review?(azasypkin)
Attachment #8697763 - Attachment is obsolete: true
Comment on attachment 8703328 [details] [review]
[gaia] Ilphrin:master > mozilla-b2g:master

Looks good, carrying over r+. Let's land this!

Thanks for the contribution :)
Attachment #8703328 - Flags: review?(azasypkin) → review+
Thanks you :D
https://github.com/mozilla-b2g/gaia/commit/64dd32a7e54a49cb6484fc1b20a578eca1448663
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.6 S5 - 1/15
It was long due, thanks so much Kevin ! And congrats for your first patch in Firefox OS ;)
I learned a lot of things about Git thanks to that x) Thanks! :)
You need to log in before you can comment on or make changes to this bug.