-
Notifications
You must be signed in to change notification settings - Fork 0
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
OV-377: Create preview page and flow #429
Conversation
@@ -0,0 +1,7 @@ | |||
|
|||
const createVideoUrl = (jwtToken: string): string => { | |||
const baseUrl = 'http://localhost:3000'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here how could I create the baseUrl dynamically for deployment?
public async createVideoIdToken(videoId: string): Promise<string> { | ||
const jwt = await new SignJWT({ videoId }) | ||
.setProtectedHeader({ alg: 'HS256' }) | ||
.sign(this.secretKey); | ||
return jwt.replaceAll('.', '~'); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets move it to video service
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use createToken method and set expiration time optional?
|
||
public async getVideoIdFromToken(token: string): Promise<string | null> { | ||
const payload = await this.verifyToken(token); | ||
return (payload?.['videoId'] as string) || null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return (payload?.['videoId'] as string) || null; | |
return (payload?.['videoId'] as string) ?? null; |
const Preview: React.FC<Properties> = ({ jwt }) => { | ||
const dispatch = useAppDispatch(); | ||
const [url, setUrl] = useState<string>(''); | ||
const [loading, setLoading] = useState<boolean>(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isLoading, setIsLoading
… and decoding tokens
@@ -0,0 +1,10 @@ | |||
const createVideoUrl = (jwtToken: string): string => { | |||
const baseUrl = | |||
import.meta.env['VITE_APP_NODE_ENV'] === 'production' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use our class on frontend to handle .env
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config.ENV.APP.ENVIRONMENT === AppEnvironment.PRODUCTION | ||
? 'http://bsa-2024-outreachvids-dev.eu-north-1.elasticbeanstalk.com' | ||
: 'http://localhost:3000'; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a new env variable and use it. What if the deployment URL changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done but could you test it if it was done correctly before merging? I have no way of testing in production enviroment and I dont want to break something a day before the demo. Thank you
const videoTokenHeader = headers['video_token']?.toString() ?? ''; | ||
|
||
return { | ||
status: HTTPCode.OK, | ||
payload: await this.publicVideoService.findUrlByToken( | ||
videoTokenHeader ?? '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need this nullish coalescing here? You already check it on line 32
this.throwVideoNotFoundError(); | ||
} | ||
|
||
const url = video.toObject().url; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const url = video.toObject().url; | |
const { url } = video.toObject(); |
@@ -109,6 +109,17 @@ class VideoController extends BaseController { | |||
}>, | |||
), | |||
}); | |||
this.addRoute({ | |||
path: `${VideosApiPath.ID}/share`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add this path to the enum
|
||
public async getVideoIdJWT(id: string): Promise<string> { | ||
const response = await this.load( | ||
this.getFullEndpoint(`${VideosApiPath.ROOT}${id}/share`, {}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
? 'http://bsa-2024-outreachvids-dev.eu-north-1.elasticbeanstalk.com' | ||
? config.ENV.DEPLOYMENT.URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to add something like PUBLIC_URL. And use it as it is without checks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im having problems with this one. I changed .env, EnvironmentSchema, and base-config.package adding the new ENV PUBLIC_URL variable but when I try to access it in createVideoUrl I keep getting undefined.
frontend/.env.example
Outdated
# | ||
# DEPLOYMENT | ||
# | ||
DEPLOYMENT_URL=http://bsa-2024-outreachvids-dev.eu-north-1.elasticbeanstalk.com |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# | |
# DEPLOYMENT | |
# | |
DEPLOYMENT_URL=http://bsa-2024-outreachvids-dev.eu-north-1.elasticbeanstalk.com | |
# | |
# DEPLOYMENT | |
# | |
PUBLIC_URL=http://localhost:3000 |
Summary:
This pull request implements the following features:
Preview Page Creation:
Button for Generating Preview Link:
JWT-based Preview URL Generation:
preview/:jwt
, which includes the JWT and the video ID.Preview URL Access and JWT Decoding:
New Public-Video Controller:
public-video
controller to handle requests related to video previews, including decoding JWTs and retrieving video information.OutreachVids.-.Google.Chrome.2024-09-25.09-10-12.mp4