Skip to content

Conversation

@begedin
Copy link
Contributor

@begedin begedin commented Jul 31, 2017

What's in this PR?

The idea of this pull request is to use the knowledge and experience gained in #1350 to create an MVP for the integration tab.

In this MVP, we look at our installations from this point of view:

A user has a list of their own installations (the ones they were the sender off). These can originate from codecorps or from github (the api matches them up by github id).

An organization has a list of connected installations

All user installations which are not also connected installations are considered unconnected installations.

We show a list of unconnected installations (one component), a list of connected installations (another component, also shows repos) and a button to add more installations.

Unconnected installations can be connected, which creates an OrganizationGithubInstallation record.

Repos from a connected installation can then be connected to the project, which creates a ProjectGithubRepo record.

What's not in this PR

Redirection after installation on github.

I feel it's not strictly necessary in an MVP and it complicates our relationships a good degree. Instead, we could just add a message along the lines of

Is your installation not shown here? Click the button below to install our github application to your user or organization account and return here when you do that

We could have the button open a new tab to make it simpler.

Any sort of state indicator

This is something ideal to be added as a separate PR

Any matching of user accessible installations

This is worth a discussion of it's own, and again, ideal to expand upon here.

I'm thinking of a way where even the relationship between the user and the installation is superfluous.

Basically, we could simply have the index endpoint only return installations scoped to current user. It would make sense. The endpoint would first look up the github api to get a list of github ids, then we could fetch our own records filtered using those ids and return that.

If the user is not connected to github, the endpoint just returns no results.

With an approach like that, no actual user relationship with an installation is necessary.

It also doesn't have to be async. Sure, the request to get the installations takes slightly longer than a github-unrelated request, but this is an action that is not often done and in most cases, the user will expect integration stuff to last a bit longer.

What this PR still needs

  • minor UI tweaks
    • make the link open in new tab
    • make the message and the rest of the ui more descriptive
    • loading states
  • tests fixed/written

Also worth discussing

OrganizationGithubAppInstallation

Similarly to the user relationship, I do not think an organization relationship with an installation is strictly necessary. I'm not aware of any benefit it might provide us, unless we extend access rights at a later time and this connection is somehow necessary for that. Even so, it doesn't seem necessary for the time being and we could migrate the data easily if there is a need for it in the future.

As it is right now, we could just as simply link up a repo and a project within any user-accessible installation.

RequestiogOrganization

Further more, the requesting organization relationship also feels unnecessary. The only benefit is provide is the ability to redirect correctly after installing the application on github, and even then, it only provides information about the organization, not the project, so along those lines, we'd need a requesting project instead, or in addition to.

On top of that, this is sort of implicit behavior, slighly confusing and in no way apparent. I'd rather we'd have some sort of explicit relationship. Maybe we could create an installationAttempt/Request which is a sort of singleton record tied to a user, to redirect back from.

That being said, these are just random thoughts. If well documented, a requestingProject relationship would work.

@begedin begedin requested a review from joshsmith July 31, 2017 14:59
@begedin begedin force-pushed the project-integrations-tab-alternate-mvp branch from 481d137 to 1e22eac Compare August 2, 2017 14:40
@begedin begedin force-pushed the project-integrations-tab-alternate-mvp branch from 1e22eac to b7f7ecf Compare August 4, 2017 11:40
@begedin begedin force-pushed the project-integrations-tab-alternate-mvp branch from b7f7ecf to f9059c7 Compare August 4, 2017 15:33
@begedin begedin merged commit fc6fb23 into develop Aug 8, 2017
@begedin begedin deleted the project-integrations-tab-alternate-mvp branch August 8, 2017 08:20
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.

2 participants