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

Replace "User" role with "Partner" role #1628

Conversation

abbiefarr
Copy link
Contributor

@abbiefarr abbiefarr commented Aug 9, 2021

Issue

#1616
Also CityOfDetroit#57

Addresses # (issue)

  • This change addresses the issue in full
  • This change addresses only certain aspects of the issue
  • This change is a dependency for another issue
  • This change has a dependency from another issue

Description

  • Creates a new user_roles table, which holds user ids and their partner/admin permissions.
  • Adds users from the listings_leasing_agents_user_accounts to the new table as partners, as well as any admins from the user_accounts table.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Prototype/POC (not to merge)
  • This change is a refactor/address technical debt
  • This change requires a documentation update
  • This change requires a SQL Script

How Can This Be Tested/Reviewed?

Features manually tested:

  • Partner login and application view
  • Admin login
  • Public listing view
  • Resident login
  • Resident application view
  • Desktop View
  • Mobile View

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have reviewed the changes in a desktop view
  • I have reviewed the changes in a mobile view
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have assigned reviewers
  • I have updated the changelog to include a description of my changes

@netlify
Copy link

netlify bot commented Aug 9, 2021

👷 Deploy request for dev-bloom pending review.
Visit the deploys page to approve it

🔨 Explore the source changes: 7163ef7

@netlify
Copy link

netlify bot commented Aug 9, 2021

👷 Deploy request for jovial-davinci-1d67a0 pending review.
Visit the deploys page to approve it

🔨 Explore the source changes: 9988dc3

@netlify
Copy link

netlify bot commented Aug 9, 2021

👷 Deploy request for dev-partners-bloom pending review.
Visit the deploys page to approve it

🔨 Explore the source changes: 7163ef7

@netlify
Copy link

netlify bot commented Aug 9, 2021

✔️ Deploy Preview for dev-storybook-bloom ready!

🔨 Explore the source changes: 7163ef7

🔍 Inspect the deploy log: https://app.netlify.com/sites/dev-storybook-bloom/deploys/61151bfea1dce40008f89267

😎 Browse the preview: https://deploy-preview-1628--dev-storybook-bloom.netlify.app

Copy link
Collaborator

@seanmalbert seanmalbert left a comment

Choose a reason for hiding this comment

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

Hey @abbiefarr ,
I like your solution as it's a small departure from the current logic and meets the immediate need that the issue calls out. I'm not sure on how well this solution will scale to support more roles or roles with more attributes. At the same time, I don't think we have enough information to layout a proper proposal for a role manager that wouldn't potentially be overkill or create more technical debt, so I'm inclined to continue with this. Does Detroit have more complex needs further down the roadmap?

Please see my comment on the migration script, I think we can assign the partner role up front based off of data in the db.

@abbiefarr
Copy link
Contributor Author

Thanks for pointing me to the listings_leasing_agents_user_accounts table. Exactly what I was looking for!

Re: Detroit's future access control, I don't think they'll need anything more advanced than what would exist after this PR.

@abbiefarr abbiefarr marked this pull request as ready for review August 12, 2021 13:03
Copy link
Collaborator

@seanmalbert seanmalbert left a comment

Choose a reason for hiding this comment

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

Thanks, @abbiefarr , this looks good and the mirgration script works well! Can you resolve the merge conflict with the changelog, then I can get this in.

@seanmalbert
Copy link
Collaborator

Since I wanted to get this in for #1627, I branched this and resolved the conflict. So This got in on #1673 . @willrlin , not sure if this is possible, but if I can get permission to push to CityOfDetroit, I can handle merge conflicts on your team's PRs before merging.

@willrlin
Copy link
Contributor

It looks like @abbiefarr did a merge from upstream yesterday in CityOfDetroit#408. I assume that had everything that changed in #1673.

@willrlin willrlin deleted the contribute/abbiefarr-user-role-for-partners branch August 17, 2021 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants