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

Can't submit timesheet entries. #36

Closed
Tracked by #32
BeckyPollard opened this issue Sep 30, 2022 · 3 comments · Fixed by #40
Closed
Tracked by #32

Can't submit timesheet entries. #36

BeckyPollard opened this issue Sep 30, 2022 · 3 comments · Fixed by #40
Assignees
Labels
bug Something isn't working
Milestone

Comments

@BeckyPollard
Copy link
Contributor

BeckyPollard commented Sep 30, 2022

Task Summary

Need to be able to submit/save timesheet entries to the API, either automatically or by hitting the SUBMIT button on the timesheet row.

Screen Shot 2022-09-30 at 3 25 06 PM

Context

After some testing we found that the endpoint is making a POST request on /person/1/sheet/2022/42
Contextually, this doesn't make sense bc person/1/entry would be ideal next to this submit.

After discussing with @evert and the gang, we came to the conclusion it would be best for
the endpoint: /person/1/sheet/{year}/{week}
to dynamically accept either a single day's entry or the entire weeks' entry.

@BeckyPollard BeckyPollard added this to the MVP 0.5 milestone Sep 30, 2022
@BeckyPollard BeckyPollard transferred this issue from badgateway/tt-app Oct 17, 2022
@syedfkabir syedfkabir self-assigned this Oct 18, 2022
@syedfkabir syedfkabir added the bug Something isn't working label Oct 18, 2022
@evert
Copy link
Collaborator

evert commented Oct 19, 2022

For context the frontend used /person/1/entry for getting the list of entries and submitting new entries. I added a new API route for reading per-week entries (/person/1/sheet/{year}/{week}) but this broke submissions because there was no POST request on that endpoint.

2 options for fixing this:

  1. Accept POST requests on /person/1/sheet/{year}/{week}
  2. Change the frontend to POST to /person/1/entry

I think both solutions are good, as long as in the end the backend supports only 1 way to do this, not both.

@syedfkabir
Copy link
Contributor

as long as in the end the backend supports only 1 way to do this, not both.

After some thought, I think the best approach is to change the frontend to POST to /person/1/entry
I understand how using /person/1/sheet/{year}/{week} can be done, but my argument would be
we shouldn't overcomplicate this endpoint by giving it multiple jobs. Separation of concerns!

@syedfkabir
Copy link
Contributor

badgateway/tt-app#48 should resolve this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants