AuburnACM / auacm

The Auburn ACM Website
Apache License 2.0
15 stars 3 forks source link

Submit API and Recent Submission #79

Closed BrandonLMorris closed 8 years ago

BrandonLMorris commented 8 years ago

Changes

Notes

This pull request builds off of #74. That PR should be merged first

WilliamHester commented 8 years ago

This looks good for the most part. I did find a couple of bugs, though:

  1. If you refresh the judge page, sometimes the recent submissions load. Sometimes, they don't.
  2. If you log out/switch users from the judge page, the recent submissions do not change.
BrandonLMorris commented 8 years ago
  1. I actually listed your first point in my initial description of the PR as a known issue. Truth be told, I don't know exactly why the API calls of MainController sometimes don't run on refresh. We should probably look into moving those kinds of field to the module level, or maybe providing them through services or something.
  2. I didn't try logging in/out, but I'm going to bet that it has to do with the first issue. To my knowledge, logging in/out won't trigger the $scope.username field to get reset (at least not in the lower scope of the JudgeController)

If you have suggestions, I'll hear them, but I don't think the root cause is with my code, but how we (don't) provide app-level values.

WilliamHester commented 8 years ago

I think I was able to fix it relatively easily.

  1. Sorry for missing that.
  2. It actually does trigger the $scope.username field to get reset. See this.

Also, here is the code that I added to JudgeController.js to fix the logging-in/out issue.

$scope.$watch('loggedIn', function(newValue, oldValue) {
    if (newValue) {
        getSubmits();
    } else {
        $scope.submitted = [];
    }
});
BrandonLMorris commented 8 years ago

The corrections were made to the routes to make them much more RESTful. Thanks for bringing it up, I don't really understand REST as much as I should and I completely forgot about query string parameters.

I did not go ahead and change the api/submit to api/submissions. I really didn't want to dig into the client code.

WilliamHester commented 8 years ago

I like most of the changes here. I'm just missing why the api/submits/<int:job> was dropped in favor of a query string. Since you are getting a specific submission, it should be in the path, I believe. I feel like it would keep the code cleaner as well, since it seems that you simply made one function into two by adding an if-statement. If you have strong feelings towards the query string, we can leave it, but I think getting an individual submission would best be done through the path.

BrandonLMorris commented 8 years ago

Hm, you're right. I think I originally left it out because it could get misinterpreted as the limit (hence the route madness of my first attempt), and then it got lost in the refactoring.

WilliamHester commented 8 years ago

Cool. Well once that is resolved, I'll merge it in.

BrandonLMorris commented 8 years ago

API design is hard.

WilliamHester commented 8 years ago

That it is. I spent about an hour trying to come up with a good solution to it yesterday.

The pull looks good. I'm merging it in.