Page MenuHomePhabricator

Fix editing previous Conpherence room bug

Authored by chad on Sep 12 2016, 2:55 AM.



Fixes T8972. This is the easiest fix I could find, I'm not great a JavaScript and don't feel like learning on Conpherence, but buggypoo is gone here if I get the variable from a more reliable place. Also removing the 'ajax' portion of saving. It doesn't work cleanly (updating the title doesn't update the page until you refresh), and just redirecting back to the page seems cleaner.

Test Plan

Make 2 rooms, Room A, Room B. Click on A, then B in sidebar. Click to edit room, see info come up for B (previously A). Save new title, see page reload and new title in room

Diff Detail

rP Phabricator
conph-edit-but (branched from master)
Lint Passed
Tests Passed
Build Status
Buildable 13689
Build 17658: Run Core Tests
Build 17657: arc lint + arc unit

Event Timeline

chad retitled this revision from to Fix editing previous Conpherence room bug.
chad updated this object.
chad edited the test plan for this revision. (Show Details)
chad added a reviewer: epriestley.
chad edited the test plan for this revision. (Show Details)
  • remove unneed force-ajax

I think this is likely correct/reasonable, but I need to look at this code for a bit to convince myself of that.

chad edited edge metadata.
  • use correct variable

I did some more debugging then and into this variable. Basically this is one of the race conditions seems expected, which this variable was created for. It just wasn't used in this case.

if we delete 100% of this code does that also work

Yeah, there is a lot of JS here that seems unnecessary just to rebuild the UI when user clicks on new thread. Which seems excessive given we don't have a heavy UI / stack.

But may be expected if each piece needs to be updatable in real time like if we have per user status.

Let me spend like 10 minutes reading through this, but I think I did that before and wasn't able to come up with a nice easy fix like this. You may reasonably have just done a better job of it than I did, but there might be some more weird races or something here, too.

If we conclude this stuff needs to be burned to the ground, would you want to change the design of the right sidebar? Seems weird now that you click "Participants" to access "Edit", for example...

Also maybe trying to unify the durable column controls with the main controls so they look/act more similar? Not sure if that makes sense. There might be mocks of this stuff that I'm just forgetting about, too.

I console.log'd the crap out of it.

I've thought about moving edit and settings somewhere else, don't have a solid plan right now but could by weeks end.

epriestley edited edge metadata.

This sort of works, but it's a really hacky fix with at least one problem.

As far as I can tell, the root issue is that every time we load a thread we create a new dropdown menu. When you click "Participants", you're actually getting a "random" dropdown menu (where "random" isn't really random) for some thread, not necessarily the menu for the visible thread. When the "random" menu is the menu for a different thread, it edits that other thread instead of the visible one.

This changes "fixes" that by making every thread's menu item link to the right edit endpoint for the current visible thread. So that works, but you're still opening the wrong menu and clicking the wrong item, just being taken to the right place. This creates at least one bug:

  • Create threads A and B.
  • As a user who can edit A and not B:
    • Load thread A.
    • Switch to B.
    • Use the dropdown menu.
  • Expected behavior:
    • "Edit Room" is disabled because you can't edit B.
  • Actual behavior:
    • "Edit Room" is enabled because you can edit A, and the actual item you are seeing is "Edit Room A", just linked to the URI for "Edit Room B".

Actually I don't even know if this is a bug since I think we get the state of this item wrong anyway (it never shows disabled?). And I still get lost about five times every time I try to figure something out here -- we started building Conpherence as a very different sort of application than we ended up with and the code hasn't caught up yet:

Screen Shot 2016-09-12 at 10.13.54 AM.png (989×1 px, 173 KB)

Also I managed to invite a room to another room, which I found very amusing (display glitch only):

Screen Shot 2016-09-12 at 10.12.34 AM.png (179×273 px, 14 KB)

This "edit a disabled room" issue is a million times less bad than the current "edit the wrong room" issue, so I think this fix is fine, but I think the real solution here is to burn it to the ground and rewrite the code to better align with how Conpherence actually ended up working.

This revision is now accepted and ready to land.Sep 12 2016, 5:21 PM

If you have design ideas for that sidebar I can find a day or so to put into fixing this stuff -- I don't think it's really all that bad, there's just a ton of widget code which doesn't make sense anymore, and a ton of code trying to really AJAX everything perfectly. We'd be in better shape with less AJAX and things that actually work properly, and then we can make the loading fancier later as we build out the durable sidebar, private messaging, online status, search, etc.

I want to probably put a "Manage Room" page somewhere. That roughly means:

  • Moving search / new room into the left column.
  • Putting "Manage Room" "Search Room" "View Members" into the crumbs area
  • Probably adding a topic subheader to Conpherence
  • Making the Crumbs bar a real room header again.
  • "Widgets" can essentially go away and just be members / status.

I think I can do all that myself then you can just delete all the JS you don't want anymore.