Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix various google photos issues #5275

Merged
merged 1 commit into from
Jun 27, 2024
Merged

fix various google photos issues #5275

merged 1 commit into from
Jun 27, 2024

Conversation

mifi
Copy link
Contributor

@mifi mifi commented Jun 25, 2024

#5061 (comment) also change the type of UppyFile.name, because it can be undefined

#5061 (comment)
also change the type of UppyFile.name, because it can be undefined
Copy link
Contributor

Diff output files
diff --git a/packages/@uppy/companion/lib/server/provider/google/googlephotos/index.js b/packages/@uppy/companion/lib/server/provider/google/googlephotos/index.js
index ce54a6a..46249cc 100644
--- a/packages/@uppy/companion/lib/server/provider/google/googlephotos/index.js
+++ b/packages/@uppy/companion/lib/server/provider/google/googlephotos/index.js
@@ -59,7 +59,7 @@ class GooglePhotos extends Provider {
         return paginate(
           (pageToken) =>
             client.get("albums", { searchParams: { pageToken, pageSize: 50 }, responseType: "json" }).json(),
-          (response) => response.albums,
+          (response) => response.albums ?? [],
         );
       }
       async function fetchSharedAlbums() {
@@ -82,7 +82,8 @@ class GooglePhotos extends Provider {
         }).json();
         return resp;
       }
