commit to add connected devices #2312
Conversation
farhan787
changed the title
|
@vbudhram your review please. |
There was a problem hiding this comment.
Thanks @farhan787, left a couple comments on things that need to be fixed.
| .then(testElementExists(selectors.SECURITY_EVENTS.CONNECTED_DEVICES)); | ||
| }, | ||
|
|
||
| 'gets at least one connected device': function() { |
There was a problem hiding this comment.
Lets combine this with the test above and make a new test that logs into 123Done or another client. You should be able to test that the service appears in the connected devices and apps.
packages/fxa-content-server/app/scripts/templates/security_events.mustache
Outdated
Show resolved
Hide resolved
|
@vbudhram and @farhan787 - what more needs to happen with this PR to get it merged? |
|
@shane-tomlinson The only thing that needs to be done are getting the tests updated. @farhan787 was stuck on it and I was going to try and unstick him. |
|
@farhan787 Please take a look at 7555ba9, this the logic need to correctly unit test security events. You can pull the commit into this PR or as needed. The main issue I found was that you needed to call the |
@vbudhram thanks for helping me out but is it compulsory to call the |
Yea should be fine since in this case you are explicitly testing logic in the |
|
@vbudhram could you review it please ? |
|
Hey @vbudram - mind reviewing these? |
|
@vladikoff why is this closed ? |
|
@vbudhram could you please look into this, if I should make a new PR for this ? Is there something wrong with this work ? |
|
Hey @farhan787 Unfortunately, this feature got pushed to beginning next year. I don't think there is anything else for you to do. Once this year bugs get completed, I will circle back to it. Thanks again for helping with this! |
|
@vbudhram I'd like to continue working on this, I can ping you in January 2020 and complete this one if that's ok ? |
@vbudhram connected devices table...