Remove MediaUploader #1122

orangejenny · 2024-09-10T12:41:03Z

Summary

Removes dependency on the MediaUploader component, which is based on YUI, which has been EOL for quite some time.

I'm just replacing this with a basic HTML file upload (hidden by a slightly more nicely styled button). Vellum doesn't use most of the more advanced MediaUploader functionality (multi-file uploads, sharing).

https://dimagi.atlassian.net/browse/SAAS-15969

Product Description

There are minor UI changes:

Removal of progress bar
Removal of within-popup "Cancel upload" button

Safety Assurance

Risk label is set correctly
The set of people pinged as reviewers is appropriate for the level of risk of the change
If QA is part of the safety story, the "Awaiting QA" label is used
I have confidence that this PR will not introduce a regression for the reasons below

Automated test coverage

There are tests for the code that updates filenames on upload.

QA Plan

https://dimagi.atlassian.net/browse/QA-7088

Safety story

Multimedia is historically finicky area (at least in HQ, I might be inappropriately slandering Vellum with that statement), but risk is limited to the widget.

Rollback instructions

This PR can be reverted after deploy with no further considerations

…ic file upload

This is hidden by default and toggled with the .hqm-share-media checkbox, which does not exist in vellum. See https://github.com/dimagi/MediaUploader/blob/1038b486ecfca4251bb5c85bc427a92df35cf8d7/hqmedia.upload_controller.js#L211-L214

This in a non-standard form of substitution, and it's standard (at least in HQ) for translations to contain a bit of HTML/underscore tags.

Intentionally sacrificing this functionality as more complex than its value justifies.

Since now there's both hqm-status and hqm-upload-status, there will likely be more naming updates.

This renames hqm-status to hqm-errors, becuase it's only used to display errors (in multi-file versions of the uploader, it would do more). It intentionally removes the "Cancel Upload" button shown when there are errors as a simplification. Closing and re-opening the model also allows the user to uplaod a new file.

With the progress bar gone, add this indictor that the server is working.

The only thing this does is call updateMediaPath, which is already called in the click handler for the upload confirmation button.

So far as I can tell, this isn't used. Also see #1004

Replicates https://github.com/dimagi/MediaUploader/blob/1038b486ecfca4251bb5c85bc427a92df35cf8d7/hqmedia.upload_controller.js#L275-L276

Now that we're no longer asynchronously loading file-uploader (hqmedia.upload_controller.js), this doesn't need to be deferred.

This is no longer needed now that the upload controls are simpler objects, rather than containing a YUI widget.

This is handled on the server side: https://github.com/dimagi/commcare-hq/blob/cd491e4844ee8b66188699e06d00d80b1ce835d0/corehq/apps/hqmedia/views.py#L645-L648

… opening a different modal of the same media type

There's only one modal per media type. The old code was adding a listener to each modal for every type of multimedia (normal, help, hint), all of which would fire whenever the modal was opened.

This doesn't appear to be used.

millerdev

❤️

millerdev · 2024-09-10T17:57:30Z

src/uploader.js

