-
Notifications
You must be signed in to change notification settings - Fork 9
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
feat: support total_count param for GET /submissions #1481
Conversation
Co-authored-by: Ole <ole.richter91@gmail.com> Co-authored-by: Daria Kozlova <Nargle30@gmail.com> Co-authored-by: Tam <kis.tamara91@gmail.com>
@@ -24,6 +24,10 @@ def update | |||
end | |||
|
|||
def index | |||
# Return x-total-count header if total_count param is present | |||
if params[:total_count].present? | |||
headers["X-Total-Count"] = Submission.count |
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.
suggestion: This would always return the total number of Submissions in the database. We should push this further down below any scopes/filters, if we need it. What UI would this support?
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 x-total-count header if total_count param is present. Keep this last | ||
if params[:total_count].present? | ||
headers["X-Total-Count"] = Submission.count |
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.
Should this return a total number of user's submissions? In this case use can do user.submissions.count
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.
Oh actually just submissions.count
, because there are other filters above and it is already filtered by user
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.
good catch - thanks!
e98510f
to
02822f1
Compare
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.
looks good, left 2 small comments
@@ -34,6 +34,12 @@ def index | |||
end | |||
|
|||
submissions = submissions.order(created_at: :desc).page(page).per(size) | |||
|
|||
# Return x-total-count header if total_count param is present. Keep this last |
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.
Keep this last
- was this sentence incomplete?
@@ -57,6 +57,18 @@ | |||
expect(body.first["id"]).to eq submission.id | |||
end | |||
|
|||
fit "returns total count when total_count param is present" do |
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.
don't forget to remove fit
!
Closing this for now - we can bring it again if we decide to support save and continue in the backend |
This PR adds support to a new param to
GET /submissions
endpoint in order to return the total count of submission.This might not be the best approach because we might need to support the same param in other endpoints, It would be interesting to investigate how to support that everywhere - but maybe that's not possible.
Why did we add this?
This was important in order to return the
totalCount
of submissions for a user in thesubmissionsConnection
interface.