Skip to content
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

Second branch to be reviewed before merge #2

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft

Conversation

Singhj2
Copy link
Owner

@Singhj2 Singhj2 commented Sep 7, 2023

Hugo gene symbol dropdown added returning top 50 options of the full hugo gene list utilizing gene api

@Singhj2 Singhj2 changed the title Second branch Second branch to be reviewed before merge Sep 7, 2023
.env.example Outdated Show resolved Hide resolved
apps/api/.env.example Outdated Show resolved Hide resolved
Comment on lines 16 to 17
const symbols = await this.geneService.fetchGeneSymbols();
return symbols;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const symbols = await this.geneService.fetchGeneSymbols();
return symbols;
return await this.geneService.fetchGeneSymbols();

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

made this change and tested that the api and dropdown both still work

@@ -13,7 +13,7 @@ async function bootstrap() {
const app = await NestFactory.create(AppModule);
const globalPrefix = 'api';
app.setGlobalPrefix(globalPrefix);
let origin = process.env.CTIMS_ENV === 'development' ? '*' : 'https://ctims.ca';
let origin = process.env.CTIMS_ENV === 'development' ? '*' : 'http://localhost:3000';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not modify.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I leave it in its original form, then the CTIMS home page does not load properly. I'll revert to original form before making the final commit

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CITMS_ENV should come from the .env file. First copy the .env.example file with the CITMS_ENV variable in it to same directory and name it .env. Next set the value to "development" then it should make origin "*" which should make everything work.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Ill do that now

useEffect(() => {
async function fetchTopSymbols() {
try {
const response = await axios.get('http://localhost:3333/api/genes')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could consider using useAxios hook under web/hooks

Copy link
Owner Author

@Singhj2 Singhj2 Sep 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would that look like this?

`axios.defaults.baseURL = 'http://localhost:3333/api';

const useAxios = () => {
const [hugoSymbols, setHugoSymbols] = useState([]);
const [error, setError] = useState('');

const fetchHugoSymbols = () => {
    axios
        .get('/genes')
        .then(function (response) {
        const symbols = response.data;
        setHugoSymbols(symbols);
        hugoSymbols.push(symbols);
      })
        .catch((err) => {
            setError(err);
        })
};

useEffect(() => {
    fetchHugoSymbols();
}, []);

return hugoSymbols;

};`

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Singhj2 this commit includes the changes I think we can use 3eb8781

Sorry I accidentally pushed the changes to your branch, but I reverted it. Therefore the two additional commits.

@@ -4,7 +4,7 @@ DATABASE_URL=mysql://ctims:ctims@localhost:3306/ctims
KEYCLOAK_URL=http://localhost:8080
KEYCLOAK_CLIENT_ID=ctims

# Keycloak client uuid. This is different from the id above.
# Keycloak client uuid. This is daifferent from the id above.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

Comment on lines -5 to -11
KEYCLOAK_URL=<KEYCLOAK_URL>
KEYCLOAK_CLIENT_ID=<client_id>
KEYCLOAK_CLIENT_UUID=<uuid>
KEYCLOAK_REALM=<realm>
KEYCLOAK_CLIENT_SECRET=<client_secret>
KEYCLOAK_ADMIN_CLIENT_ID=ctims-admin # a client with sevice account enabled
KEYCLOAK_ADMIN_CLIENT_SECRET=<secret>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should restore to what they have previously.

Comment on lines +15 to +19
try {
return await this.geneService.fetchGeneSymbols();
} catch (error) {
console.error('Error fetching data:', error);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you should catch the error but rather throw the exception so the frontend is aware.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants