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

Approve organization account #11208

Merged
merged 29 commits into from
May 1, 2022

Conversation

divbzero
Copy link
Contributor

@divbzero divbzero commented Apr 18, 2022

  • Add new organization list page for administrators.
  • Add new organization detail page for administrators.
  • Add form to approve/decline organization to organization detail page.
  • Record events for new organization approve/decline.
  • Send emails to user and admins for new organization approve/decline.
  • Closes Approve account #11087.
  • Closes List pending organization requests #11287.

Testing Setup

  1. make serve to run locally.
  2. make initdb to initialize database.
  3. Uncheck and save the disable-organizations admin flag at http://localhost/admin/flags/.
  4. Submit the Create new organization form at http://localhost/manage/organizations/.
  5. Open the request notification email to admins using the maildev interface at http://localhost:1080/.

Testing Steps

  1. Follow the approval link from the notification email to open the organization detail page.
  2. Alternatively, open the detail page from the list page at http://localhost/admin/organizations/.
  3. Click Approve/Decline and follow prompts to approve/decline the new organization.

Expected Behavior

  1. Approve/decline fails if the confirmation prompts are not followed.
  2. Organization approval page indicates when organization is approved/declined.
  3. Approval notification emails to admins and the requesting user appear in the maildev interface at http://localhost:1080/.

@divbzero divbzero marked this pull request as draft April 18, 2022 20:36
@divbzero
Copy link
Contributor Author

@ewdurbin This pull request is fully working and tested and is ready for review as soon as we merge #11184.

@divbzero divbzero force-pushed the feature/approve-organization-account branch 2 times, most recently from 281f7b3 to 55ef819 Compare April 20, 2022 19:38
@divbzero
Copy link
Contributor Author

Rebased against main to incorporate recent changes:

Events now use Organization.Event and record user.id for additional data.

@divbzero divbzero marked this pull request as ready for review April 20, 2022 19:52
@divbzero divbzero requested a review from ewdurbin April 20, 2022 19:52
divbzero added 16 commits April 26, 2022 13:02
Realized that translations for admin-* emails don't really make sense.
Similar to commit bec7058.
- admin-new-organization-approved
- admin-new-organization-declined
- new-organization-approved
- new-organization-declined
As @ewdurbin pointed out, the approve organziation form in the admin
interface should use the same design as the rest of the admin interface:

- Changed style to AdminLTE theme.
- Moved *Approve* or *Decline* dialog to *Actions* box.
- Added confirmation modals for *Approve* or *Decline*.
- Added *type orgnization name to confirm* to confirmation modals.

The *Actions* box and confirmation modals follow the same patterns used
in the user detail admin page.
Used Organization.events relationship per @sterbo's suggestion.
- Add "Approval Status" to "Organization Request" details
- Allow admin to change approval decision
- Disable "Approve" button if already approved
- Disable "Decline" button if already declined
`Organization.Event` with tag:

- organization.approve
- organization.decline
@divbzero divbzero force-pushed the feature/approve-organization-account branch from 55ef819 to 4995dec Compare April 26, 2022 20:03
@divbzero
Copy link
Contributor Author

Rebased against main to include all changes since #11184.

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.

One request, but it can be addressed later. @s-mm can we ensure we capture the requested feature (list view for organization requests) somewhere?

{% block title %}{{ organization.name }}{% endblock %}

{% block breadcrumb %}
{# TODO: <li><a href="{{ request.route_path('admin.organization.list') }}">Organizations</a></li> #}
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer if we included the list view as part of this PR 1) so we don't forget 2) so admins can view/process requests in bulk rather than having to go email by email link by link.

Ideally this would by default only show requests that have not been acted on, but have the option to display "approved", "declined", "submitted"

@ewdurbin
Copy link
Member

@divbzero if you don't respond saying "oh that's easy I'll add the list admin view real quick" I'll merge this later today.

@divbzero
Copy link
Contributor Author

@ewdurbin It would be a bit more than “real quick” to make sure I do it right so maybe I can address this in another PR? I will be adding a list of pending organization requests for both users and admins.

@divbzero
Copy link
Contributor Author