+
+            // Clear file input's value on click so that change event will fire on re-selecting the same file
+            $fileInput.on('click' , function() {
+                $(this).val('');


Is this destructive? Like if the user clicks the file input where a file had previously been selected, but then decides not to select a new file (maybe cancels out?), is the input now empty?

It is. I didn't think that was a problem, but after testing a bit, it turns out that behavior causes a js error, and I don't think it's doing anything useful. Removed in 49f1c6d

millerdev · 2024-09-10T18:04:00Z

package.json

    "main": "src/main.js",
    "scripts": {
-        "testserver": "`npm bin`/http-server -p ${VELLUM_PORT:-8088}",
+        "testserver": "node_modules/.bin/http-server -p ${VELLUM_PORT:-8088}",


I thought this was the standard way to get the "bin" path with npm. What version of npm are you running? I have 8.19.4 here (not sure how old that is), and npm bin works.

It's at least middle-aged. I'm on 10 and get an error:

[Vellum] (master) $ npm --version 10.8.1 [Vellum] (master) $ npm bin Unknown command: "bin" To see a list of supported npm commands, run: npm help

In places where commands need to be run that are not part of package.json, npm exec or npx seem to be the recommended replacement. However, in this context I'm not sure we need any of that. This works in my local environment

Suggested change

"testserver": "node_modules/.bin/http-server -p ${VELLUM_PORT:-8088}",

"testserver": "http-server -p ${VELLUM_PORT:-8088}",

Is it possible you installed http-server globally at some point? When I was setting up my Vellum environment last week (new laptop since the last time I worked on vellum), I couldn't run just http-server until I installed the package globally. I just deleted the node_modules directory and tried again to see if it was really necessary, and I think it is:

[Vellum] (jls/drop-MediaUploader) $ rm -rf node_modules [Vellum] (jls/drop-MediaUploader) $ ls node_modules/http-server ls: node_modules/http-server: No such file or directory [Vellum] (jls/drop-MediaUploader) $ yarn install --frozen-lockfile yarn install v1.22.21 (node:36307) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead. (Use `node --trace-deprecation ...` to show where the warning was created) [1/4] 🔍 Resolving packages... [2/4] 🚚 Fetching packages... [3/4] 🔗 Linking dependencies... [4/4] 🔨 Building fresh packages... warning Your current version of Yarn is out of date. The latest version is "1.22.22", while you're on "1.22.21". info To upgrade, run the following command: $ curl --compressed -o- -L https://yarnpkg.com/install.sh | bash ✨ Done in 39.73s. [Vellum] (jls/drop-MediaUploader) $ ls node_modules/http-server LICENSE README.md bin doc lib node_modules package.json [Vellum] (jls/drop-MediaUploader) $ http-server zsh: command not found: http-server [Vellum] (jls/drop-MediaUploader) $ npm install -g http-server added 46 packages in 2s 15 packages are looking for funding run `npm fund` for details [Vellum] (jls/drop-MediaUploader) $ http-server Starting up http-server, serving ./ http-server version: 14.1.1 ...

I forgot about your first sentence. npx exec http-server and npx http-server do both work for me locally even when the package is only installed locally, thank you for pointing those out.

I verified that http-server was not on my path, and npx does not seem to be necessary in package.json. Does that not work for you? My guess is that npx http-server is only necessary when running it directly on the command line, but it's not needed in the context of a command in package.json.

$ which http-server /usr/bin/which: no http-server in (...PATH...) $ http-server bash: http-server: command not found... $ grep testserver package.json "testserver": "http-server -p ${VELLUM_PORT:-8088}", $ npm run testserver > vellum@1.6.0 testserver > http-server -p ${VELLUM_PORT:-8088} Starting up http-server, serving ./ ... ^Chttp-server stopped.

src/uploader.js

millerdev · 2024-09-10T18:20:43Z

src/uploader.js

                uploadController.value.updateMediaPath();
+
+                var newExtension = '.' + file.name.split('.').pop().toLowerCase();
+                uploadController.value.uploadParams.path = uploadController.value.uploadParams.path.replace(/(\.[^/.]+)?$/, newExtension);


What happens here if the file does not have an extension? My quick test says it will add the full name as the extension:

>> newExtension = "." + "name".split('.').pop().toLowerCase() ".name" >> "/path/name".replace(/(\.[^/.]+)?$/, newExtension) "/path/name.name"

Is that expected?

It's weird, but I don't think it's harmful. Ultimately the server will reject the file for having an unrecognized extension, which I think is fine, we don't need to accept files without extensions.

millerdev · 2024-09-10T18:29:54Z

src/uploader.js

+            return $("#" + SLUG_TO_UPLOADER_SLUG[widget.form]);
+        };
+
+        widget.updateModalExistingFile = function (objectMap, isNew) {


It's not obvious to me what isNew means in this context. Seems like it causes an "upload completed" thing to become visible. Would it make sense to change the name to isCompleted?

sure - c423dfd

orangejenny · 2024-09-17T17:21:28Z

QA passed, merging.

orangejenny added 26 commits September 5, 2024 10:08

Fixed outdated HQ link

492557c

Removed direct call of HQMediaFileUploadController, replaced with bas…

ae6d468

…ic file upload

Simplified translation

3d3cedd

This in a non-standard form of substitution, and it's standard (at least in HQ) for translations to contain a bit of HTML/underscore tags.

Removed progress bar markup

0cb2f56

Intentionally sacrificing this functionality as more complex than its value justifies.

Removed bundle that has been commented out approximately forever

9fb8841

Removed references to MediaUploader and YUI

2f5c835

Replaced unknown npm bin command

d56fd34

Renames to get rid of "queue"

41f0edf

Since now there's both hqm-status and hqm-upload-status, there will likely be more naming updates.

Restored button instead of file input

e48c68f

Added spinner

182e261

With the progress bar gone, add this indictor that the server is working.

Removed old startUpload

642f2df

The only thing this does is call updateMediaPath, which is already called in the click handler for the upload confirmation button.

Restored allowClose behavior

004d097

Restored UI for current media in modal

21df873

Removed sessionid

023e7a7

So far as I can tell, this isn't used. Also see #1004

Restored logic to update file extension

52e91aa

Replicates https://github.com/dimagi/MediaUploader/blob/1038b486ecfca4251bb5c85bc427a92df35cf8d7/hqmedia.upload_controller.js#L275-L276

Removed logic to defer init

8d91694

Now that we're no longer asynchronously loading file-uploader (hqmedia.upload_controller.js), this doesn't need to be deferred.

Removed uplaodController attributes that are no longer used

ef11965

Simplified usage of uploadController

f355aaf

Removed destroy method

9a70d0f

This is no longer needed now that the upload controls are simpler objects, rather than containing a YUI widget.

Renamed uploadControls to match uploadController

b0cf613

DRY up existing file UI code

06e218c

Simplified SUPPORTED_EXTENSIONS

bd7b4f9

This is handled on the server side: https://github.com/dimagi/commcare-hq/blob/cd491e4844ee8b66188699e06d00d80b1ce835d0/corehq/apps/hqmedia/views.py#L645-L648

Fixed display of message with filename and size not disappearing when…

0a476e7

… opening a different modal of the same media type

Moved code for displaying existing file in modal

f62c0d6

There's only one modal per media type. The old code was adding a listener to each modal for every type of multimedia (normal, help, hint), all of which would fire whenever the modal was opened.

orangejenny added awaiting QA product/all-users-all-environments labels Sep 10, 2024

orangejenny requested a review from millerdev September 10, 2024 12:41

orangejenny mentioned this pull request Sep 10, 2024

Stop passing sessionid cookie in to media uploader #1004

Open

5 tasks

Removed modal unique id

8a66380

This doesn't appear to be used.

millerdev reviewed Sep 10, 2024

View reviewed changes

orangejenny added 4 commits September 10, 2024 15:55

Renamed var

c423dfd

Replaced node_modules path with npm exec

ed0035d

Removed click handler that clears file value

49f1c6d

Replaced npm exec with npx

606332f

millerdev approved these changes Sep 11, 2024

View reviewed changes

Tried removing npx from testserver command

c02c9ab

millerdev approved these changes Sep 16, 2024

View reviewed changes

orangejenny added passed QA and removed awaiting QA labels Sep 17, 2024

orangejenny merged commit 81a02d4 into master Sep 17, 2024

orangejenny deleted the jls/drop-MediaUploader branch September 17, 2024 17:21

orangejenny mentioned this pull request Sep 17, 2024

vellum:81a02d4222d41dad1b32ca2230c40e366f7c0a37 dimagi/commcare-hq#35136

Merged

orangejenny mentioned this pull request Oct 17, 2024

Remove MediaUploader and YUI from HQ dimagi/commcare-hq#35230

Merged

3 tasks

	"testserver": "node_modules/.bin/http-server -p ${VELLUM_PORT:-8088}",
	"testserver": "http-server -p ${VELLUM_PORT:-8088}",

Remove MediaUploader #1122

Remove MediaUploader #1122

Uh oh!

Conversation

orangejenny commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Product Description

Safety Assurance

Automated test coverage

QA Plan

Safety story

Rollback instructions

Uh oh!

millerdev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orangejenny commented Sep 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

orangejenny commented Sep 10, 2024 •

edited

Loading