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

Switch to project user system #1114

Merged
merged 1 commit into from Mar 7, 2017
Merged

Conversation

begedin
Copy link
Contributor

@begedin begedin commented Mar 1, 2017

What's in this PR?

List the changes you made and your reasons for them.

This PR does a major switch from organizationMembers to projectUsers. I tried doing it more granularly, but it ended up being too much of a pain.

The switch here is mostly complete. The couple of minor things that need doing are:

  • Decide what to do with the remaining joinProject function in the credentials service. The one place it's used in could define it directly, or it could trigger a route action and we can define the action on the route. Moved it into the component. It's not the best approach, but there's no point in having just that one function that's used in just that one spot in a separate service. Initially, I wanted to move it to a route action, but with the way our project, project.index, project.settings and project.donate templates are written, the component is used in several spots on several different routes, so we would have to write the action several times. I think we need to reconsider how our templates/routes are organized in these cases. Created issue Discussion: Reconsider the route/template structure for donate and thank-you #1122 to further deal with it.
  • Decide what to do with the user profile page. Right now, it renders user's organizations through the organizationMemberships relation. I recommend instead adding a hasMany('organizations') to users as an inverse of organization.owner and rendering that. We can do this in a separate issue. created Discussion: Should we maintain and render a list of user organizations? #1121 to discuss
  • Also in a separate issue, possibly add a list of the user's project's on their profile page. Both those they own (as an inverse of project.owner) as well as those they contribute to (via user.ProjectUsers). - Actually added this by just renaming things. Seems to be working fine.
  • General code cleanup. There's some duplication marked with a TODO, and I'm sure there's unused code we need to remove
  • Decide what to do with the final remnants of organizationMemberships in the mirage scenarios, etc. - I just outright removed all that I could find.

@joshsmith
Copy link
Contributor

I think maybe we just kill organizations on a user's page and instead do something with projects.

@joshsmith
Copy link
Contributor

Wouldn't unused code be caught by coveralls @begedin?

@joshsmith joshsmith self-requested a review March 2, 2017 03:43
@joshsmith
Copy link
Contributor

joshsmith commented Mar 2, 2017

The project card appears to still be using organization members, as does I think the sidebar. Not sure where else. Is this an oversight, or intended to be done as a separate issue?

I'm having a bit of trouble knowing how to review since there seems to potentially be a decent number of such instances.

Here are unique file results for git grep organization-membership

app/components/organization-members.js
app/models/organization.js
app/models/user.js
app/styles/app.scss
app/styles/components/organization-members.scss
app/templates/components/organization-profile.hbs
app/templates/project/index.hbs
mirage/config.js
mirage/scenarios/default.js
tests/acceptance/profile-test.js
tests/integration/components/organization-members-test.js
tests/pages/components/organization-members.js
tests/pages/components/organization-profile.js
tests/pages/organization.js
tests/unit/models/organization-membership-test.js
tests/unit/models/organization-test.js
tests/unit/models/project-test.js
tests/unit/models/user-test.js

And git grep organizationMember

app/components/organization-profile.js
app/components/project-card.js
app/components/user-details.js
app/controllers/project/index.js
app/models/organization.js
app/models/organization.js
app/models/organization.js
app/models/user.js
mirage/models/organization.js
mirage/models/user.js
tests/acceptance/organization-settings-profile-test.js
tests/acceptance/organization-settings-profile-test.js
tests/acceptance/organization-test.js
tests/acceptance/projects-test.js
tests/integration/components/organization-members-test.js
tests/integration/components/organization-members-test.js
tests/integration/components/organization-profile-test.js
tests/integration/components/organization-profile-test.js
tests/integration/components/organization-profile-test.js
tests/integration/components/project-card-test.js
tests/pages/components/organization-profile.js
tests/pages/components/organization-profile.js
tests/unit/models/organization-test.js
tests/unit/models/user-test.js

And git grep member

app/components/member-list-item.js
app/components/organization-members.js
app/components/project-card-members.js
app/components/project-card.js
app/components/task-card.js
app/controllers/project/index.js
app/models/organization-membership.js
app/models/organization.js
app/models/user.js
app/routes/project/tasks/index.js
app/routes/project/tasks/new.js
app/styles/app.scss
app/styles/components/member-list-item.scss
app/styles/components/organization-members.scss
app/templates/components/member-list-item.hbs
app/templates/components/organization-members.hbs
app/templates/components/organization-profile.hbs
app/templates/components/organization-settings.hbs
app/templates/components/project-card-members.hbs
app/templates/components/project-card.hbs
app/templates/components/task-list-cards.hbs
app/templates/privacy.hbs
app/templates/project/index.hbs
app/templates/project/settings/contributors.hbs
app/templates/project/tasks/index.hbs
app/utils/create-task-user-options.js
mirage/config.js
mirage/models/organization-membership.js
mirage/models/user.js
mirage/scenarios/default.js
tests/acceptance/contributors-test.js
tests/acceptance/organization-settings-profile-test.js
tests/acceptance/organization-test.js
tests/acceptance/profile-test.js
tests/acceptance/projects-test.js
tests/acceptance/task-list-test.js
tests/integration/components/member-list-item-test.js
tests/integration/components/organization-members-test.js
tests/integration/components/organization-profile-test.js
tests/integration/components/project-card-members-test.js
tests/integration/components/project-card-test.js
tests/integration/components/task-card-test.js
tests/integration/components/team-member-test.js
tests/pages/components/member-list-item.js
tests/pages/components/organization-members.js
tests/pages/components/organization-profile.js
tests/pages/components/project-card.js
tests/pages/contributors.js
tests/pages/organization.js
tests/unit/abilities/task-test.js
tests/unit/models/organization-membership-test.js
tests/unit/models/organization-test.js
tests/unit/models/project-test.js
tests/unit/models/user-test.js

This excludes some irrelevant stuff but does not attempt to deduplicate and is likely not exhaustive. There are also likely some irrelevant things in here.

But it does appear that we have more than just some cleanup to do, as at least a few components will change in some fundamental way and several parts of the UI appear to be impacted.

@begedin
Copy link
Contributor Author

begedin commented Mar 2, 2017

A good part of that is simply removal of all references to organization memberships, which I did not do.

I also definitely missed a few, mostly doe to not having tests. I'm not sure what the best approach to handle it is. I would say we try to switch the remainder here, then deal with cleanup in another PR.

@begedin
Copy link
Contributor Author

begedin commented Mar 6, 2017

This is good for another review, and hopefully, for a merge. I think I did as much as I could with it. It's gonna be a pain to review, but changing one think kept breaking another until I cleaned it all up. I managed to reorder and organize the commits, at least, so I guess reviewing individual commits might make it easier.

@begedin
Copy link
Contributor Author

begedin commented Mar 6, 2017

The drop in coverage is unavoidable. None of the remaining files have a coverage drop, but we did remove a couple of well-tested files in this PR, which drops the overall average.

Get rid of obsolete code

Get rid of organization memberships completely

Get rid of credentials service
@joshsmith joshsmith force-pushed the switch-to-project-user-system branch from 3dadbce to 996ea56 Compare March 7, 2017 01:35
@joshsmith joshsmith merged commit 83f3895 into develop Mar 7, 2017
@joshsmith joshsmith deleted the switch-to-project-user-system branch March 7, 2017 02:40
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.

None yet

2 participants