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

Remove JaResource from ProjectGithubRepoController #1046

Conversation

landongrindheim
Copy link
Contributor

@landongrindheim landongrindheim commented Oct 10, 2017

Remove JaResource/Canary from ProjectGithubRepoController

References

Closes #898

Progress on: #864


plug :load_resource, model: ProjectGithubRepo, only: [:show], preload: [:github_repo, :project]
plug :load_and_authorize_changeset, model: ProjectGithubRepo, only: [:create]
plug :load_and_authorize_resource, model: ProjectGithubRepo, only: [:delete]
Copy link
Contributor

Choose a reason for hiding this comment

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

@landongrindheim As you can see, we do actually need to support a delete action.

It's not clear here, but we likely also need to support an index action (it's implicit from us having a filter function before). These are the reasons we're switching away from ja_resource. Lot's of hidden stuff.

Basically, the controller needs to support

  • index (with id filter)
  • show
  • create (with authorization)
  • delete (with authorization)

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use OrganizationGithubAppInstallationController as a sort of template.

@landongrindheim landongrindheim force-pushed the 898-remove-ja-resource-from-github-project-repo-controller branch from 4664b79 to 8b8581d Compare October 10, 2017 18:03
@landongrindheim
Copy link
Contributor Author

@begedin I've added delete and index functionality back in. Can you take a look?

joshsmith
joshsmith previously approved these changes Oct 10, 2017
@joshsmith joshsmith force-pushed the 898-remove-ja-resource-from-github-project-repo-controller branch from 8b8581d to 9b9f786 Compare October 10, 2017 18:53
@joshsmith
Copy link
Contributor

@landongrindheim looks great! I just rebased but have to step away for a moment. Feel free to merge once the tests pass.

@landongrindheim
Copy link
Contributor Author

@joshsmith Merging is blocked for me currently

@joshsmith joshsmith merged commit e29b316 into code-corps:develop Oct 10, 2017
@joshsmith
Copy link
Contributor

Sorry about that! Didn't realize. Dismissed review and just merged for you.

@landongrindheim
Copy link
Contributor Author

Thanks :)

@landongrindheim landongrindheim deleted the 898-remove-ja-resource-from-github-project-repo-controller branch October 10, 2017 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants