Fall 2022 Work #53

Li-Haowei · 2022-12-01T23:21:31Z

Delete the file on file retrieval
Upload files function polished
Uploaded file structures changed
Frontend UI changed
minor changes

…to main

This is specific for testing GitHub actions to see tests

IanSaucy · 2022-12-05T18:33:02Z

Is this deployed somewhere so I can look at it? I know you have the hosting setup on our(spark's) GCP account. Is it deployed there?

…nking

IanSaucy

Overall looks good. Nice job working on both the frontend and backend. I know multiple people worked on this PR so make sure everyone individually responds my comments.

.github/workflows/firebase-hosting-merge.yml

IanSaucy · 2022-12-05T18:33:59Z

.github/workflows/add_collaborators.yml

@@ -1,51 +1,51 @@
-# Author: Rishab 


Please restore the original version of this file

I commented them out since the github action runs everytime we push and it slow down my testing on my unit test yml. I have reverted it back to older version

IanSaucy · 2022-12-05T18:35:36Z

README.md

+- Create different user types, with support for different user permissions
+  - Motivation: so patients and caretakers can both upload/manage files on behalf of the patient
+  - Suggested direction
+    - Currently, the ERD of the system looks as follows: [link](https://excalidraw.com/#json=-LCSG-ShDmak9AprUI9LT,zhR7TQiJovH9fbLHI2MJsA)
+    - A modification of the database and user flow to follow this general guideline would allow for these features to be built, as this would allow both patients and caretakers to upload/retrieve from the same document within the newly created collections: [link](https://excalidraw.com/#json=21EzZvSgTpRM558zRtxWx,e5qdQqTUEmp2myNCfwgo-g)
 - Develop additional audio processing features: 
  - Background noise reduction
  - Improve the clarity of slurred subject voice
- Other features: 
-  - Folder layout in processed files pages
-  - User is able to delete processed files
-  - User input in trimming/splitting process
-
+  - User input in trimming/splitting process


Please expand this section to be more verbose and better explain why the user features are needed etc. Talk about use cases etc

IanSaucy · 2022-12-05T18:39:37Z

backend/app.py

@@ -208,7 +206,78 @@ def retrieve_audio():
    print("Generated GET signed URL:")
    return url

+@app.route('/delete_unprocessed_audio', methods=['DELETE'])


Add documentation on the expected form of the HTTP request. Should provide an example HTTP request.

IanSaucy · 2022-12-05T18:41:59Z

backend/app.py

+            if cloud_storage_filename not in list(audio.values()):
+                new_audios.append(audio)


Why do you do this? It seems that the user could pass in arbitrary audio file names and they could get appended to the users list of audio. Which would not be valid since they would not actually exist.

It seems you should check if the passed audio is in the list, if it's not then that's an error. Since we must use the database as the single source of truth since it's the only thing that keeps data on what audio belongs to each person.

Audio is an example audio already existing in the user's document (see lines 229-233, these get the user doc, then the audios). So, this line is seeing if the unprocessed audio is part of this given unprocessed/processed audio combination. Then, if it is not it appends it to new_audios which is becomes the new list of audios in the firebase.

IanSaucy · 2022-12-05T18:43:19Z

backend/app.py


+@app.route('/delete_processed_audio', methods=['DELETE'])


This function does the same thing as the other one. Please find a way to not duplicate the code. Instead, maybe write an inner function that you pass a parameter of some sort to etc. If you're copying and pasting code you're doing something wrong (: (Normally but not always)

They have some slight differences. We have changed the database a little bit, and yes, there is more elegant way of combining them, but they are essentially deleting different things so we figured it is better having two of them separated to avoid future confusion

Please add a comment in the code on what the slight difference is! The next team will need to know.

Done! Left extensive comments.

IanSaucy · 2022-12-05T18:43:53Z

backend/audio.py

+    #prepPath = '/tmp/' + re.split('.wav|.WAV', (filePath.split('/tmp/')[1]))[0] + 'Edited.WAV' # the original way that append "Edited" each chunk
+    prepPath = '/tmp/' + re.split('.wav|.WAV', (filePath.split('/tmp/')[1]))[0] + '.WAV'


What happens if the file does not end in .wav?

It is still .wav, we just remove "Edited" since there are original clip and processed clip stored and displayed separately

My question was more, what happens if the user uploads something that is not a .wav file? Recall that any frontend verification can always be bypassed so the API should not trust any data it receives.

bump @Li-Haowei

IanSaucy · 2022-12-05T18:44:38Z

client/myModuleResolver.js

+// my-module-resolve.js
+module.exports = (request, options) => {
+    // Call the defaultResolver, so we leverage its cache, error handling, etc.
+    return options.defaultResolver(request, {
+      ...options,
+      // Use packageFilter to process parsed `package.json` before the resolution (see https://www.npmjs.com/package/resolve#resolveid-opts-cb)
+      packageFilter: pkg => {
+        if(pkg.name.startsWith('@firebase')) {
+          return {
+            ...pkg,
+            // Alter the value of `main` before resolving the package
+            main: pkg.esm5 || pkg.module,
+          };
+        }
+
+        return pkg;
+      },
+    });
+  };


Can you add a comment in this file on why this file exists

Yes, just added comments

IanSaucy · 2022-12-05T18:47:26Z

client/src/components/Retrieve/Retrieve.js

+    document.body.appendChild(style);
+    var frame = document.createElement("div");
+    frame.id = "divLoadingFrame";
+    frame.classList.add("loading-frame");
+    for (var i = 0; i < 10; i++) {
+      var track = document.createElement("div");
+      track.classList.add("loading-track");
+      var dot = document.createElement("div");
+      dot.classList.add("loading-dot");
+      track.style.transform = "rotate(" + String(i * 36) + "deg)";
+      track.appendChild(dot);
+      frame.appendChild(track);
+    }
+    document.body.appendChild(frame);
+    var wait = 0;
+    var dots = document.getElementsByClassName("loading-dot");
+    // There was a warning i is already defined, thus use j for index instead.
+    for (var j = 0; j < dots.length; j++) {
+      window.setTimeout(function(dot) {
+        dot.classList.add("loading-dot-animated");
+      }, wait, dots[j]);
+      wait += 150;
+    }
+  };


Can you explain why you're manually manipulating the DOM? It's generally never a good idea to manually touch the DOM when using react.

I didn't write this code but someone in my team. But I think it is for the loading animation.

I mean, I know what it's doing but I want to know why it was written. It is very very very bad form to manually manipulate the DOM in react.

@shaolin-x Can you please let me know why it's been coded this way? How are you sure it's safe?

Hi Ian, thanks for the notice. I actually used the existing code from Upload.js where it used this code to show a loading window, so I am not sure why it is originally written this way.

I have been working on using a different approach to show the loading window. Could you also give me some tips for an alternative approach?

You can use a conditional render based on a state variable(`useState) and render something that says loading or some spinner etc.

Loading windows fixed for both Retrieve.js and Upload.js!

…to main

toucham

I see that there are alot components that are commented out. Please try to clean it abit. If there are some you don't want to delete, maybe leave a comment to let the next team know what it's for.

backend/app.py

toucham · 2022-12-05T21:09:26Z

client/src/components/Dashboard/Dashboard.js

@@ -35,10 +61,16 @@ function Dashboard() {
    // eslint-disable-next-line react-hooks/exhaustive-deps


please do not disable eslint

The reason why it's giving you the following errors is because there are multiple dependencies used inside useEffect(), such as db, setName, history and console. Either decrease the dependencies in useEffect or declare the dependencies inside the array.

toucham · 2022-12-05T21:22:08Z

client/src/components/Dashboard/Dashboard.js

@@ -6,6 +6,32 @@ import { auth, db, logout } from "../firebase";
 import { Button } from "react-bootstrap";
 import "./Dashboard.css";

+import { Card, CardActionArea, CardContent, Grid, Typography, Paper, Container, Box } from "@mui/material"
+import { makeStyles } from "@mui/styles";


please note in your documentation that @mui/styles is a legacy library so that the next team will know.

@danieldelijani Thanks Daniel, appreciate your help!

toucham · 2022-12-05T21:27:30Z

client/src/components/Retrieve/Retrieve.js

+        console.log('Response from delete audio:');
+        console.log(response);


don't forget to delete console.log

IanSaucy · 2022-12-08T18:11:30Z

Bump, please respond to our comments team!

…nking

IanSaucy

a few more changes

IanSaucy · 2022-12-12T15:06:08Z

client/src/components/Retrieve/Retrieve.js

+    document.body.appendChild(style);
+    var frame = document.createElement("div");
+    frame.id = "divLoadingFrame";
+    frame.classList.add("loading-frame");
+    for (var i = 0; i < 10; i++) {
+      var track = document.createElement("div");
+      track.classList.add("loading-track");
+      var dot = document.createElement("div");
+      dot.classList.add("loading-dot");
+      track.style.transform = "rotate(" + String(i * 36) + "deg)";
+      track.appendChild(dot);
+      frame.appendChild(track);
+    }
+    document.body.appendChild(frame);
+    var wait = 0;
+    var dots = document.getElementsByClassName("loading-dot");
+    // There was a warning i is already defined, thus use j for index instead.
+    for (var j = 0; j < dots.length; j++) {
+      window.setTimeout(function(dot) {
+        dot.classList.add("loading-dot-animated");
+      }, wait, dots[j]);
+      wait += 150;
+    }
+  };


I mean, I know what it's doing but I want to know why it was written. It is very very very bad form to manually manipulate the DOM in react.

@shaolin-x Can you please let me know why it's been coded this way? How are you sure it's safe?

TerryL99 and others added 30 commits November 5, 2022 19:22

local env setup

18df8ef

edit app.py & firestore.rules

3a7b54c

Merge branch 'main' of github.com:Li-Haowei/se-bch-als-msg-banking in…

cc017f0

…to main

Merge branch 'main' of github.com:Li-Haowei/se-bch-als-msg-banking

91eedbd

test files

00e55f9

pause test yml on pushes

48d888e

pause push github actions

6ece7de

try jest

d8fe74b

change some ymls

f1308c8

push test

91d3231

push test

eb8003d

should be passing

9bcf4d5

Update push-test.yml

4b2f6bf

Update push-test.yml

b9564bf

Update push-test.yml

2dc1074

Update push-test.yml

071be96

Update package.json

93af240

This is specific for testing GitHub actions to see tests

Update push-test.yml

0c69486

Update push-test.yml

d6ea23c

Update push-test.yml

a85006a

Update push-test.yml

890dafb

Update push-test.yml

157cc9a

Update package.json

e4f880f

Update package.json

ea41a49

Update push-test.yml

ca212df

Update push-test.yml

43dac51

Update push-test.yml

1da632b

Update push-test.yml

884b5a1

Update push-test.yml

27162a5

Update push-test.yml

bac65bb

shaolin-x removed request for IanSaucy and toucham December 5, 2022 18:10

Update frontend ui for dashboard page

0ec42d4

shaolin-x added 2 commits December 5, 2022 13:36

fixed audio splicing

3e1bff3

Merge branch 'main' of https://github.com/Li-Haowei/se-bch-als-msg-ba…

e46ee09

…nking

IanSaucy suggested changes Dec 5, 2022

View reviewed changes

shaolin-x added 2 commits December 5, 2022 13:53

added mui/system package

0309546

made Navbar look nicer

7f13db3

IanSaucy changed the title ~~So far what we have done~~ Fall 2022 Work Dec 5, 2022

shaolin-x and others added 4 commits December 5, 2022 15:22

small fixes on frontend presentation

a01364a

Update ui for dashboard page

e2fd810

Merge branch 'main' of github.com:Li-Haowei/se-bch-als-msg-banking in…

cb64c9a

…to main

updated home

964f65d

toucham requested changes Dec 5, 2022

View reviewed changes

danieldelijani and others added 7 commits December 8, 2022 16:27

remove console.log from retrieve.js

5034f44

deprecated all otherlogin methods except google

275b396

Merge branch 'main' of https://github.com/Li-Haowei/se-bch-als-msg-ba…

13f5769

…nking

Update firebase-hosting-merge.yml

065210a

revert back to older version

4f3634c

Update myModuleResolver.js

d75698c

Update myModuleResolver.js

ca4d991

IanSaucy suggested changes Dec 12, 2022

View reviewed changes

danieldelijani and others added 6 commits December 12, 2022 14:18

made next steps in documentation more verbose

36ad9c6

Note that mui/styles is a legacy library

5b39088

Left comments on delete_unprocessed_audio

ba70210

add docs for delete un/processed audio

8326048

fix eslint issue

2d57af4

fix loading windows in Retrieve.js and Upload.js

cb3df0a

		if cloud_storage_filename not in list(audio.values()):
		new_audios.append(audio)

		#prepPath = '/tmp/' + re.split('.wav\|.WAV', (filePath.split('/tmp/')[1]))[0] + 'Edited.WAV' # the original way that append "Edited" each chunk
		prepPath = '/tmp/' + re.split('.wav\|.WAV', (filePath.split('/tmp/')[1]))[0] + '.WAV'

		@@ -35,10 +61,16 @@ function Dashboard() {
		// eslint-disable-next-line react-hooks/exhaustive-deps

		console.log('Response from delete audio:');
		console.log(response);


		@app.route('/delete_processed_audio', methods=['DELETE'])

Fall 2022 Work #53

Are you sure you want to change the base?

Fall 2022 Work #53

Conversation

Li-Haowei commented Dec 1, 2022

IanSaucy commented Dec 5, 2022

IanSaucy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieldelijani Dec 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Li-Haowei Dec 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toucham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IanSaucy commented Dec 8, 2022

IanSaucy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieldelijani Dec 12, 2022 •

edited

Loading

Li-Haowei Dec 12, 2022 •

edited

Loading