Updated code to work with new DB rules, better tests (#214)

NikkelM · web-flow · commit 2bb803a9f04f · 2023-08-05T14:04:14.000+02:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,11 +1,16 @@
 # Changelog
 
-## v2.2.2
+## v2.2.3
 
 <!--Releasenotes start-->
+- Error messages now include the current channel ID to help with reporting an issue.
+- Some changes in how data that is sent to the database is handled, to prepare for a future security update.
+<!--Releasenotes end-->
+
+## v2.2.2
+
 - Fixed a bug where there would be no video IDs stored in the database, leading to an error.
 - Fixed the 'Changelog' button on the changelog page not doing anything.
-<!--Releasenotes end-->
 
 ## v2.2.1
 
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "random-youtube-video",
-  "version": "2.2.2",
+  "version": "2.2.3",
   "description": "Play a random video uploaded on the current YouTube channel.",
   "scripts": {
 		"dev": "concurrently \"npm run dev:chromium\" \"npm run dev:firefox\"",
diff --git a/src/background.js b/src/background.js
@@ -138,7 +138,7 @@ chrome.runtime.onMessage.addListener((request, sender, sendResponse) => {
 		// Updates (overwriting videos) a playlist in Firebase
 		case "overwritePlaylistInfoInDB":
 			updatePlaylistInfoInDB('uploadsPlaylists/' + request.data.key, request.data.val, true).then(sendResponse);
-			break;		
+			break;
 		// Before v1.0.0 the videos were stored in an array without upload times, so they need to all be refetched
 		case 'updateDBPlaylistToV1.0.0':
 			updateDBPlaylistToV1_0_0('uploadsPlaylists/' + request.data.key).then(sendResponse);
@@ -189,7 +189,14 @@ const app = initializeApp(firebaseConfig);
 const db = getDatabase(app);
 
 async function updatePlaylistInfoInDB(playlistId, playlistInfo, overwriteVideos) {
-	if (overwriteVideos) {
+	// Find out if the playlist already exists in the database
+	// We only need to send this request if we don't already have to overwrite the entry
+	let playlistExists = true;
+	if (!overwriteVideos) {
+		playlistExists = await readDataOnce(playlistId);
+	}
+
+	if (overwriteVideos || !playlistExists) {
 		console.log("Setting playlistInfo in the database...");
 		// Update the entire object. Due to the way Firebase works, this will overwrite the existing 'videos' object, as it is nested within the playlist
 		update(ref(db, playlistId), playlistInfo);
diff --git a/src/content.js b/src/content.js
@@ -120,9 +120,10 @@ function resetShuffleButtonText() {
 // ---------- Shuffle ----------
 // Called when the 'Shuffle' button is clicked
 async function shuffleVideos() {
+	let channelId;
 	try {
 		// Get the saved channelId from the button
-		const channelId = shuffleButton?.children[0]?.children[0]?.children[0]?.children?.namedItem('channelId')?.innerText;
+		channelId = shuffleButton?.children[0]?.children[0]?.children[0]?.children?.namedItem('channelId')?.innerText;
 
 		// If the channelId somehow wasn't saved, throw an error
 		if (!channelId) {
@@ -179,7 +180,7 @@ The page will reload and you can try again.`)
 		}
 
 		// Alert the user about the error
-		window.alert(`Random YouTube Video:\n\n${displayText}${error.message ? "\n" + error.message : ""}${error.reason ? "\n" + error.reason : ""}${error.solveHint ? "\n" + error.solveHint : ""}${error.showTrace !== false ? "\n\n" + error.stack : ""}`);
+		window.alert(`Random YouTube Video:\n\nChannel ${channelId}\n${displayText}${error.message ? "\n" + error.message : ""}${error.reason ? "\n" + error.reason : ""}${error.solveHint ? "\n" + error.solveHint : ""}${error.showTrace !== false ? "\n\n" + error.stack : ""}`);
 
 		// Immediately display the error
 		shuffleButtonTextElement.innerText = `\xa0${displayText}`;
diff --git a/src/shuffleVideo.js b/src/shuffleVideo.js
@@ -131,10 +131,10 @@ export async function chooseRandomVideo(channelId, firedFromPopup, progressTextE
 			} else {
 				// Otherwise, we want to only upload new videos. If there are no "newVideos", we upload all videos, as this is the first time we are uploading the playlist
 				console.log("Uploading new video IDs to the database...");
-				if(getLength(playlistInfo["newVideos"] ?? {}) > 0) {
+				if (getLength(playlistInfo["newVideos"] ?? {}) > 0) {
 					videosToDatabase = playlistInfo["newVideos"];
 				} else {
-					videosToDatabase = playlistInfo["videos"] ?? 0;
+					videosToDatabase = playlistInfo["videos"];
 				}
 			}
 
@@ -155,7 +155,7 @@ export async function chooseRandomVideo(channelId, firedFromPopup, progressTextE
 			// Remember the last time the playlist was accessed locally (==now)
 			"lastAccessedLocally": new Date().toISOString(),
 			"lastFetchedFromDB": playlistInfo["lastFetchedFromDB"] ?? new Date(0).toISOString(),
-			"lastVideoPublishedAt": playlistInfo["lastVideoPublishedAt"] ?? new Date(0).toISOString(),
+			"lastVideoPublishedAt": playlistInfo["lastVideoPublishedAt"] ?? new Date(0).toISOString().slice(0, 19) + 'Z',
 			"videos": playlistInfo["videos"] ?? {}
 		};
 
@@ -225,10 +225,24 @@ async function uploadPlaylistToDatabase(playlistInfo, videosToDatabase, uploadsP
 	// Only upload the wanted keys
 	const playlistInfoForDatabase = {
 		"lastUpdatedDBAt": playlistInfo["lastUpdatedDBAt"] ?? new Date().toISOString(),
-		"lastVideoPublishedAt": playlistInfo["lastVideoPublishedAt"] ?? new Date(0).toISOString(),
+		"lastVideoPublishedAt": playlistInfo["lastVideoPublishedAt"] ?? new Date(0).toISOString().slice(0, 19) + 'Z',
 		"videos": videosToDatabase
 	};
 
+	// Make sure the data is in the correct format
+	if (playlistInfoForDatabase["lastUpdatedDBAt"].length !== 24) {
+		alert(`Random YouTube Video:\nPlease send this information to the developer:\n\nlastUpdatedDBAt has the wrong format (got ${playlistInfoForDatabase["lastVideoPublishedAt"]}).\nChannelId: ${uploadsPlaylistId}.`);
+		return;
+	}
+	if (playlistInfoForDatabase["lastVideoPublishedAt"].length !== 20) {
+		alert(`Random YouTube Video:\nPlease send this information to the developer:\n\nlastVideoPublishedAt has the wrong format (got ${playlistInfoForDatabase["lastVideoPublishedAt"]}).\nChannelId: ${uploadsPlaylistId}.`);
+		return;
+	}
+	if (getLength(playlistInfoForDatabase["videos"]) < 1) {
+		alert(`Random YouTube Video:\nPlease send this information to the developer:\n\nNo videos object was found.\nChannelId: ${uploadsPlaylistId}.`);
+		return;
+	}
+
 	// Send the playlist info to the database
 	const msg = {
 		command: encounteredDeletedVideos ? 'overwritePlaylistInfoInDB' : 'updatePlaylistInfoInDB',
@@ -916,12 +930,14 @@ async function aprilFoolsJoke() {
 async function tryGetPlaylistFromLocalStorage(playlistId) {
 	return await chrome.storage.local.get([playlistId]).then(async (result) => {
 		if (result[playlistId]) {
+			/* c8 ignore start */
 			// To fix a bug introduced in v2.2.1
-			if(!result[playlistId]["videos"]) {
+			if (!result[playlistId]["videos"]) {
 				// Remove from localStorage
 				await chrome.storage.local.remove([playlistId]);
 				return {}
 			}
+			/* c8 ignore stop */
 			return result[playlistId];
 		}
 		return {};
diff --git a/static/manifest.json b/static/manifest.json
@@ -1,7 +1,7 @@
 {
 	"name": "Random YouTube Video",
 	"description": "Play a random video uploaded on the current YouTube channel.",
-	"version": "2.2.2",
+	"version": "2.2.3",
 	"manifest_version": 3,
 	"content_scripts": [
 		{
diff --git a/test/playlistPermutations.js b/test/playlistPermutations.js
@@ -272,19 +272,19 @@ const defaultDBDeletedVideos = {
 
 // The YT API returns a full-length timestamp
 const oneNewYTAPIVideo = {
-	"YT_V_000001": zeroDaysAgo
+	"YT_V_000001": zeroDaysAgo.slice(0, 19) + 'Z'
 };
 
 // Get over the 50 per page API limit, and get to more than one additional page for the inner while loop
 const multipleNewYTAPIVideos = {};
 for (let i = 1; i <= 70; i++) {
 	const key = `YT_V_${String(i).padStart(6, '0')}`;
-	multipleNewYTAPIVideos[key] = zeroDaysAgo;
+	multipleNewYTAPIVideos[key] = zeroDaysAgo.slice(0, 19) + 'Z';
 }
 // Add another 35 shorts, with _S_ in the key
 for (let i = 71; i <= 105; i++) {
 	const key = `YT_S_${String(i).padStart(6, '0')}`;
-	multipleNewYTAPIVideos[key] = zeroDaysAgo;
+	multipleNewYTAPIVideos[key] = zeroDaysAgo.slice(0, 19) + 'Z';
 }
 
 // ---------- Permutations for testing ----------
@@ -379,7 +379,7 @@ for (let i = 0; i < playlistModifiers[0].length; i++) {
 						}
 
 						// When was the last locally known video published
-						localLastVideoPublishedAt = threeDaysAgo;
+						localLastVideoPublishedAt = threeDaysAgo.slice(0, 19) + 'Z';
 
 						if (playlistModifiers[3][l] === "LocalPlaylistContainsDeletedVideos") {
 							localVideos = deepCopy(defaultLocalVideos);
@@ -401,7 +401,7 @@ for (let i = 0; i < playlistModifiers[0].length; i++) {
 						if (playlistModifiers[5][n] === "DBContainsVideosNotInLocalPlaylist") {
 							dbVideos = deepCopy({ ...defaultLocalVideos, ...defaultDBVideos });
 							dbDeletedVideos = null;
-							dbLastVideoPublishedAt = twoDaysAgo;
+							dbLastVideoPublishedAt = twoDaysAgo.slice(0, 19) + 'Z';
 						} else if (playlistModifiers[5][n] === "DBContainsNoVideosNotInLocalPlaylist") {
 							dbVideos = playlistModifiers[3][l] === "LocalPlaylistContainsOnlyShorts"
 								? deepCopy(defaultLocalShorts)
@@ -421,10 +421,10 @@ for (let i = 0; i < playlistModifiers[0].length; i++) {
 						if (playlistModifiers[1][j] !== "DBEntryIsUpToDate") {
 							if (playlistModifiers[4][m] === "OneNewVideoUploaded") {
 								newUploadedVideos = deepCopy(oneNewYTAPIVideo);
-								newLastVideoPublishedAt = zeroDaysAgo;
+								newLastVideoPublishedAt = zeroDaysAgo.slice(0, 19) + 'Z';
 							} else if (playlistModifiers[4][m] === "MultipleNewVideosUploaded") {
 								newUploadedVideos = deepCopy(multipleNewYTAPIVideos);
-								newLastVideoPublishedAt = zeroDaysAgo;
+								newLastVideoPublishedAt = zeroDaysAgo.slice(0, 19) + 'Z';
 							} else if (playlistModifiers[4][m] === "NoNewVideoUploaded") {
 								newUploadedVideos = null;
 								newLastVideoPublishedAt = dbLastVideoPublishedAt;
diff --git a/test/shuffleVideo.test.js b/test/shuffleVideo.test.js
@@ -42,6 +42,28 @@ function setUpMockResponses(mockResponses) {
 	});
 }
 
+// Checks that a set of messages contains the correct data format for the database
+function checkPlaylistsUploadedToDB(messages, input) {
+	messages.forEach((message) => {
+		const data = message[0].data;
+
+		expect(message.length).to.be(1);
+
+		expect(data.key).to.be(input.playlistId);
+		expect(Object.keys(data.val)).to.contain('lastUpdatedDBAt');
+		expect(data.val.lastUpdatedDBAt.length).to.be(24);
+		expect(Object.keys(data.val)).to.contain('lastVideoPublishedAt');
+		expect(data.val.lastVideoPublishedAt.length).to.be(20);
+		expect(Object.keys(data.val)).to.contain('videos');
+		expect(Object.keys(data.val.videos).length).to.be.greaterThan(0);
+		// Check the format of the videos
+		for (const [videoId, publishTime] of Object.entries(data.val.videos)) {
+			expect(videoId.length).to.be(11);
+			expect(publishTime.length).to.be(10);
+		}
+	});
+}
+
 describe('shuffleVideo', function () {
 
 	beforeEach(function () {
@@ -481,7 +503,8 @@ describe('shuffleVideo', function () {
 							if (videoId.startsWith('DEL_LOC')) {
 								delete allVideos[videoId];
 							} else {
-								allVideos[videoId] = publishTime;
+								// The YT API returns the publishTime in ISO 8601 format, so we need to convert it, as the localStorage and DB use a different format
+								allVideos[videoId] = new Date(publishTime).toISOString().slice(0, 19) + 'Z';
 							}
 						}
 						allVideos = Object.fromEntries(Object.entries(allVideos).sort((a, b) => b[1].localeCompare(a[1])));
@@ -856,7 +879,8 @@ describe('shuffleVideo', function () {
 							if (videoId.startsWith('DEL_LOC')) {
 								delete allVideos[videoId];
 							} else {
-								allVideos[videoId] = publishTime;
+								// The YT API returns the publishTime in ISO 8601 format, so we need to convert it, as the localStorage and DB use a different format
+								allVideos[videoId] = new Date(publishTime).toISOString().slice(0, 19) + 'Z';
 							}
 						}
 						allVideos = Object.fromEntries(Object.entries(allVideos).sort((a, b) => b[1].localeCompare(a[1])));
@@ -963,7 +987,8 @@ describe('shuffleVideo', function () {
 							it('should only interact with the database to remove deleted videos', async function () {
 								await chooseRandomVideo(input.channelId, false, domElement);
 
-								const commands = chrome.runtime.sendMessage.args.map(arg => arg[0].command);
+								const messages = chrome.runtime.sendMessage.args;
+								const commands = messages.map(arg => arg[0].command);
 
 								// callCount is 3 if we didn't choose a deleted video, 4 else
 								expect(chrome.runtime.sendMessage.callCount).to.be.within(3, 4);
@@ -974,6 +999,9 @@ describe('shuffleVideo', function () {
 
 								if (chrome.runtime.sendMessage.callCount === 4) {
 									expect(commands).to.contain('overwritePlaylistInfoInDB');
+									// One entry should contain a valid DB update
+									const overwriteMessages = messages.filter(arg => arg[0].command === 'overwritePlaylistInfoInDB');
+									checkPlaylistsUploadedToDB(overwriteMessages, input);
 								}
 
 							});
@@ -982,7 +1010,8 @@ describe('shuffleVideo', function () {
 								await chooseRandomVideo(input.channelId, false, domElement);
 								const playlistInfoAfter = await getKeyFromLocalStorage(input.playlistId);
 
-								const commands = chrome.runtime.sendMessage.args.map(arg => arg[0].command);
+								const messages = chrome.runtime.sendMessage.args;
+								const commands = messages.map(arg => arg[0].command);
 
 								if (needsYTAPIInteraction(input)) {
 									expect(chrome.runtime.sendMessage.callCount).to.be(6);
@@ -1008,14 +1037,21 @@ describe('shuffleVideo', function () {
 										break;
 									case 5:
 										expect(commands).to.contain('overwritePlaylistInfoInDB');
+										const overwriteMessages = messages.filter(arg => arg[0].command === 'overwritePlaylistInfoInDB');
+										checkPlaylistsUploadedToDB(overwriteMessages, input);
+
 										expect(numDeletedVideosBefore).to.be.greaterThan(numDeletedVideosAfter);
 										break;
 									case 6:
 										expect(commands).to.contain('getAPIKey');
 										if (numDeletedVideosBefore > numDeletedVideosAfter) {
 											expect(commands).to.contain('overwritePlaylistInfoInDB');
+											const overwriteMessages = messages.filter(arg => arg[0].command === 'overwritePlaylistInfoInDB');
+											checkPlaylistsUploadedToDB(overwriteMessages, input);
 										} else {
 											expect(commands).to.contain('updatePlaylistInfoInDB');
+											const updateMessages = messages.filter(arg => arg[0].command === 'updatePlaylistInfoInDB');
+											checkPlaylistsUploadedToDB(updateMessages, input);
 										}
 										break;
 									default:
@@ -1027,7 +1063,8 @@ describe('shuffleVideo', function () {
 								await chooseRandomVideo(input.channelId, false, domElement);
 								const playlistInfoAfter = await getKeyFromLocalStorage(input.playlistId);
 
-								const commands = chrome.runtime.sendMessage.args.map(arg => arg[0].command);
+								const messages = chrome.runtime.sendMessage.args;
+								const commands = messages.map(arg => arg[0].command);
 
 								expect(commands).to.contain('connectionTest');
 								expect(commands).to.contain('getPlaylistFromDB');
@@ -1041,6 +1078,8 @@ describe('shuffleVideo', function () {
 									expect(chrome.runtime.sendMessage.callCount).to.be(6);
 									expect(commands).to.contain('getAPIKey');
 									expect(commands).to.contain('updatePlaylistInfoInDB');
+									const updateMessages = messages.filter(arg => arg[0].command === 'updatePlaylistInfoInDB');
+									checkPlaylistsUploadedToDB(updateMessages, input);
 								} else {
 									expect(chrome.runtime.sendMessage.callCount).to.be(4);
 								}
@@ -1053,7 +1092,8 @@ describe('shuffleVideo', function () {
 							it('should only fetch data from the database', async function () {
 								await chooseRandomVideo(input.channelId, false, domElement);
 
-								const commands = chrome.runtime.sendMessage.args.map(arg => arg[0].command);
+								const messages = chrome.runtime.sendMessage.args;
+								const commands = messages.map(arg => arg[0].command);
 
 								// 4 because we only need to fetch from the DB
 								expect(chrome.runtime.sendMessage.callCount).to.be(4);
@@ -1068,7 +1108,8 @@ describe('shuffleVideo', function () {
 							it('should update the database', async function () {
 								await chooseRandomVideo(input.channelId, false, domElement);
 
-								const commands = chrome.runtime.sendMessage.args.map(arg => arg[0].command);
+								const messages = chrome.runtime.sendMessage.args;
+								const commands = messages.map(arg => arg[0].command);
 
 								// 6 because we need to fetch from the DB, fetch from the YT API, and update the DB
 								expect(chrome.runtime.sendMessage.callCount).to.be(6);
@@ -1079,6 +1120,8 @@ describe('shuffleVideo', function () {
 								expect(commands).to.contain('getCurrentTabId');
 								expect(commands).to.contain('getAPIKey');
 								expect(commands).to.contain('updatePlaylistInfoInDB');
+								const updateMessages = messages.filter(arg => arg[0].command === 'updatePlaylistInfoInDB');
+								checkPlaylistsUploadedToDB(updateMessages, input);
 							});
 						}
 					});

Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,6 @@`
`1`	`1`	`{`
`2`	`2`	`"name": "random-youtube-video",`
`3`		`- "version": "2.2.2",`
	`3`	`+ "version": "2.2.3",`
`4`	`4`	`"description": "Play a random video uploaded on the current YouTube channel.",`
`5`	`5`	`"scripts": {`
`6`	`6`	`"dev": "concurrently \"npm run dev:chromium\" \"npm run dev:firefox\"",`
Original file line number	Diff line number	Diff line change
`@@ -1,7 +1,7 @@`
`1`	`1`	`{`
`2`	`2`	`"name": "Random YouTube Video",`
`3`	`3`	`"description": "Play a random video uploaded on the current YouTube channel.",`
`4`		`- "version": "2.2.2",`
	`4`	`+ "version": "2.2.3",`
`5`	`5`	`"manifest_version": 3,`
`6`	`6`	`"content_scripts": [`
`7`	`7`	`{`