search

Ada-Developers-Academy / classroom-app

Application for tracking and providing feedback on Ada student PRs
http://classroom.adadevelopersacademy.org/
2 stars 5 forks source link

Handle unknown repo case for group projects #46

Closed Hamled closed 7 years ago

Hamled commented 7 years ago

When GitHub has a PR made from a repository/fork that has been deleted it shows "unknown repository" as the source on the PR page and the API does not include any repo information for the "head" section of the PR data.

We now check whether that information is available from the API and effectively ignore the PR if it does not exist. My understanding is that this means the submission line for each associated student will continue to be listed as unsubmitted, however I do not have a good test case for this situation.

This fix only applies to the logic for group projects as the individual project does not require the same repo information.

Note: The safety check code that I've put in the contributors_url method could be cleaned up a bit if we switch to Ruby 2.3.0 or later, which supports Hash#dig for exactly this purpose.

kariabancroft commented 7 years ago

Looks good - thank you for addressing this. We could potentially still mark the person who submitted the PR as submitted (not using the group/collaborator functionality) - but I'm not sure if that will add any value

Hamled commented 7 years ago

If the repo with the project code doesn't exist anymore (which I think is the only case that would trigger this bug), I don't think we should consider the PR as submitted.

Hamled commented 7 years ago

Actually, it looks like all of the commits are still available somehow (they must be copied somewhere on GitHub's database when you make the PR), so it would still be possible for us to review it and write in feedback.

Should I update this code to mark the PR creator as having submitted, even if the original repo was removed?

Also, given that we technically still have access to each of the commits it actually is possible to get a list of all students involved in the PR. It'd involve a lot more API calls though unless we used the GraphQL API.

Hamled commented 7 years ago

We're sticking with the current version of this fix for now.