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

Implementation of the /api/days method, some documentation updates #28

Draft
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

kolayne
Copy link
Member

@kolayne kolayne commented Jan 17, 2021

Relates to #19

So sorry I make backwards-incompatible updates of the docs, while front-end is being built on top(?) of them! :'(
Apparently, I didn't think deep enough when drawing the documentation up. There are mostly clarifications or new details added, but there are currently at least two incompatible fixes: I replaced the 400 Bad Request status code with 412 Precondition Failed for some cases and updated date format for one method. So, @tanya-kta and anyone else who needs the documentation, please, take a look at the changes and tell me if there is something unclear about what I've added (it's a bit late now, so I might have written some nonsense or explained something too complicatedly)

Welcome the new method for adding new days!

Is blocked by #29 because of the following:

session, _ := cookieStorage.Get(r, "session")
// Panics if user is not authorized. Will be fixed with the appropriate middleware
userID := session.Values["id"].(int)

… cases

Also added some details on 400/412 responses to the general methods description above
- Added a clarification of the status selection when there are multiple errors
- [BACKWARD COMPATIBILITY BREAKING] Updated date format in the docs of `/api/days`
- Added much more details about errors which may occur in `/api/days`
- Added the `error_type` field to the response of `/api/days`, helping to identify the error occurred during a request processing based on the response
- Moved response description to the "Response" subsection of the docs of `/api/days`
Wow, it's so big, there are so many TODOs... I haven't even checked that this is working yet
Comment on lines 133 to 137
// TODO: the two lines below are formatted automatically this way. I wonder if it is possible to write it better
if len(r.Form["activity_type"]) != len(r.Form["activity_proportion"]) ||
len(r.Form["emotion_type"]) != len(r.Form["emotion_proportion"]) {

js, err := json.Marshal(map[string]interface{}{"ok": false,
Copy link
Member Author

Choose a reason for hiding this comment

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

@DarkSquirrelComes, I am planning to request your complete review on the PR when I finish it, but I have a question so far. I hate how these lines look together (especially if I remove the blank line). Is there a nice way to fix it? Our hard limit is 120 characters per line (including the indentation, my tab size is 4)

I would also be thankful if you have checked other TODOs added in this PR and tell me if there is something you disagree with or know a better way for. Thank you!

Copy link
Member

Choose a reason for hiding this comment

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

This autoformat seems good-looking to me. But I don't really understand, why you compare lens of this values?

Copy link
Member Author

Choose a reason for hiding this comment

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

This autoformat seems good-looking to me.

Then should I level the empty line there? Look what happens if I remove it:

	// TODO: the two lines below are formatted automatically this way. I wonder if it is possible to write it better
	if len(r.Form["activity_type"]) != len(r.Form["activity_proportion"]) ||
		len(r.Form["emotion_type"]) != len(r.Form["emotion_proportion"]) {
		js, err := json.Marshal(map[string]interface{}{"ok": false,

I can't understand where the condition of this if ends and the body begins based on the indentation!

But I don't really understand, why you compare lens of this values?

Each activity/emotion here is described with one type and one proportion. I ensure there exists a type for each proportion and a proportion for each value. You can see more details on this method in the documentation

Still don't fully understand the rules of Markdown indentation... Hope, this is correct
(oops, accidentally fixed an older issue...)
kolayne and others added 3 commits January 25, 2021 17:03
Adding new days is now done via one transaction

Co-authored-by: Nikolay Nechaev <[email protected]>
Only activities and emotions which belong to user X can now be added to day  owned by user X

Co-authored-by: Nikolay Nechaev <[email protected]>
Updated the way to retrieve the id of the added day, added duplicated day error handling
…ode more specific

Also added a comment about the constraints names to the scheme file
# Conflicts:
#	database_scheme.sql
#	src/webhandlers.go
@kolayne kolayne mentioned this pull request Feb 1, 2021
…k in `HandleAPIDays`

Compare the value to the `MaxProportion` constant, not to the number `100`
@kolayne kolayne force-pushed the api_days branch 2 times, most recently from 53cdcc3 to aeb1c6e Compare February 8, 2021 08:15
"Type id" is an object in the SQL scheme, which is called "type" in the API docs
@kolayne
Copy link
Member Author

kolayne commented Feb 8, 2021

That's pretty much it. Waiting for @VladimirKoryakin to resolve #29.

I would also like the reviewers (@DarkSquirrelComes, @tanya-kta) to pay additional attention to comparing the documented and the actual behaviours of the method in this PR! Please, ensure all the errors expected to be returned are returned in all the appropriate cases

@kolayne
Copy link
Member Author

kolayne commented Feb 8, 2021

Note: I want both two reviews on this

@kolayne
Copy link
Member Author

kolayne commented Apr 11, 2021

One more thing: the database schema is invalid because PostgreSQL does not support CHECK constraints that reference data that is not in the row being inserted or updated (docs). This must be fixed before the PR can be merged (if this will ever happen)

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

Successfully merging this pull request may close these issues.

3 participants