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

Request organization account #11184

Merged
merged 47 commits into from
Apr 20, 2022

Conversation

divbzero
Copy link
Contributor

@divbzero divbzero commented Apr 15, 2022

  • Create data models for organizations.
  • Define services to create and retrieve organizations.
  • Add form to request new organization to /manage/organizations/ page.
  • Record events for new organization request.
  • Send emails to user and admins for new organization request.
  • Closes Create an Organization account #11070.

Testing Setup

  1. make serve to run locally.
  2. make initdb to initialize the database.
  3. Uncheck and save the disable-organizations admin flag at http://localhost/admin/flags/.

Testing Steps

  1. Submit the Create new organization form at http://localhost/manage/organizations/.
  2. Submit the same form again with the same organization name.

Expected Behavior

  1. Notification emails to admins and the requesting user appear in the maildev interface at http://localhost:1080/.
  2. Form displays validation error when submitted again with the same organization name.
  3. New organization is inserted into the warehouse.organizations database table with is_active IS FALSE and is_approved IS NULL.

divbzero and others added 30 commits April 15, 2022 01:34
On second thought, we probably don't want localization for admin emails.
    docker-compose run web python -m warehouse db revision --autogenerate --message "Create Organization models"
    make reformat

Forced to regenerate migrations due to recent database changes (pypi#11157).
- Organization account name
- Organization name
- Organization URL
- Organization description
- Organization type
Found the droids that we're looking for.
This is a placeholder so we can reference `admin.organization.approve`
as a route in the admin-new-organization-requested email.
- `ManageOrganizationsViews.default_response`
- `ManageOrganizationsViews.manage_organizations()`
- `ManageOrganizationsViews.create_organization()`
@divbzero
Copy link
Contributor Author

@ewdurbin I think this PR is ready for review again — and ready for merge too if everything looks good. Please let us know if there are other issues or questions we should address. Thank you!

The next PR for approving the organization account (#11208) is ready for review too as soon as this PR is merged.

@divbzero
Copy link
Contributor Author

We also added the disable-organizations admin flag with 5034ac0 and 161ab19.

Copy link
Member

@ewdurbin ewdurbin left a comment

Choose a reason for hiding this comment

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

I would like to see us store references to users in the OrganizationEvents as User.id rather than sticking with the status quo, so that is a blocker on merge for me.

As far as the Enum questions go, I believe that can be a migration if we decide down the line, so it's more of a preference.

Otherwise this is 👍🏼 for me.

)
self.organization_service.add_organization_role(
"Owner", self.request.user.id, organization.id
)
Copy link
Member

Choose a reason for hiding this comment

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

For the purposes of traceability we should use User IDs and reconstitute the user on display.

We should also do this elsewhere, since username changes are something that we want to support in the future, but that's not on you :)

)


class OrganizationInvitationStatus(enum.Enum):
Copy link
Member

Choose a reason for hiding this comment

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

I should have done some research up front rather than ask the question, but SQLAlchemy has support for proper postgres enums, I think the biggest advantage is that if we utilize that we get actual types in the DB and it becomes possible to inspect them as well as ensure that columns don't contain references to out of date values since when using a naive enum.Enum we get a varchar/int field that isn't validated.

sterbo and others added 6 commits April 19, 2022 11:02
- `make translations` for warehouse/locale/messages.pot
- Update migration parent for 614a7fcb40ed_create_organization_models.py
- Remove `OrganizationEvents` class
- Add `HasEvents` mixing to `Organization`
- Update references {OrganizationEvents => Organization.Events}
- Update database migration {organization_id => source_id}
`Organization.Event` with tag:

- organization:create
- organization:catalog_entry:add
- organization:organization_role:invite
- organization:organization_role:accepted

`User.Event` with tag:

- account:organization_role:accepted
If there is a validation error, return the existing invalid form instead
of a new blank form so user can actually see that validation error.
- {created_by_id => created_by_user_id}
- {submitted_by_id => submitted_by_user_id}

Discussed with @ewdurbin. Using `*_user_id` seems more clear.
Copy link
Member

@ewdurbin ewdurbin left a comment

Choose a reason for hiding this comment

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

I'm really happy with where this is at.

If there are no objections, I plan to merge.

@divbzero
Copy link
Contributor Author

Note about PostgreSQL enums for future reference: SQLAlchemy supports PostgreSQL enums as @ewdurbin mentioned, but it looks like there have been issues with Alembic supporting PostgreSQL enums (sqlalchemy/alembic#278).

sterbo added a commit to sterbo/warehouse that referenced this pull request Apr 27, 2022
di pushed a commit that referenced this pull request Apr 27, 2022
domdfcoding pushed a commit to domdfcoding/warehouse that referenced this pull request Jun 7, 2022
* admin-new-organization-requested email template

* new-orgnization-requested email template

* Test admin-new-organization-requested email

* Test new-organization-requested email

* Translate *-organization-requested consistently

* `make translations` for *-organization-requested

* Remove translations from admin-* emails

On second thought, we probably don't want localization for admin emails.

* Initial cut at db model architecture

* Ran code thru make reformat and lint (pypa#11070)

* Added tests for new db models (pypa#11070)

* Numerous tweaks to db models

* Require value for is_active and ran reformat.

* Regenerate migrations for Organization models

    docker-compose run web python -m warehouse db revision --autogenerate --message "Create Organization models"
    make reformat

Forced to regenerate migrations due to recent database changes (pypi#11157).

* Numerous tweaks to alembic scripts

* Initial implementation of organization service.

* Implementing organization events functionality.

* Added tests for organization services.

* Added OrganizationFactory class for model.

* Blank /manage/organizations/ page

* Create organization form on /manage/organizations/

- Organization account name
- Organization name
- Organization URL
- Organization description
- Organization type

* Register DatabaseOrganizationService

Found the droids that we're looking for.

* POST /manage/organizations/ database updates

* Add .get_admins method to user service

* POST /manage/organizations/ email notifications

* Blank /admin/organizations/approve/ page

This is a placeholder so we can reference `admin.organization.approve`
as a route in the admin-new-organization-requested email.

* Test GET /manage/organizations/

- `ManageOrganizationsViews.default_response`
- `ManageOrganizationsViews.manage_organizations()`

* Translations for /manage/organizations/

    make translations

* Fixed OrganizationType enum.

* Test POST /manage/organizations/

- `ManageOrganizationsViews.create_organization()`

* Test CreateOrganizationForm

* Placeholder to test /admin/organizations/approve/

Provides code coverage for the blank page added in 2c70616.

* Test `DatabaseUserService.get_admins()`

* NFC: Add comments about intentionally blank page

* Record events for POST /manage/organizations/

Co-Authored-By: sterbo <matt.sterba@gmail.com>

* Test record events for POST /manage/organizations/

* Functional test for POST /manage/organizations/

* Comment out `OrganizationFactory` for future use

Co-Authored-By: sterbo <matt.sterba@gmail.com>

* NFC: Fix camel case for class names

* Converted OrganizationRoleType to SQLAlchemy Enum

* Add disable-organizations global admin flag

* Add `AdminFlagValue.DISABLE_ORGANIZATIONS`

* Modified org name catalog to store normalized name

* {OrganizationEvents => Organization.Events}

- Remove `OrganizationEvents` class
- Add `HasEvents` mixing to `Organization`
- Update references {OrganizationEvents => Organization.Events}
- Update database migration {organization_id => source_id}

* Store id instead of username in new events

`Organization.Event` with tag:

- organization:create
- organization:catalog_entry:add
- organization:organization_role:invite
- organization:organization_role:accepted

`User.Event` with tag:

- account:organization_role:accepted

* Display CreateOrganizationForm errors

If there is a validation error, return the existing invalid form instead
of a new blank form so user can actually see that validation error.

* Tweak naming in events data {*_id => *_user_id}

- {created_by_id => created_by_user_id}
- {submitted_by_id => submitted_by_user_id}

Discussed with @ewdurbin. Using `*_user_id` seems more clear.

Co-authored-by: sterbo <matt.sterba@gmail.com>
domdfcoding pushed a commit to domdfcoding/warehouse that referenced this pull request Jun 7, 2022
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.

Create an Organization account
4 participants