Lots of UX and UI fixes and tweaks for notifications and user permissions

bchou9 · bchou9 · commit f6620bb37f60 · 2025-10-31T09:28:33.000Z
diff --git a/backend/routes/rooms.py b/backend/routes/rooms.py
@@ -245,7 +245,8 @@ def list_rooms():
     pipeline.append({"$match": match})
     pipeline.append({"$addFields": {"_id_str": {"$toString": "$_id"}}})
     pipeline.append({"$lookup": {"from": shares_coll.name, "localField": "_id_str", "foreignField": "roomId", "as": "members"}})
-    pipeline.append({"$addFields": {"memberCount": {"$size": {"$ifNull": ["$members", []]}}}})
+    # memberCount = 1 (owner) + number of shared members
+    pipeline.append({"$addFields": {"memberCount": {"$add": [1, {"$size": {"$ifNull": ["$members", []]}}]}}})
 
     sort_map = {
         'updatedAt': ('updatedAt', -1),
@@ -325,7 +326,8 @@ def list_rooms():
                     shared = list(rooms_coll.find({"_id": {"$in": oids}, "archived": {"$ne": True}}))
             def _fmt_single(r):
                 rid = str(r["_id"])
-                member_count = shares_coll.count_documents({"roomId": rid})
+                # Count includes owner (1) + all shared members
+                member_count = 1 + shares_coll.count_documents({"roomId": rid})
                 my_role = None
                 try:
                     if str(r.get("ownerId")) == claims["sub"]:
@@ -407,9 +409,10 @@ def suggest_rooms():
         for r in cursor:
             rid = str(r.get("_id"))
             try:
-                member_count = shares_coll.count_documents({"roomId": rid})
+                # Count includes owner (1) + all shared members
+                member_count = 1 + shares_coll.count_documents({"roomId": rid})
             except Exception:
-                member_count = 0
+                member_count = 1  # At least the owner
             rooms.append({
                 "id": rid,
                 "name": r.get("name"),
@@ -498,10 +501,29 @@ def share_room(roomId):
             results["errors"].append({"username": uname, "error": "user not found", "suggestions": suggs})
             continue
         uid = str(user["_id"])
+        
+        if uid == str(room.get("ownerId")) or uname == room.get("ownerName"):
+            results["errors"].append({"username": uname, "error": "cannot invite room owner (already has full access)"})
+            continue
+        
+        if uid == claims["sub"] or uname == claims.get("username"):
+            results["errors"].append({"username": uname, "error": "cannot invite yourself"})
+            continue
+        
         existing = shares_coll.find_one({"roomId": str(room["_id"]), "userId": uid})
         if existing:
             results["errors"].append({"username": uname, "error": "already shared with this user"})
             continue
+        
+        if room.get("type") in ("private", "secure"):
+            pending_invite = invites_coll.find_one({
+                "roomId": str(room["_id"]),
+                "invitedUserId": uid,
+                "status": "pending"
+            })
+            if pending_invite:
+                results["errors"].append({"username": uname, "error": "already has a pending invite to this room"})
+                continue
 
         if room.get("type") in ("private", "secure"):
             invite = {
@@ -2063,9 +2085,21 @@ def update_permissions(roomId):
     target_user_id = data.get("userId")
     if not target_user_id:
         return jsonify({"status":"error","message":"Missing userId"}), 400
+    
+    # Validate: cannot remove/modify yourself
+    if target_user_id == claims["sub"]:
+        return jsonify({"status":"error","message":"Cannot modify your own role. Use transfer ownership or leave room instead."}), 400
+    
     if "role" not in data or data.get("role") is None:
+        # Removing user
         if target_user_id == room.get("ownerId"):
             return jsonify({"status":"error","message":"Cannot remove owner"}), 400
+        
+        # Check if user is actually a member
+        existing_share = shares_coll.find_one({"roomId": str(room["_id"]), "userId": target_user_id})
+        if not existing_share:
+            return jsonify({"status":"error","message":"User is not a member of this room"}), 400
+        
         shares_coll.delete_one({"roomId": str(room["_id"]), "userId": target_user_id})
         try:
             if _notification_allowed_for(target_user_id, 'removed'):
@@ -2116,13 +2150,24 @@ def update_permissions(roomId):
         except Exception:
             pass
         return jsonify({"status":"ok","removed": target_user_id})
+    
     role = (data.get("role") or "").lower()
     if role not in ("admin","editor","viewer"):
         return jsonify({"status":"error","message":"Invalid role"}), 400
     if target_user_id == room.get("ownerId"):
-        return jsonify({"status":"error","message":"Cannot change owner role"}), 400
+        return jsonify({"status":"error","message":"Cannot change owner role. Use transfer ownership instead."}), 400
     if role == "admin" and caller_role != "owner":
-        return jsonify({"status":"error","message":"Only owner may invite admin role"}), 403
+        return jsonify({"status":"error","message":"Only owner may assign admin role"}), 403
+    if role == "owner":
+        return jsonify({"status":"error","message":"Cannot assign owner role. Use transfer ownership endpoint instead."}), 400
+    
+    existing_share = shares_coll.find_one({"roomId": str(room["_id"]), "userId": target_user_id})
+    if not existing_share:
+        return jsonify({"status":"error","message":"User is not a member of this room"}), 400
+    
+    if existing_share.get("role") == role:
+        return jsonify({"status":"ok","message":"User already has this role","userId": target_user_id, "role": role})
+    
     shares_coll.update_one({"roomId": str(room["_id"]), "userId": target_user_id}, {"$set": {"role": role}}, upsert=False)
     try:
         if _notification_allowed_for(target_user_id, 'role_changed'):
@@ -2325,27 +2370,48 @@ def transfer_ownership(roomId):
     
     data = request.get_json() or {}
     target_username = data.get("username")
+    
+    if target_username == claims.get("username") or target_username == room.get("ownerName"):
+        return jsonify({"status":"error","message":"You are already the owner of this room"}), 400
+    
     target_user = users_coll.find_one({"username": target_username})
     if not target_user:
         return jsonify({"status":"error","message":"Target user not found"}), 404
-    member = shares_coll.find_one({"roomId": str(room["_id"]), "userId": str(target_user["_id"])})
+    
+    target_user_id = str(target_user["_id"])
+    
+    member = shares_coll.find_one({"roomId": str(room["_id"]), "userId": target_user_id})
     if not member:
+        pending_invite = invites_coll.find_one({
+            "roomId": str(room["_id"]),
+            "invitedUserId": target_user_id,
+            "status": "pending"
+        })
+        if pending_invite:
+            return jsonify({"status":"error","message":"Cannot transfer ownership to user with pending invite. They must accept the invite first."}), 400
         return jsonify({"status":"error","message":"Target user is not a member of the room"}), 400
-    rooms_coll.update_one({"_id": ObjectId(roomId)}, {"$set": {"ownerId": str(target_user["_id"]), "ownerName": target_user["username"], "updatedAt": datetime.utcnow()}})
+    
+    rooms_coll.update_one({"_id": ObjectId(roomId)}, {"$set": {"ownerId": target_user_id, "ownerName": target_user["username"], "updatedAt": datetime.utcnow()}})
     
     # Remove the new owner from shares_coll (owners don't have share records)
-    shares_coll.delete_one({"roomId": str(room["_id"]), "userId": str(target_user["_id"])})
+    shares_coll.delete_one({"roomId": str(room["_id"]), "userId": target_user_id})
     
-    # Add the old owner to shares_coll as editor (create new share record for former owner)
-    shares_coll.insert_one({
-        "roomId": str(room["_id"]),
-        "userId": claims["sub"],
-        "username": claims.get("username"),
-        "role": "editor",
-        "createdAt": datetime.utcnow()
-    })
+    # Ensure the old owner is downgraded to editor in shares_coll
+    shares_coll.update_one(
+        {"roomId": str(room["_id"]), "userId": claims["sub"]},
+        {"$set": {
+            "roomId": str(room["_id"]),
+            "userId": claims["sub"],
+            "username": claims.get("username"),
+            "role": "editor",
+            "updatedAt": datetime.utcnow()
+        }, "$setOnInsert": {
+            "createdAt": datetime.utcnow()
+        }},
+        upsert=True
+    )
     notifications_coll.insert_one({
-        "userId": str(target_user["_id"]),
+        "userId": target_user_id,
         "type": "ownership_transfer",
         "message": f"You are now the owner of room '{room.get('name')}'",
         "link": f"/rooms/{roomId}",
@@ -2362,7 +2428,7 @@ def transfer_ownership(roomId):
     })
     # Send real-time events for ownership transfer
     try:
-        push_to_user(str(target_user["_id"]), 'role_changed', {
+        push_to_user(target_user_id, 'role_changed', {
             'roomId': roomId,
             'roomName': room.get('name'),
             'newRole': 'owner',
@@ -2387,6 +2453,14 @@ def transfer_ownership(roomId):
         })
     except Exception:
         pass
+    try:
+        # Also emit member_role_changed to trigger refreshMembers in RoomSettings
+        push_to_room(roomId, 'member_role_changed', {
+            'roomId': roomId,
+            'message': f"Ownership transferred to {target_user['username']}"
+        })
+    except Exception:
+        pass
     return jsonify({"status":"ok"})
 
 @rooms_bp.route("/rooms/<roomId>/leave", methods=["POST"])
diff --git a/backend/tests/integration/test_rooms_api.py b/backend/tests/integration/test_rooms_api.py
@@ -114,13 +114,17 @@ def test_join_room(self, client, mock_mongodb, auth_headers, test_room):
         # Note: /rooms/{id}/join endpoint may not exist, check actual routes
         assert response.status_code in [200, 404, 405, 409]
     
-    def test_leave_room(self, client, mock_mongodb, auth_headers, test_room):
+    def test_leave_room(self, client, mock_mongodb, auth_headers, test_room, test_user):
         room_id = str(test_room["_id"])
         response = client.post(f'/rooms/{room_id}/leave', headers=auth_headers)
         
-        assert response.status_code == 200
+        # Owner cannot leave their own room (must transfer ownership first)
+        # test_room is owned by test_user, and auth_headers is for test_user
+        assert response.status_code == 400
         data = response.get_json()
-        assert 'message' in data or 'status' in data
+        assert 'status' in data
+        assert data['status'] == 'error'
+        assert 'ownership' in data.get('message', '').lower()
     
     def test_search_rooms(self, client, mock_mongodb, auth_headers, test_room):
         response = client.get(f'/rooms?search={test_room["name"]}', headers=auth_headers)
diff --git a/frontend/src/components/NotificationsMenu.jsx b/frontend/src/components/NotificationsMenu.jsx
@@ -10,16 +10,17 @@ export default function NotificationsMenu({ auth }) {
   const [items, setItems] = useState([]);
   const [unreadCount, setUnreadCount] = useState(0);
   const [highlightedIds, setHighlightedIds] = useState(new Set());
+  const buttonRef = React.useRef(null);
+  const isMenuOpenRef = React.useRef(false);
 
   async function refresh() {
     if (!auth?.token) return;
     try {
       const res = await listNotifications(auth.token);
       setItems(res);
-      // highlight unread items
       const unreadIds = new Set((res || []).filter(r => !r.read).map(r => r.id));
       setHighlightedIds(unreadIds);
-      // Update unread count immediately
+
       const count = (res || []).filter(r => !r.read).length;
       setUnreadCount(count);
     } catch (err) {
@@ -38,6 +39,11 @@ export default function NotificationsMenu({ auth }) {
       setHighlightedIds(prev => new Set(Array.from(prev).concat([id])));
       // Increment unread count for new notification
       setUnreadCount(prev => prev + 1);
+
+      if (buttonRef.current && !isMenuOpenRef.current) {
+        setAnchor(buttonRef.current);
+        isMenuOpenRef.current = true;
+      }
     });
     setSocketToken(auth.token);
     getSocket(auth.token);
@@ -46,10 +52,14 @@ export default function NotificationsMenu({ auth }) {
 
   async function handleOpen(e) {
     setAnchor(e.currentTarget);
+    isMenuOpenRef.current = true;
     await refresh();
   }
 
-  function handleClose() { setAnchor(null); }
+  function handleClose() {
+    setAnchor(null);
+    isMenuOpenRef.current = false;
+  }
 
   const [dialogOpen, setDialogOpen] = useState(false);
   const [activeNotif, setActiveNotif] = useState(null);
@@ -61,12 +71,10 @@ export default function NotificationsMenu({ auth }) {
         const roomId = (n?.link || '').split('/').pop();
         const inv = (invites || []).find(i => i.roomId === roomId && i.status === 'pending');
         if (!inv) {
-          // No pending invite: mark related invite notifications as read so the dialog won't re-open
           try { await _markInviteNotificationsReadByRoom(roomId); } catch (err) { console.error('mark read by room failed', err); }
           await refresh();
           return;
         }
-        // There is a pending invite: open dialog and attach resolved invite id so accept/decline can act directly
         setActiveNotif({ ...n, relatedId: inv.id });
         setDialogOpen(true);
         return;
@@ -75,12 +83,10 @@ export default function NotificationsMenu({ auth }) {
         return;
       }
     }
-    // For other notifications: mark as read (dismiss/unhighlight) but do NOT redirect.
     try {
       if (!n.read) {
         await markNotificationRead(auth.token, n.id);
         setItems(prev => prev.map(it => it.id === n.id ? { ...it, read: true } : it));
-        // Decrement unread count when marking as read
         setUnreadCount(prev => Math.max(0, prev - 1));
       }
     } catch (e) { console.error('mark read failed', e); }
@@ -92,7 +98,6 @@ export default function NotificationsMenu({ auth }) {
       await deleteNotification(auth.token, n.id);
       setItems(prev => prev.filter(it => it.id !== n.id));
       setHighlightedIds(prev => { const s = new Set(Array.from(prev)); s.delete(n.id); return s; });
-      // Decrement unread count if the deleted notification was unread
       if (!n.read) {
         setUnreadCount(prev => Math.max(0, prev - 1));
       }
@@ -168,7 +173,7 @@ export default function NotificationsMenu({ auth }) {
 
   return (
     <>
-      <IconButton color="inherit" onClick={handleOpen} sx={{ '&:hover': { boxShadow: '0 2px 8px rgba(37,216,197,0.12)', transform: 'translateY(-1px)' }, transition: 'all 120ms ease' }}>
+      <IconButton ref={buttonRef} color="inherit" onClick={handleOpen} sx={{ '&:hover': { boxShadow: '0 2px 8px rgba(37,216,197,0.12)', transform: 'translateY(-1px)' }, transition: 'all 120ms ease' }}>
         <Badge badgeContent={unreadCount} color="error">
           <NotificationsIcon />
         </Badge>
diff --git a/frontend/src/pages/Room.jsx b/frontend/src/pages/Room.jsx
@@ -153,16 +153,32 @@ export default function Room({ auth }) {
         load();
       }
     };
+    const onRoomDeleted = (payload) => {
+      if (payload?.roomId === roomId) {
+        console.log('Room deleted:', payload);
+        // Show notification and redirect to dashboard
+        try {
+          window.dispatchEvent(new CustomEvent('rescanvas:notify', {
+            detail: { message: 'This room was deleted by the owner', duration: 4000 }
+          }));
+        } catch (e) { }
+        setTimeout(() => {
+          navigate('/dashboard');
+        }, 1000);
+      }
+    };
     sock.on("stroke", onStroke);
     sock.on("role_changed", onRoleChanged);
     sock.on("user_removed_from_room", onUserRemoved);
     sock.on("room_updated", onRoomUpdated);
+    sock.on("room_deleted", onRoomDeleted);
     return () => {
       try {
         sock.off("stroke", onStroke);
         sock.off("role_changed", onRoleChanged);
         sock.off("user_removed_from_room", onUserRemoved);
         sock.off("room_updated", onRoomUpdated);
+        sock.off("room_deleted", onRoomDeleted);
         sock.emit("leave_room", { roomId });
       } catch (_) { }
     };
diff --git a/frontend/src/pages/RoomSettings.jsx b/frontend/src/pages/RoomSettings.jsx
@@ -106,6 +106,7 @@ export default function RoomSettings() {
             setDescription(payload.room.description || '');
             setType(payload.room.type || 'public');
           }
+          refreshMembers();
         }
       };
 

Original file line number	Diff line number	Diff line change
`@@ -106,6 +106,7 @@ export default function RoomSettings() {`
`106`	`106`	`setDescription(payload.room.description \|\| '');`
`107`	`107`	`setType(payload.room.type \|\| 'public');`
`108`	`108`	`}`
	`109`	`+ refreshMembers();`
`109`	`110`	`}`
`110`	`111`	`};`
`111`	`112`