-      const [sharedAlbums, albums, { mediaItems, nextPageToken }] = await Promise.all([
+      // mediaItems seems to be undefined if empty folder
+      const [sharedAlbums, albums, { mediaItems = [], nextPageToken }] = await Promise.all([
         fetchSharedAlbums(),
         fetchAlbums(),
         fetchMediaItems(),
diff --git a/packages/@uppy/core/lib/Restricter.js b/packages/@uppy/core/lib/Restricter.js
index 0dd13c1..b63360c 100644
--- a/packages/@uppy/core/lib/Restricter.js
+++ b/packages/@uppy/core/lib/Restricter.js
@@ -97,10 +97,11 @@ class Restricter {
       }
     }
     if (maxFileSize && file.size != null && file.size > maxFileSize) {
+      var _file$name;
       throw new RestrictionError(
         this.getI18n()("exceedsSize", {
           size: prettierBytes(maxFileSize),
-          file: file.name,
+          file: (_file$name = file.name) != null ? _file$name : this.getI18n()("unnamed"),
         }),
         {
           file,
@@ -137,9 +138,10 @@ class Restricter {
     }
   }
   getMissingRequiredMetaFields(file) {
+    var _file$name2;
     const error = new RestrictionError(
       this.getI18n()("missingRequiredMetaFieldOnFile", {
-        fileName: file.name,
+        fileName: (_file$name2 = file.name) != null ? _file$name2 : this.getI18n()("unnamed"),
       }),
     );
     const {
diff --git a/packages/@uppy/core/lib/Uppy.js b/packages/@uppy/core/lib/Uppy.js
index dd04229..0577f7e 100644
--- a/packages/@uppy/core/lib/Uppy.js
+++ b/packages/@uppy/core/lib/Uppy.js
@@ -1168,9 +1168,10 @@ function _checkAndUpdateFileState2(filesToAdd) {
       }
       const onBeforeFileAddedResult = this.opts.onBeforeFileAdded(newFile, nextFilesState);
       if (!onBeforeFileAddedResult && this.checkIfFileAlreadyExists(newFile.id)) {
+        var _newFile$name;
         throw new RestrictionError(
           this.i18n("noDuplicates", {
-            fileName: newFile.name,
+            fileName: (_newFile$name = newFile.name) != null ? _newFile$name : this.i18n("unnamed"),
           }),
           {
             file: fileToAdd,
diff --git a/packages/@uppy/image-editor/lib/ImageEditor.js b/packages/@uppy/image-editor/lib/ImageEditor.js
index b74b720..cc110db 100644
--- a/packages/@uppy/image-editor/lib/ImageEditor.js
+++ b/packages/@uppy/image-editor/lib/ImageEditor.js
@@ -47,11 +47,12 @@ export default class ImageEditor extends UIPlugin {
     });
     this.save = () => {
       const saveBlobCallback = blob => {
+        var _name;
         const {
           currentImage,
         } = this.getPluginState();
         this.uppy.setFileState(currentImage.id, {
-          data: new File([blob], currentImage.name, {
+          data: new File([blob], (_name = currentImage.name) != null ? _name : this.i18n("unnamed"), {
             type: blob.type,
           }),
           size: blob.size,
diff --git a/packages/@uppy/provider-views/lib/Breadcrumbs.js b/packages/@uppy/provider-views/lib/Breadcrumbs.js
index a911a5c..1b0f1e6 100644
--- a/packages/@uppy/provider-views/lib/Breadcrumbs.js
+++ b/packages/@uppy/provider-views/lib/Breadcrumbs.js
@@ -5,6 +5,7 @@ export default function Breadcrumbs(props) {
     title,
     breadcrumbsIcon,
     breadcrumbs,
+    i18n,
   } = props;
   return h(
     "div",
@@ -14,18 +15,27 @@ export default function Breadcrumbs(props) {
     h("div", {
       className: "uppy-Provider-breadcrumbsIcon",
     }, breadcrumbsIcon),
-    breadcrumbs.map((folder, index) =>
-      h(
+    breadcrumbs.map((folder, index) => {
+      var _folder$data$name;
+      return h(
         Fragment,
         null,
-        h("button", {
-          key: folder.id,
-          type: "button",
-          className: "uppy-u-reset uppy-c-btn",
-          onClick: () => openFolder(folder.id),
-        }, folder.type === "root" ? title : folder.data.name),
+        h(
+          "button",
+          {
+            key: folder.id,
+            type: "button",
+            className: "uppy-u-reset uppy-c-btn",
+            onClick: () => openFolder(folder.id),
+          },
+          folder.type === "root"
+            ? title
+            : (_folder$data$name = folder.data.name) != null
+            ? _folder$data$name
+            : i18n("unnamed"),
+        ),
         breadcrumbs.length === index + 1 ? "" : " / ",
-      )
-    ),
+      );
+    }),
   );
 }
diff --git a/packages/@uppy/provider-views/lib/Item/components/GridItem.js b/packages/@uppy/provider-views/lib/Item/components/GridItem.js
index 9b425f2..1393a50 100644
--- a/packages/@uppy/provider-views/lib/Item/components/GridItem.js
+++ b/packages/@uppy/provider-views/lib/Item/components/GridItem.js
@@ -1,6 +1,7 @@
 import { h } from "preact";
 import ItemIcon from "./ItemIcon.js";
 function GridItem(_ref) {
+  var _file$data$name, _file$data$name2;
   let {
     file,
     toggleCheckbox,
@@ -9,6 +10,7 @@ function GridItem(_ref) {
     restrictionError,
     showTitles,
     children = null,
+    i18n,
   } = _ref;
   return h(
     "li",
@@ -30,13 +32,13 @@ function GridItem(_ref) {
       "label",
       {
         htmlFor: file.id,
-        "aria-label": file.data.name,
+        "aria-label": (_file$data$name = file.data.name) != null ? _file$data$name : i18n("unnamed"),
         className: "uppy-u-reset uppy-ProviderBrowserItem-inner",
       },
       h(ItemIcon, {
         itemIconString: file.data.thumbnail || file.data.icon,
       }),
-      showTitles && file.data.name,
+      showTitles && ((_file$data$name2 = file.data.name) != null ? _file$data$name2 : i18n("unnamed")),
       children,
     ),
   );
diff --git a/packages/@uppy/provider-views/lib/Item/components/ListItem.js b/packages/@uppy/provider-views/lib/Item/components/ListItem.js
index 7417bac..143d98e 100644
--- a/packages/@uppy/provider-views/lib/Item/components/ListItem.js
+++ b/packages/@uppy/provider-views/lib/Item/components/ListItem.js
@@ -1,6 +1,7 @@
 import { h } from "preact";
 import ItemIcon from "./ItemIcon.js";
 export default function ListItem(_ref) {
+  var _file$data$name, _file$data$name2, _file$data$name3;
   let {
     file,
     openFolder,
@@ -26,7 +27,7 @@ export default function ListItem(_ref) {
       checked: file.status === "checked",
       "aria-label": file.data.isFolder
         ? i18n("allFilesFromFolderNamed", {
-          name: file.data.name,
+          name: (_file$data$name = file.data.name) != null ? _file$data$name : i18n("unnamed"),
         })
         : null,
       disabled: isDisabled,
@@ -40,7 +41,7 @@ export default function ListItem(_ref) {
           className: "uppy-u-reset uppy-c-btn uppy-ProviderBrowserItem-inner",
           onClick: () => openFolder(file.id),
           "aria-label": i18n("openFolderNamed", {
-            name: file.data.name,
+            name: (_file$data$name2 = file.data.name) != null ? _file$data$name2 : i18n("unnamed"),
           }),
         },
         h(
@@ -69,7 +70,7 @@ export default function ListItem(_ref) {
             itemIconString: file.data.icon,
           }),
         ),
-        showTitles && file.data.name,
+        showTitles && ((_file$data$name3 = file.data.name) != null ? _file$data$name3 : i18n("unnamed")),
       ),
   );
 }
diff --git a/packages/@uppy/provider-views/lib/ProviderView/Header.js b/packages/@uppy/provider-views/lib/ProviderView/Header.js
index aa73a46..c930a83 100644
--- a/packages/@uppy/provider-views/lib/ProviderView/Header.js
+++ b/packages/@uppy/provider-views/lib/ProviderView/Header.js
@@ -21,6 +21,7 @@ export default function Header(props) {
         breadcrumbs: props.breadcrumbs,
         breadcrumbsIcon: props.pluginIcon && props.pluginIcon(),
         title: props.title,
+        i18n: props.i18n,
       }),
       h(User, {
         logout: props.logout,
diff --git a/packages/@uppy/provider-views/lib/ProviderView/ProviderView.js b/packages/@uppy/provider-views/lib/ProviderView/ProviderView.js
index 66f9861..3e84e3e 100644
--- a/packages/@uppy/provider-views/lib/ProviderView/ProviderView.js
+++ b/packages/@uppy/provider-views/lib/ProviderView/ProviderView.js
@@ -78,9 +78,11 @@ export default class ProviderView {
         searchString,
       } = this.plugin.getPluginState();
       const inThisFolder = partialTree.filter(item => item.type !== "root" && item.parentId === currentFolderId);
-      const filtered = searchString === ""
-        ? inThisFolder
-        : inThisFolder.filter(item => item.data.name.toLowerCase().indexOf(searchString.toLowerCase()) !== -1);
+      const filtered = searchString === "" ? inThisFolder : inThisFolder.filter(item => {
+        var _item$data$name;
+        return ((_item$data$name = item.data.name) != null ? _item$data$name : this.plugin.uppy.i18n("unnamed"))
+          .toLowerCase().indexOf(searchString.toLowerCase()) !== -1;
+      });
       return filtered;
     };
     this.validateAggregateRestrictions = partialTree => {
diff --git a/packages/@uppy/transloadit/lib/Client.js b/packages/@uppy/transloadit/lib/Client.js
index 3c83a61..d1187c3 100644
--- a/packages/@uppy/transloadit/lib/Client.js
+++ b/packages/@uppy/transloadit/lib/Client.js
@@ -103,12 +103,13 @@ export default class Client {
     );
   }
   async addFile(assembly, file) {
+    var _file$name;
     if (!file.uploadURL) {
       return Promise.reject(new Error("File does not have an `uploadURL`."));
     }
     const size = encodeURIComponent(file.size);
     const uploadUrl = encodeURIComponent(file.uploadURL);
-    const filename = encodeURIComponent(file.name);
+    const filename = encodeURIComponent((_file$name = file.name) != null ? _file$name : "Unnamed");
     const fieldname = "file";
     const qs = `size=${size}&filename=${filename}&fieldname=${fieldname}&s3Url=${uploadUrl}`;
     const url = `${assembly.assembly_ssl_url}/add_file?${qs}`;

@mifi mifi changed the title fix various issues fix various google photos issues Jun 25, 2024
@aduh95 aduh95 self-requested a review June 25, 2024 14:53
@lakesare lakesare self-requested a review June 26, 2024 03:53
@@ -159,7 +159,7 @@ export default class Client<M extends Meta, B extends Body> {
}
const size = encodeURIComponent(file.size!)
const uploadUrl = encodeURIComponent(file.uploadURL)
const filename = encodeURIComponent(file.name)
const filename = encodeURIComponent(file.name ?? 'Unnamed')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm it looks like we have to repeat "Unnamed" in a lot of places.
And in general, we have to think about the possibility that .name can be absent.

Have you considered injecting "Unnamed" as a .name in GooglePhotos.tsx?

@lakesare
Copy link
Contributor

lakesare commented Jun 26, 2024

All the issue described here are fixed in this PR 👍

Not sure about making .name optional however, left a comment.

@lakesare
Copy link
Contributor

lakesare commented Jun 26, 2024

Some thoughts about the UI of GooglePhotos.

Here is what Uppy shows to me:

image
  • 5 normal albums - this is expected - I also have these 5 albums in GooglePhotos

    image
  • then goes "NDR discussions", this is apparently the album that was shared with me
    image

  • and then go 4 unnamed albums - I do not know where they come from.
    The photos are mine, but I do not see why one of the videos of the cow in a lake I took is in a separate album.
    3 of those albums are just empty.


Suggestions:

  • Can we remove the 4 unnamed albums? Are there any folder properties google api returns to us that could indicate these albums are unusual, or is their only unusual quality the fact that they are unnamed?
    Even if that's the case - I would go for removing "Unnamed" albums, I think this is almost certainly a GooglePhotos artifact. If someone created an album on purpose, they would sure name it. This won't prevent people from selecting a photo from that album, they can simply select the photo from the "Timeline" album.
  • Can we move the shared albums to the fake "Shared with me" folder, like we do in GoogleDrive?

So, taken together with my messages in Slack, I suggest the following interface:

  • remove "Unnamed" albums
  • add 3 fake folders: "Shared with me" (contains all photos/albums shared with me), "Albums" (contains a list of my albums), and "Timeline" (contains both albumless and within-album photos, exactly what we see in GooglePhotos interface).

@aduh95
Copy link
Contributor

aduh95 commented Jun 27, 2024

Taking @lakesare to a new issue as I think 3.x is also affected (and therefore it should be fixed in a separate PR).

@aduh95 aduh95 merged commit d536a35 into 4.x Jun 27, 2024
20 checks passed
@aduh95 aduh95 deleted the google-photos-issues branch June 27, 2024 08:31
@lakesare
Copy link
Contributor

lakesare commented Jun 27, 2024

@aduh95, I feel like the merge was a bit premature, we should have let Mikael respond to our questions & then merge it when some consensus is reached.
I understand this feels like an urgent fix for 3.x, but GooglePhotos is very clearly mvp functionality that only shows albums - it's not a big deal that some folder ids weren't working.
UPDATE: wait this is merged into 4.x, don't see a good reason to merge so soon then?

@mifi, one problem with letting .name sometimes be undefined is that in getCheckedFilesWithPaths.ts this gives us absDirPath and relDirPath that look like "/my.jpg", even though in reality it's "Unnamed/my.jpg".

@aduh95
Copy link
Contributor

aduh95 commented Jun 27, 2024

My understanding was this PR was bringing 4.x at the same feature level as main, and your suggestions were not related to 4.x but to the plugin in general.

@lakesare
Copy link
Contributor

lakesare commented Jun 27, 2024

You probably missed #5275 (comment) (name? change isn't in main yet).
The #5275 (comment) suggestion is here because it might remove the need for "Unnamed" folders altogether, removing the necessity of name?.

@aduh95
Copy link
Contributor

aduh95 commented Jun 27, 2024

@mifi can you please open a PR against main that implements that change, and address Evgenia's comment?

github-actions bot added a commit that referenced this pull request Jun 27, 2024
| Package                |       Version | Package                |       Version |
| ---------------------- | ------------- | ---------------------- | ------------- |
| @uppy/audio            |  2.0.0-beta.7 | @uppy/image-editor     |  3.0.0-beta.6 |
| @uppy/aws-s3           |  4.0.0-beta.8 | @uppy/instagram        |  4.0.0-beta.7 |
| @uppy/box              |  3.0.0-beta.8 | @uppy/onedrive         |  4.0.0-beta.8 |
| @uppy/companion        | 5.0.0-beta.11 | @uppy/provider-views   | 4.0.0-beta.10 |
| @uppy/companion-client |  4.0.0-beta.8 | @uppy/react            |  4.0.0-beta.8 |
| @uppy/core             | 4.0.0-beta.11 | @uppy/screen-capture   |  4.0.0-beta.6 |
| @uppy/dashboard        | 4.0.0-beta.11 | @uppy/transloadit      | 4.0.0-beta.10 |
| @uppy/drop-target      |  3.0.0-beta.6 | @uppy/unsplash         |  4.0.0-beta.8 |
| @uppy/dropbox          |  4.0.0-beta.9 | @uppy/url              |  4.0.0-beta.8 |
| @uppy/facebook         |  4.0.0-beta.7 | @uppy/utils            |  6.0.0-beta.9 |
| @uppy/file-input       |  4.0.0-beta.6 | @uppy/vue              |  2.0.0-beta.4 |
| @uppy/form             |  4.0.0-beta.5 | @uppy/webcam           |  4.0.0-beta.9 |
| @uppy/golden-retriever |  4.0.0-beta.6 | @uppy/xhr-upload       |  4.0.0-beta.7 |
| @uppy/google-drive     |  4.0.0-beta.1 | @uppy/zoom             |  3.0.0-beta.7 |
| @uppy/google-photos    |  0.2.0-beta.2 | uppy                   | 4.0.0-beta.13 |

- @uppy/companion: implement facebook app secret proof (Mikael Finstad / #5249)
- @uppy/provider-views: `Loader.tsx` - delete the file (Evgenia Karunus / #5284)
- @uppy/vue: fix passing of `props` (Antoine du Hamel / #5281)
- @uppy/google-photos: fix various issues (Mikael Finstad / #5275)
- @uppy/transloadit: fix strict type errors (Antoine du Hamel / #5271)
- @uppy/transloadit: simplify plugin to always run a single assembly (Merlijn Vos / #5158)
- meta: update Yarn version and npm deps (Antoine du Hamel / #5269)
- docs: prettier: 3.2.5 -> 3.3.2 (Antoine du Hamel / #5270)
- @uppy/provider-views: Provider views rewrite (.files, .folders => .partialTree) (Evgenia Karunus / #5050)
- @uppy/react: TS strict mode (Merlijn Vos / #5258)
- meta: simplify `build:ts` script (Antoine du Hamel / #5262)
@mifi
Copy link
Contributor Author

mifi commented Jul 1, 2024

@aduh95 but this PR was merged into 4.x, and if we decide on removing unnamed folders, I need to revert most of what's done in this PR (because name will never be undefined anymore), so I think it has to be done in 4.x

@mifi
Copy link
Contributor Author

mifi commented Jul 1, 2024

Btw should we i18n "Shared with me", "Albums" and "Timeline"? currently for Google Drive, the static string Shared with me is returned as name from Companion so it's always in english. Not sure the best way to support i18n, but maybe leave name empty and have a separate field named something like specialFolderType which can then be i18n in the Uppy frontend? (maybe Uppy can set name to this i18n value when it receives a folder with specialFolderType from companion)

@mifi mifi mentioned this pull request Jul 1, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants