Teacher session #162

snowman562 · 2020-10-17T21:38:39Z

Please review and let me know

github-actions · 2020-10-17T21:43:18Z

Visit the preview URL for this PR (updated for commit e54e371):

https://joie-app--pr162-teacher-session-crl1q2ip.web.app

_{(expires Sat, 14 Nov 2020 10:28:47 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

yinonov

snowman562 · 2020-10-18T14:20:20Z

hook to the Kaltura final confirmation of success on the backend after its progress, so that we trigger the session document video reference property update on firebase.

upload component on session page should only be visible to an owner of a session

make sure upload is secure, so that only an owner can upload a video

garbage collect replaced videos on new upload (delete previous video from the Kaltura storage)

We update the session document video reference property when we get Kaltura final confirmation of success.
Upload component is only visible for an owner. And it's secure.
We are replacing the videos by entryId now, so we don't worry about garbage collect.

yinonov · 2020-10-18T15:52:47Z

hook to the Kaltura final confirmation of success on the backend after its progress, so that we trigger the session document video reference property update on firebase.

upload component on session page should only be visible to an owner of a session

make sure upload is secure, so that only an owner can upload a video

garbage collect replaced videos on new upload (delete previous video from the Kaltura storage)

We update the session document video reference property when we get Kaltura final confirmation of success.

Upload component is only visible for an owner. And it's secure.

We are replacing the videos by entryId now, so we don't worry about garbage collect.

still able to upload to a session I don't own

snowman562 · 2020-10-18T16:03:41Z

hook to the Kaltura final confirmation of success on the backend after its progress, so that we trigger the session document video reference property update on firebase.

upload component on session page should only be visible to an owner of a session

make sure upload is secure, so that only an owner can upload a video

garbage collect replaced videos on new upload (delete previous video from the Kaltura storage)

We update the session document video reference property when we get Kaltura final confirmation of success.

Upload component is only visible for an owner. And it's secure.

We are replacing the videos by entryId now, so we don't worry about garbage collect.

still able to upload to a session I don't own

Which session?

yinonov · 2020-10-18T20:33:23Z

@snowman562 4CJrFfb53KVLOuivduYV

snowman562 · 2020-10-19T06:17:26Z

add the ability to replace a current video by a new one
We already have this feature.

upload area should be wrapped by mat-card
It's done

as the Kaltura video processing take quite a while we may should a pending indicator to let users be aware
Added manual pending time while video processing on Kaltura.

once the video upload has finished I couldn't see the new video replacing the uploader. only when I refreshed the page. firestore should be easily reactive with data and we can hook to the document observable to reactively show the new video
It's weird. It works fine on my end. Please recheck on latest version.

trying to upload specific video of mine result in a blank rectangle. the video is a webm file
I uploaded my webm file successfully. Please recheck it.

And it's very strange why you can upload on the session that's not owned by you. It shouldn't be. Please recheck latest commits and show me full screenshot(with session id and login detail)

yinonov · 2020-10-19T11:12:48Z

now all sessions have the same video, which is odd

each session should have its own video
we should have the ability to replace the video with a fab button or something

LolaRev · 2020-10-20T15:35:50Z

Comments:

Hi, I have a couple of questions about it.
The figma file has 2 parts:
upload an on-demand video.
upload a thumbnail photo or an intro video.
The way it is created it looks like there is only one option available which is uploading the video.
My comments:
The video uploaded on the top part is the on-demand video and it should be as long as needed. The comment is not for this video. I would also like to add a progression bar and not a loader as it is a better indication for the user to know how long it will take.
The part at the bottom is missing completely. This is where they should be able to upload an image or an introduction video (up to a minute) or a photo.
The UI doesn't look as in the Figma and needs to be fixed.

Upload a video failed:

Once the video and the photo is uploaded the UI should no longer be there, only on editing mode.

snowman562 · 2020-10-20T19:08:54Z

Comments:

Hi, I have a couple of questions about it.
The figma file has 2 parts:
upload an on-demand video.
upload a thumbnail photo or an intro video.
The way it is created it looks like there is only one option available which is uploading the video.
My comments:
The video uploaded on the top part is the on-demand video and it should be as long as needed. The comment is not for this video. I would also like to add a progression bar and not a loader as it is a better indication for the user to know how long it will take.
The part at the bottom is missing completely. This is where they should be able to upload an image or an introduction video (up to a minute) or a photo.
The UI doesn't look as in the Figma and needs to be fixed.

Upload a video failed:

Once the video and the photo is uploaded the UI should no longer be there, only on editing mode.

UI was built based on your design, but it's updated after discussing with @yinonov . Please confirm it with him.
Regarding the progress bar, it's still pending task because Kaltura API doesn't give us the estimated uploading time.

yinonov · 2020-10-22T09:04:16Z

apps/joie/src/app/services/player.service.ts

+    uiconf_id: 46213871,
+    flashvars: {},
+    cache_st: 1602684332,
+    entry_id: '1_v3d6yfj9',


where is this value coming from?

@yinonov as I am seeing I guess the entry_id it should come from session object and the others from kalturaConfiguration

yinonov · 2020-10-22T09:05:53Z

apps/joie/src/app/sessions/components/video-upload/video-upload.component.ts

+                entryId: this.entryId,
+                entryLastUpdated: new Date().getTime(),
+              })
+              .subscribe(() => {


subscriptions must be unsubscribed at some point. add the untilDestroy decorator and pipe as we do in other subscriptions

@yinonov adjusted this, using the untilDestroy

yinonov · 2020-10-22T09:12:52Z

apps/joie/src/app/sessions/components/video-upload/video-upload.component.ts

+                entryLastUpdated: new Date().getTime(),
+              })
+              .subscribe(() => {
+                window.location.reload();


this can cause issues if user is in editing process or reading. can be really bad ui-wise.
I'd suggest the following practice:
entryLastUpdated should also be an input and a member of the app-video-player component. then we would be able to hook to SimpleChanges conditionally to observe entryLastUpdated and trigger the video embedding on ngOnChanges instead of ngOnInit in KalturaVodPlayerComponent

@yinonov this pice of code doesn't exists anymore

yinonov · 2020-10-22T09:18:41Z

apps/joie/src/app/sessions/components/video-upload/video-upload.component.ts

+              .subscribe(() => {
+                window.location.reload();
+              });
+          }, 60000);


is this the best we can do? no hook to actual completion indication by Kaltura's observable?

yinonov · 2020-10-22T10:57:18Z

Reply from Kaltura on issues we face -

1 – there isn’t a notification on progress, however you can register for an http notification when the media entry’s status changes and/or is ready. Please see the workflow detailed here for more info: https://developer.kaltura.com/workflows/Integration_Scheduling_and_Hooks/Backend_and_Email_Notifications

2 – Yes, please read about authentication and security via the Kaltura Session here: https://developer.kaltura.com/api-docs/VPaaS-API-Getting-Started/Kaltura_API_Authentication_and_Security.html
Also, if you provide more info on the workflow and scenario that you are concerned about, that will help

This was also asked in Yinon’s email:
We have cases where we replace videos and want the old videos to be garbage collected from storage to prevent bloat or unnecessary storing. Currently we are replacing the videos by entryId is that enough for videos to be collected (if they have no other reference)? or should we explicitly delete them?

In general, it’s not good practice to replace videos of the same entry. Why not delete the entry and create a new one?

points taken:

we can register for an http notification when the media entry’s status changes and/or is ready
we should dig on security docs to prevent random user with no access claims uploading to storage
Upon successful upload we should delete the previous entry explicitly and replace its id reference in our firebase session doc with the new id

yinonov · 2020-10-22T17:15:15Z

apps/joie/src/app/sessions/components/video-upload/video-upload.component.ts

+            this.sessionsFacade
+              .setSession(this.sessionId, {
+                entryId: this.entryId,
+                entryLastUpdated: new Date().getTime(),


firebase stores its own timestamp in documents when it's provided with a js Date value. so better to remove the getTime method

such practices should be done on cloud function event triggers. hook to session doc update and set the new Date()

what is the use of the entryLastUpdated anyway?

…subscribe

…her-session

…creation on server side

…her-session

ilirhushi · 2020-11-03T19:48:47Z

Hi @LolaRev,

I have this situation of a session which has video but alongside it is also visible the upload form as shown in the following image:

what do you suggest for this case in UI perspective?

Thanks.

…her-session

LolaRev · 2020-11-04T12:09:36Z

@ilirhushi @yinonov that was an issue that wasn't clear to me (I comment on it on the other task).

There are 2 components for uploading a video:

The teacher needs to upload the video (for on-demand) or create a link for it (for live streaming).
The teacher needs to upload a thumbnail photo/intro video (30 sec) to both types of video. These will appear on the card and on the session page on top, and will be viewed by every user.

The way it was built is that you have 2 different videos and it is not how it should be. (this is why you have another screen at the bottom).

Since we decided not to have the intro video to begin with and only a thumbnail photo, we need to have the option to add it while uploading the video. It should be a separate image for a thumbnail.

I hope it is clear now. Let me know if you have any questions.

…her-session

yinonov

Review still in progress
I'm aware of the fact most of the is inherited

yinonov · 2020-11-06T05:23:37Z

apps/joie/src/app/app.module.ts

@@ -51,6 +52,7 @@ import { NgxAuthFirebaseUIModule } from 'ngx-auth-firebaseui';
    //   provide: ORIGIN,
    //   useValue: environment.production ? undefined : 'http://localhost:5001',
    // },
+    KalturaClient


Is this required everywhere on the app?

yep the KalturaClient is used at KalturaApiHandShakeService

can we create a kaltura module of our own to be imported where needed? that way we could also remove the script fetch in

joie-web-app/apps/joie/src/index.html

Lines 30 to 33 in d842a6f

<script

src="https://cdnapisec.kaltura.com/p/2976751/sp/297675100/embedIframeJs/uiconf_id/46180971/partner_id/2976751"

defer

></script>

to that module (same as youtube component)

yinonov · 2020-11-06T05:31:11Z

apps/joie/src/app/sessions/components/video-intro/video-intro.component.html

Not sure we need this entryLastUpdated. Will break anything if we remove it from the app totally?

This whole component actually seems like just a proxy to the VOD player component. Consider removing it.

@yinonov If we remove it I guess it will not break anything, is just a timestamp on when the entry was updated on Kaltura side

as @pfuternik suggested, entry shouldn't be updated but deleted, and create a new entry. so this should probably be removed

yinonov · 2020-11-06T05:36:49Z

apps/joie/src/app/sessions/components/video-upload/video-upload.component.html

+        <mat-tab label="Image">
+            <h3 class="joie-list-name">Upload a thumbnail image</h3>
+            <mat-spinner style="margin: auto" *ngIf="uploading"></mat-spinner>
+            <ngx-file-drop dropZoneLabel="Drop files here" (onFileDrop)="dropped($event)" accept="image/*" *ngIf="!uploading">
+                <ng-template ngx-file-drop-content-tmp let-openFileSelector="openFileSelector">
+                    <div class="mt-3">Drop your file here or</div>
+                    <button mat-flat-button color="primary" (click)="uploadImgControl.click()" type="button"
+                        class="uploadBtn" [disabled]="uploading">
+                        <span *ngIf="!uploading">Browse</span>
+                    </button>
+                </ng-template>
+            </ngx-file-drop>
+            <h5 class="border-bottom">
+                Size: 1280x720 px. Formats: JPG, GIF, or PNG.
+            </h5>
+        </mat-tab>
+    </mat-tab-group>
+    <input hidden="true" type="file" accept="image/*" #uploadImgControl (change)="onClickFile($event)" />


The should be an own dedicated component for image handling. We also use image thumbnail picker in session form so we should unite.
The should be split to 3 parts

File picker & dropzone

Video upload handler (service?)

Image upload handler (service?)

@yinonov so basically I tried to refactor and have them in service: KalturaApiHandShakeService, having Video & Image upload handler in one service

u mean ur on that? working out?

yinonov · 2020-11-06T12:16:36Z

apps/joie/src/app/sessions/components/video-upload/video-upload.component.ts

+      );
+  }
+
+  fileReplace() {


@pfuternik suggested to delete an entry of video and replace by a new video and entry. This seems like a backend concern as when a new video ref is about to override a previous one, then we should delete that previous entry video before setting the new ref.

yinonov · 2020-11-06T12:17:59Z

apps/joie/src/app/sessions/components/video-upload/video-upload.component.ts

+    }
+  }
+
+  public dropped(files: NgxFileDropEntry[]) {


This should be extracted to a dedicated component

apps/joie/src/app/sessions/models/session-variants.ts

apps/joie/src/assets/icons/icons-set/border-or.svg

…her-session

snowman562 added 2 commits October 17, 2020 12:21

add upload video feature

8ed6b25

upload feature done

468006b

snowman562 requested a review from yinonov October 17, 2020 21:38

Merge branch 'live' into teacher-session

26ec1ac

snowman562 added 2 commits October 17, 2020 19:33

add onwership for upload

a0b43d5

update the format of changing

5018c05

yinonov requested changes Oct 18, 2020

View reviewed changes

add replacing feature for video upload

79af7d4

snowman562 closed this Oct 18, 2020

snowman562 reopened this Oct 18, 2020

snowman562 added 2 commits October 18, 2020 23:09

add pending time

5d0bbae

add more field to session object

bbde123

remove unnecessary var in html

4b569bd

snowman562 added 3 commits October 20, 2020 03:03

fix merge

92b80ee

fix merge conflicts

be9169e

fix merge conflicts

81dfa08

add delay for processing

fc479b3

yinonov reviewed Oct 22, 2020

View reviewed changes

ilirhushi added 13 commits October 29, 2020 15:56

[teacher-session] / :construction: using UntilDestroy decorator to un…

8dabe98

…subscribe

[teacher-session] / :construction: added some todos

365f67e

[teacher-session] / :construction: added some todos

af4db24

Merge branch 'live' of https://github.com/joie/joie-web-app into teac…

6b2f6c7

…her-session

Merge branch 'live' of https://github.com/joie/joie-web-app into teac…

b9115ba

…her-session

[teacher-session] / :construction: working on moving kaltura session …

84ac0b3

…creation on server side

[teacher-session] / :construction: working on moving kaltura session …

72aee2b

…creation on server side

[teacher-session] / :construction: working on moving kaltura session …

4d75138

…creation on server side

[teacher-session] / :construction: working on moving kaltura session …

12d2997

…creation on server side

[teacher-session] / :construction: working on moving kaltura session …

096a970

…creation on server side

[teacher-session] / :construction: working on moving kaltura session …

b3dad56

…creation on server side

[teacher-session] / :construction: working on moving kaltura session …

44e5985

…creation on server side

Merge branch 'live' of https://github.com/joie/joie-web-app into teac…

07094c5

…her-session

Merge branch 'live' of https://github.com/joie/joie-web-app into teac…

d636709

…her-session

ilirhushi added 8 commits November 4, 2020 14:08

[teacher-session] / moved onInit kaltura handshake

d3f9da0

[teacher-session] / refactoring video-upload.component

f6be20c

[teacher-session] / refactoring video-upload.component

0a48bdd

[teacher-session] / refactoring video-upload.component

d56171c

[teacher-session] / refactoring video-upload.component

e3bc6fa

Merge branch 'live' of https://github.com/joie/joie-web-app into teac…

0bccf93

…her-session

[teacher-session] / minor fix on KalturaEvent interface

63840d9

[teacher-session] / fixed some lint issues

772e8fc

ilirhushi requested a review from yinonov November 5, 2020 11:10

yinonov requested changes Nov 6, 2020

View reviewed changes

ilirhushi and others added 3 commits November 7, 2020 11:24

[teacher-session] removed uneccessary icon

e54e371

Merge branch 'live' of https://github.com/joie/joie-web-app into teac…

8eab51f

…her-session

Merge branch 'live' into teacher-session

c74970c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Teacher session #162

Teacher session #162

snowman562 commented Oct 17, 2020

github-actions bot commented Oct 17, 2020 •

edited

Loading

yinonov left a comment •

edited

Loading

snowman562 commented Oct 18, 2020 •

edited

Loading

yinonov commented Oct 18, 2020

snowman562 commented Oct 18, 2020

yinonov commented Oct 18, 2020

snowman562 commented Oct 19, 2020 •

edited

Loading

yinonov commented Oct 19, 2020

LolaRev commented Oct 20, 2020

snowman562 commented Oct 20, 2020

yinonov Oct 22, 2020

ilirhushi Oct 29, 2020 •

edited

Loading

yinonov Oct 22, 2020

ilirhushi Oct 29, 2020

yinonov Oct 22, 2020

ilirhushi Oct 29, 2020

yinonov Oct 22, 2020

yinonov commented Oct 22, 2020

yinonov Oct 22, 2020

ilirhushi commented Nov 3, 2020

LolaRev commented Nov 4, 2020

yinonov left a comment

yinonov Nov 6, 2020

ilirhushi Nov 7, 2020

yinonov Nov 7, 2020

yinonov Nov 6, 2020

ilirhushi Nov 7, 2020

yinonov Nov 7, 2020

yinonov Nov 6, 2020

ilirhushi Nov 7, 2020

yinonov Nov 7, 2020

yinonov Nov 6, 2020

yinonov Nov 6, 2020

	<script
	src="https://cdnapisec.kaltura.com/p/2976751/sp/297675100/embedIframeJs/uiconf_id/46180971/partner_id/2976751"
	defer
	></script>

Teacher session #162

Are you sure you want to change the base?

Teacher session #162

Conversation

snowman562 commented Oct 17, 2020

github-actions bot commented Oct 17, 2020 • edited Loading

yinonov left a comment • edited Loading

Choose a reason for hiding this comment

snowman562 commented Oct 18, 2020 • edited Loading

yinonov commented Oct 18, 2020

snowman562 commented Oct 18, 2020

yinonov commented Oct 18, 2020

snowman562 commented Oct 19, 2020 • edited Loading

yinonov commented Oct 19, 2020

LolaRev commented Oct 20, 2020

snowman562 commented Oct 20, 2020

Choose a reason for hiding this comment

ilirhushi Oct 29, 2020 • 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

yinonov commented Oct 22, 2020

points taken:

Choose a reason for hiding this comment

ilirhushi commented Nov 3, 2020

LolaRev commented Nov 4, 2020

yinonov 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 17, 2020 •

edited

Loading

yinonov left a comment •

edited

Loading

snowman562 commented Oct 18, 2020 •

edited

Loading

snowman562 commented Oct 19, 2020 •

edited

Loading

ilirhushi Oct 29, 2020 •

edited

Loading