classtranscribe / FrontEnd

The React + Redux Frontend for ClassTranscribe
https://classtranscribe.illinois.edu
Other
25 stars 28 forks source link

Remove harded coded ct-dev #651

Closed angrave closed 7 months ago

angrave commented 1 year ago

The frontend can use different backend servers. Do not hardcode the backend server hostname

e.g. CTPopup.js

angrave commented 10 months ago

grep -r 'ncsa' src finds two examples where ct-dev is hard-coded!

./screens/Glossary/components/GlossaryBar/index.js: baseURL: 'https://ct-dev.ncsa.illinois.edu',

const apiInstance = axios.create({
    baseURL: 'https://ct-dev.ncsa.illinois.edu',
    timeout: 1000,
});

^ This code should be converted to use cthttp to make api calls.

./screens/Watch/Components/CTPopup/CTPopup.jsx: const hostName = "ct-dev.ncsa.illinois.edu"; // for local test

For the code below. Can we just use '/data/...' for the URLetc? i.e. drop the first bit, https://${hostName} ?

   const ret = await cthttp.get(`ASLVideo/GetASLVideosByTerm?term=${term.word}`);
    if (ret.status === 200) {
      // request success
      ret.data.forEach(element => {
        const URL = `https://${hostName}/data/aslvideos/aslcore/original/${element.uniqueASLIdentifier.replace(' ', '_')}.mp4`
        if (element.kind === 1) {
          setSignURL(URL);
        } else if (element.kind === 2) {
          setDefinitionURL(URL);
        } else if (element.kind === 3) {
          setExampleURL(URL);
        }
      })
angrave commented 8 months ago

Not sufficient. Why not use this axios object(or ifyou must copy the code).... that sets the base url correctly? https://github.com/classtranscribe/FrontEnd/blob/94ff55f8ad22b4f33ceaf556ec47c664e0f0786b/src/utils/cthttp/request.js#L20C1-L20C1

Your code-

HunterLikeFarmer commented 8 months ago

Hi professor Angrave,

I have been working on another issue for weeks and haven’t started on this issue yet. I will take care of it soon.

Thanks, Hunter

On Sun, Nov 12, 2023 at 10:15 AM Lawrence Angrave @.***> wrote:

Not sufficient. Why not use this axios object(or ifyou must copy the code).... that sets the base url correctly?

https://github.com/classtranscribe/FrontEnd/blob/94ff55f8ad22b4f33ceaf556ec47c664e0f0786b/src/utils/cthttp/request.js#L20C1-L20C1

Your code-

  • does not include port or http/https choice
  • does not work if the server is running elsewhere.

— Reply to this email directly, view it on GitHub https://github.com/classtranscribe/FrontEnd/issues/651#issuecomment-1807172105, or unsubscribe https://github.com/notifications/unsubscribe-auth/A2RYH3CU2PVEY3I4RAD5XXDYEDY2DAVCNFSM6AAAAAA2PBDNGGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBXGE3TEMJQGU . You are receiving this because you were assigned.Message ID: @.***>

angrave commented 8 months ago

Also getPath getBuffer maybe useful- https://github.com/classtranscribe/FrontEnd/blob/94ff55f8ad22b4f33ceaf556ec47c664e0f0786b/src/utils/cthttp/general-requests.js#L15 or cthttp.request(false) or the code from request.js ... env.baseURL || window.location.origin, if we don't need auth.

lijiaxi2018 commented 8 months ago

Hi Professor Angrave,

I have 3 questions or comments on this issue.

1) For the following part of code, are you suggesting that we should switch to using cthttp to make request?

const apiInstance = axios.create({
    baseURL: 'https://ct-dev.ncsa.illinois.edu',
    timeout: 1000,
});

2) For the following part of the code, I do not believe that we can drop the hostname part because the user is accessing the video content from their browser, which do not have direct access to the server file system.

const ret = await cthttp.get(`ASLVideo/GetASLVideosByTerm?term=${term.word}`);
if (ret.status === 200) {
  // request success
  ret.data.forEach(element => {
    const URL = `https://${hostName}/data/aslvideos/aslcore/original/${element.uniqueASLIdentifier.replace(' ', '_')}.mp4`
    if (element.kind === 1) {
      setSignURL(URL);
    } else if (element.kind === 2) {
      setDefinitionURL(URL);
    } else if (element.kind === 3) {
      setExampleURL(URL);
    }
  })

3) I would suggest that we first merge #716 to test if changing hostnames based on the server environment would fix the issue. Then we can work on to better improve our code.

Thanks, Jiaxi

angrave commented 7 months ago

Fixed