Closed Bug 1412359 Opened 7 years ago Closed 7 years ago

Data confusion when removing custom devices

Categories

(DevTools :: Responsive Design Mode, defect, P1)

Product:
DevTools
Component:
Responsive Design Mode
Type:
defect

Tracking

(firefox56 wontfix, firefox57 wontfix, firefox58 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- verified

People

(Reporter: jryans, Assigned: jryans)

References

Details

(Whiteboard: [designer-tools])

Attachments

(2 files)

Bug 1412359 - Clarify test event as device-association-removed.
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1412359 - Filter to matching device on remove.
59 bytes, text/x-review-board-request
gl
: review+
Details 
Something goes very wrong when removing custom devices:

STR:

1. Start RDM and enter device modal (device drop down, edit list)
2. Create a new device "1" (details unimportant)
3. Create a new device "2"
4. Click done to exit modal
5. Check device drop down
  * Observe that both new devices are shown
6. Enter device modal again
7. Click X next to device "2" to delete it
8. Click done to exit modal
9. Check device drop down
  * Observe that 2 is gone, and 1 is still there
10. Exit RDM and re-enter RDM
11. Check device drop down
  * Observe that both 1 and 2 are gone somehow!
12. Enter device modal
  * Observe that 1 is somehow gone, but 2 is back and disabled!

ER:

When exiting and re-entering RDM, the devices should have remained unchanged:

* Device 1 should still be present and enabled
* Device 2 should not be present

AR:

Instead we find in the modal:

* Device 1 is somehow removed
* Device 2 is back from the dead, but disabled
Flags: qe-verify+
This appears to be a long standing issue (not a regression), and it's late in the beta cycle, so I expect we'll leave 56 and 57 alone.
status-firefox56: --- → wontfix
Comment on attachment 8923543 [details]
Bug 1412359 - Clarify test event as device-association-removed.

https://reviewboard.mozilla.org/r/194664/#review199860
Attachment #8923543 - Flags: review?(gl) → review+
Comment on attachment 8923544 [details]
Bug 1412359 - Filter to matching device on remove.

https://reviewboard.mozilla.org/r/194666/#review199864

::: devtools/client/shared/devices.js:85
(Diff revision 1)
>    let list = localDevices[type];
>    if (!list) {
>      return false;
>    }
>  
> -  let index = list.findIndex(item => device);
> +  let index = list.findIndex(matcher);

Couldn't we have done: 

list.findIndex(entry => entry.name == device.name) and just pass in removeDevice(device, type) as usual?
Attachment #8923544 - Flags: review?(gl)
Comment on attachment 8923544 [details]
Bug 1412359 - Filter to matching device on remove.

https://reviewboard.mozilla.org/r/194666/#review199864

> Couldn't we have done: 
> 
> list.findIndex(entry => entry.name == device.name) and just pass in removeDevice(device, type) as usual?

Yes, that would be equivalent code.  I made it a callback instead because I was thinking about who "owns" validating that the device names are unique.  At the moment, `devtools/client/shared/devices.js` doesn't validate names when adding or check them when removing.  The only validation today happens at the "application" level inside RDM: the device adder checks the name for uniqueness at the UI level[1].

Mainly, I'd like the validation to be "balanced" so that the same module is checking the unique name constraint.  Since it's probably same to assume everyone wants unique names, I'll fold it back into remove like you suggest, but then also have adding a device enforce it, so it's balanced.

[1]: http://searchfox.org/mozilla-central/rev/1c4901d060e3953e41088c038b62a0c026c1e1fb/devtools/client/responsive.html/components/DeviceAdder.js#65-68
Comment on attachment 8923544 [details]
Bug 1412359 - Filter to matching device on remove.

https://reviewboard.mozilla.org/r/194666/#review201262

::: devtools/client/responsive.html/test/browser/browser_device_custom_remove.js:6
(Diff revision 2)
> +/* Any copyright is dedicated to the Public Domain.
> +http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Test adding several devices and removing one, to ensure the correct device is removed.

I don't think we need the comma here.

::: devtools/client/responsive.html/test/browser/browser_device_custom_remove.js:8
(Diff revision 2)
> +
> +"use strict";
> +
> +// Test adding several devices and removing one, to ensure the correct device is removed.
> +
> +const device = {

It seems like the other test defines TEST_URL and Types before other constants needed for the unit test. Let's move device, device1, device2, below Types.

::: devtools/client/responsive.html/test/browser/browser_device_custom_remove.js:68
(Diff revision 2)
> +
> +  info("Verify all custom devices default to enabled in modal");
> +  let deviceCbs =
> +    [...document.querySelectorAll(".device-type-custom .device-input-checkbox")];
> +  is(deviceCbs.length, 2, "Both devices have a checkbox in modal");
> +  deviceCbs.forEach(cb => {

I haven't seen us use forEach in awhile. So, I would prefer to change this to use es6 for of loops.

::: devtools/client/responsive.html/test/browser/browser_device_custom_remove.js:100
(Diff revision 2)
> +  info("Ensure device 2 is no longer in device selector");
> +  deviceOption2 = [...deviceSelector.options].find(opt => opt.value == device2.name);
> +  ok(!deviceOption2, "Test device 2 option removed");
> +});
> +
> +addRDMTask(TEST_URL, function* ({ ui }) {

Add an info() to describe the new RDM Task.

::: devtools/client/responsive.html/test/browser/browser_device_custom_remove.js:120
(Diff revision 2)
> +  let deviceOption2 = [...deviceSelector.options].find(opt => opt.value == device2.name);
> +  ok(!deviceOption2, "Test device 2 option removed");
> +
> +  openDeviceModal(ui);
> +
> +  // yield new Promise(() => {})

Remove this.

::: devtools/client/responsive.html/test/browser/head.js:385
(Diff revision 2)
>      return content.navigator.userAgent;
>    });
>    is(ua, expected, `UA should be set to ${expected}`);
>  }
> +
> +function setDeviceAdder(ui, value) {

The function setDeviceAdder name isn't totally clear. So, I think we should either rename this to describe what the function is doing (something like "simulateAddingDeviceInDeviceAdder", "addDeviceInModal") or add a JSDoc to clarify the function.
Attachment #8923544 - Flags: review?(gl) → review+
Comment on attachment 8923544 [details]
Bug 1412359 - Filter to matching device on remove.

https://reviewboard.mozilla.org/r/194666/#review201262

> Add an info() to describe the new RDM Task.

Wasn't sure about the precise line that was best for this...  I placed it inside the task after the initial variables.

> Remove this.

Oops, thanks for catching it! :)

> The function setDeviceAdder name isn't totally clear. So, I think we should either rename this to describe what the function is doing (something like "simulateAddingDeviceInDeviceAdder", "addDeviceInModal") or add a JSDoc to clarify the function.

I noticed that all callers of `setDeviceAdder` also save the device and wait for it in the store, so I rolled that into the function.  That makes it a little easier to name as well, since it's more active than just setting some form fields.

I went with `addDeviceInModal` plus a comment.
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c24b248d0cef
Clarify test event as device-association-removed. r=gl
https://hg.mozilla.org/integration/autoland/rev/973e547ce20c
Filter to matching device on remove. r=gl
Florens, could you verify that the issues you saw when deleting devices in RDM are now fixed in the latest Nightly?

I'd like to be sure this fixes the issues you were facing.
Flags: needinfo?(florens)
It works correctly in my tests in Nighly (build ID 20171106100122).

Tried adding and deleting a bunch of device profiles and there doesn’t seem to be a mismatch between the elements clicked in the UI and the data after a refresh (or after going to a new tab).

Only issue mentioned previously that remains is that if you have RDM opened in two tabs, removing a device profile in one will not impact the device list in the other tab. (Might have the same issue for creation too.) But that would be a different issue, perhaps a known one, with a lower priority.
Flags: needinfo?(florens)
Verified fixed using the latest Nightly 58.0a1 (2017-11-06) on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12.
Status: RESOLVED → VERIFIED
status-firefox58: fixed → verified
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: