Refactor controllers #315
Refactor controllers #315
Conversation
Bumps [golang.org/x/net](https://github.com/golang/net) from 0.33.0 to 0.36.0. - [Commits](golang/net@v0.33.0...v0.36.0) --- updated-dependencies: - dependency-name: golang.org/x/net dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Josh <20374744+jpahm@users.noreply.github.com>
* Added /course/:id/grades endpoint that returns all semesters * Finished all endpoint functionallity, everything seems to work * Removed a parameter I added to gradesAggregation * Undo change in swagger.yaml --------- Co-authored-by: Josh <20374744+jpahm@users.noreply.github.com>
* Empty routes * Minor changes * Working auth * Better env * some ai bs * create if dne and return contents * Revert .gitignore * Don't start storage routes if key not present * Comments * Standardize error messages * Restrict routes with password * restriction comments * Finalize cloud storage --------- Co-authored-by: jpahm <jpahman2003@gmail.com> Co-authored-by: jpahm <20374744+jpahm@users.noreply.github.com>
|
@ruba0s @sachi-jh @simar-rekhi, If you have time, can you please review this big PR? Just test all the course and events endpoints I would say. I will test the grades endpoints in the meantime. |
|
@mikehquan19, Sure |
| "former_offset": 0, // initialize the default value of offset & limit right in the map | ||
| "latter_offset": 0, | ||
| "limit": GetEnvLimit(), | ||
| // Returns the offsets and limit for pagination stage for aggregate endpoints pipeline |
There was a problem hiding this comment.
I notice how in the current version, you are appending the paginate entries directly into mongo.Pipeline. But appending a nil/empty bson.D as a stage might still produce invalid results causing runtime errors in future.
What we could do is initialize paginate with valid $skip stages (0) instead of nil and then override when a param is provided.
@mikehquan19, if you end up following the above route, let me know, we could discuss it in depth.
There was a problem hiding this comment.
@simar-rekhi Initialized default stage for offset. Wdyt?
There was a problem hiding this comment.
yep, just saw it. I think this will do the job!
There was a problem hiding this comment.
If all goes well, we might even be able to test it using one of the unit tests in #283 ... wdyt?
| paginateMap[field] = bson.D{{Key: "$skip", Value: offset}} | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Reviewed configs/setup.go
Looks good to me for the most part!
might require some final touch ups if we end up changing the logic behind
GetAggregateLimit()
There was a problem hiding this comment.
I did review this. Seems okay. But would be great if someone having worked on Astra could give it a read.
@mikehquan19, @sachi-jh, @ruba0s
There was a problem hiding this comment.
I haven't worked on Astra, but this looks good to me too
There was a problem hiding this comment.
again, needs review from @sachi-jh, @ruba0s, and @mikehquan19
Honestly, I did not find any major logic-based concerns. If it helps: it was mostly just comments re-formatted
|
@simar-rekhi Thanks for these feedbacks! Super helpful! I will look into that |
|
@mikehquan19, since there weren't any major issues apart from the |
No description provided.