@ewdurbin I’ve created #11287 to track. This will be top of my list of things to do.

@ewdurbin
Copy link
Member

Thank you so much @divbzero

@ewdurbin ewdurbin enabled auto-merge (squash) April 29, 2022 19:05
@ewdurbin
Copy link
Member

It looks like a test is broken after rebase, or may be flaky.

auto-merge was automatically disabled April 29, 2022 20:05

Head branch was pushed to by a user without write access

@divbzero divbzero force-pushed the feature/approve-organization-account branch from 33fdfec to ce90357 Compare April 29, 2022 20:37
- Avoid unstable sort using `.normalized_name.lower()` as key
- Avoid unreliable comparison of `paginate.Page` and `list`
@divbzero divbzero force-pushed the feature/approve-organization-account branch from ce90357 to 1bccec9 Compare April 29, 2022 20:39
@divbzero
Copy link
Contributor Author

@ewdurbin Thanks, looks like my tests were flaky due to a bad sort key and unreliable comparison of paginate.Page and list. Should be stable now 🤞

@ewdurbin ewdurbin merged commit ad9eb8f into pypi:main May 1, 2022
vrodou referenced this pull request May 2, 2022
* tests, warehouse: validate job_workflow_ref

Add a bunch of counterexample tests to be certain.

* oidc/models: wrap `re.match` to make mypy happy

* tests/oidc: update

* warehouse, tests: fix `job_workflow_ref` regex

* tests, warehouse: refactor `job_workflow_ref` again

* warehouse, tests: refactor `job_workflow_ref` verification again

Co-authored-by: Dustin Ingram <di@users.noreply.github.com>
domdfcoding pushed a commit to domdfcoding/warehouse that referenced this pull request Jun 7, 2022
* admin-new-organization-approved email template

* admin-new-organization-declined email template

* new-organization-approved email template

* new-organization-declined email template

* Remove translations from admin-* emails

Realized that translations for admin-* emails don't really make sense.
Similar to commit bec7058.

* Test *new-organization-{approved,declined} emails

- admin-new-organization-approved
- admin-new-organization-declined
- new-organization-approved
- new-organization-declined

* Mockup of approve organization form for admin

* Add message textarea to approve organization form

* Add more context to approve organization form

* Rename view admin.organization.{approve => detail}

* Implement GET approve organization form

* Revamp UX for approve organization form

As @ewdurbin pointed out, the approve organziation form in the admin
interface should use the same design as the rest of the admin interface:

- Changed style to AdminLTE theme.
- Moved *Approve* or *Decline* dialog to *Actions* box.
- Added confirmation modals for *Approve* or *Decline*.
- Added *type orgnization name to confirm* to confirmation modals.

The *Actions* box and confirmation modals follow the same patterns used
in the user detail admin page.

* Implement POST approve organization form

* Get requesting user for approve organization form

Used Organization.events relationship per @sterbo's suggestion.

* Handle status in approve organization form

- Add "Approval Status" to "Organization Request" details
- Allow admin to change approval decision
- Disable "Approve" button if already approved
- Disable "Decline" button if already declined

* Store id instead of username in new events

`Organization.Event` with tag:

- organization.approve
- organization.decline

* GET /admin/organizations/ to list organizations

* Add "Organizations" to admin sidebar

Show only if `AdminFlagValue.DISABLE_ORGANIZATIONS` is unchecked.

* Test GET /admin/organizations/

* NFC: Rename tests *_{disallow => disable}_organizations

* "Organizations" admin 404 if disable-organizations

404 Not Found for "Organizations" admin if `disable-organizations` admin
flag is checked.

* NFC: `organization_*` prefix for admin org views

* GET /admin/organizations/?q=... faceted search

* GET /admin/organizations?q=... improved search UI

* Update tests for GET /admin/organizations/

* Link breadcrumb to GET /admin/organizations/

* Fix flaky tests for /admin/organizations/

- Avoid unstable sort using `.normalized_name.lower()` as key
- Avoid unreliable comparison of `paginate.Page` and `list`

Co-authored-by: Ee Durbin <ewdurbin@gmail.com>
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.

List pending organization requests Approve account
2 participants