Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add admin manage article feature #382
base: dev
Are you sure you want to change the base?
Add admin manage article feature #382
Changes from all commits
1828149
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think the naming can be better. It's confusing to name like this since if you set
fetchFromDigitalLibrary
to true, it will first fetch from our DB and if not found, it will fetch from Digital Library. Therefore, I think namingfetchFromDigitalLibrary
is not accurate.Just provide one of my random idea, we can name it
useDigitalLibraryFallback
. @joannechen1223 any thought?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.
I prefer not to use this parameter and create another method instead, just like what you commented out in
getDbArticleByLink
as the logic of the digital library here is relatively complicated. Adding another API will be more straightforward if you are not going to reuse the digital library related logic.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.
Actually like what I commented out, at first I implemented a separated API to get article directly from the database, but our professor suggests me to combine the getDbArticleByLink with the original getArticleByLink because these two function are pretty similar and he doesn't want the API to explode at the beginning. Also make sense. So I changed the getArticleByLink function to the current version. May need to discuss.
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.
I think using separate method might be more scalable in this case given the original method was already really complicated. It also makes sense to try not to make too many APIs but I would prefer make functions easy to maintain than try to add complex logic in a function which is already heavy.
@joannechen1223 thoughts?