-
Notifications
You must be signed in to change notification settings - Fork 46
feat: add API endpoints for available resources #268
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
base: main
Are you sure you want to change the base?
Conversation
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.
JZK for your contribution!
Could you please look into best practices for writing tests and introduce them in this PR (or another).
cc @naveed-ahmad
Mushaf.approved | ||
end | ||
end | ||
end |
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.
Newline
I can look into it definitely for this PR iA. |
respond_to :json | ||
|
||
def index | ||
@resources = { |
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.
/resources
should render all approved resources, we don't need to group them.
JSON format should be like:
{
resources: [
{id: , name: , resource_type:, cardinality: description:},
...
]
}
resource_type
will be translation, tafisr, script etc. And cardinality_type
will be
1_word
, 1_verse
etc
JazakAllah Khair for the PR! Please take a look at this API-related PR: #196. insha`Allah, I plan to finalize and merge PR #196 in the next 2–3 days. It introduces the initial structure for the QUL API, including:
Some endpoints, especially for verses, will have a more complex JSON structure. Rendering JSON directly in controllers would make them bloated and harder to maintain or test. That's why this #196 introduces presenters to keep the controller clean and manageable. We can load simple resources in the presenter but for complex resources presenter will use finders to load data from the database. I know #196 is massive, i'll extract api and merge it to unblock you. But here are pointer for you to navigate api related stuff Slim controllers Chapter controller Presenter that's using finder to load data Chapter presenter Finder to load chapters and eager load related resources to avoid n+1 queries Chapter finder Views, simple and clean Chapter view |
Thank you very much. Since I'm new to Ruby on Rails, and I don't want to commit any broken or hacky code, it's taking me longer. I'll go through what you shared definitely, actually I had the versioning part in mind but forgot to mention in PR description. I also wrote controller tests and they were passing but then I had some issues in project setup, mostly with migrations, so I'm waiting to setup everything correctly, test again and then push. |
Please feel free to ping me on discord if you need any help or want to discuss something. |
Assalamualikum
Overview
This PR adds API endpoints for available resources as mentioned in issue #242.
Implementation Details
Recitation.approved
Mushaf.approved
*I was confused in figuring out a direct retrieval method for scripts as there are two seemingly related tables (
by_verse.rb
,by_word.rb
)For translations and tafsirs, I'm returning one sample per
resource_content_id
since I understand we want to list available resources, not individual translations of every verse. Please correct me if my understanding is wrong.Testing
I've manually tested all endpoints and they appear to be working, I didn't find any tests written previously so I haven't added tests for this PR - but I can add if required.
As someone new to Ruby on Rails, I did my best to follow good practices while creating these endpoints. I'm eager to learn and improve based on your feedback. Jazak Allah.