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

Import: add organization/team field first in the project creation UI #11111

Closed
agjohnson opened this issue Nov 19, 2021 · 18 comments · Fixed by #11558
Closed

Import: add organization/team field first in the project creation UI #11111

agjohnson opened this issue Nov 19, 2021 · 18 comments · Fixed by #11558
Assignees
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code

Comments

@agjohnson
Copy link
Contributor

When import a project by a User with SSO enabled, we want to show the Organization field first and then if the organization doesn't have SSO enabled we should show Team field.

https://github.com/readthedocs/readthedocs-corporate/pull/939#discussion_r440143849

@agjohnson agjohnson added Improvement Minor improvement to code Accepted Accepted issue on our roadmap labels Nov 19, 2021
@agjohnson agjohnson self-assigned this Dec 19, 2023
@agjohnson
Copy link
Contributor Author

Thinking about this one more, it should be a backend change not a front end change.

@agjohnson agjohnson transferred this issue from readthedocs/ext-theme Feb 13, 2024
@agjohnson agjohnson removed their assignment Feb 13, 2024
@humitos humitos changed the title Add organization/team field first in the project creation UI Import: add organization/team field first in the project creation UI Feb 13, 2024
@humitos
Copy link
Member

humitos commented Feb 13, 2024

Can you describe a little more what you have in mind here?

@agjohnson
Copy link
Contributor Author

I think we should do this change at the underlying form, and the templates should retain just a simple {% form|crispy %}. We can do this with JS if needed, which was maybe the original idea, but that seems like maybe a last resort option to me.

@humitos
Copy link
Member

humitos commented Feb 14, 2024

Yeah, I think we originally thought this couldn't be a backend change because of "users belonging to multiple organizations". So, depending on what the organization they want to import a project to, there may be Teams or not.

The approach we could follow here is presenting an intermediary page that lists all the organizations the user belongs to (in case there is more than one), then the user selects the organization and then show the actual form to import a project with/without the team fields depending on the organization the user has selected.

@agjohnson
Copy link
Contributor Author

At this point, I think that would be my suggestion as well. First, it probably only affects core team and maybe a few other users. But this would also make the logic I added to the project create view much more explicit, as we would have a single organization to test permisisons against. We already have the organization picker UI, so we probably only need to accept the organization as a view/form argument really.

@humitos
Copy link
Member

humitos commented Feb 19, 2024

Yeah, and that pattern seems easier to implement and understand from a customer perspective than dealing with magic and dynamic forms.

@agjohnson agjohnson added this to the New dashboard features milestone Mar 28, 2024
@humitos
Copy link
Member

humitos commented Jun 4, 2024

I just added a project into an organization and I wasn't asked about what Team I wanted to add it to. However, the project appears under the list of projects from that organization. I'm confused because I thought the only way to relate a project to an organization was through "Teams".

Anthony found that we have a Organization.projects.all() relation.

Anyways, we need to let the user to select the team where they want to add the project to.

Probably we need to think more about this whole UX workflow 😄

@humitos humitos moved this from In progress to Planned in 📍Roadmap Jun 4, 2024
@humitos humitos self-assigned this Jul 16, 2024
@humitos
Copy link
Member

humitos commented Aug 13, 2024

I thought a little more about this and made a POC locally. Basically, I added a new step in the import wizard form that ask for the organization upfront, and based on that it shows (via Javascript) the team available:

Screenshot_2024-08-13_15-31-33

This works fine and covers one of the cases we talked here. However, it doesn't work fine for users with only one organization and without using SSO (most of our users, currently) because we will have an extra step in the wizard just to select the team which I think it's too much.

I'm thinking about copying the pattern of pre-render all the available teams in Django and hide/show them via Javascript based on the organization selected. Then, if they have only one organization and they are using VCS SSO we don't show the team input at all.

What do you think about this approach?

@humitos humitos moved this from In progress to Needs review in 📍Roadmap Aug 13, 2024
@agjohnson
Copy link
Contributor Author

agjohnson commented Aug 14, 2024

Hrm. This feels like a good place to just use an API interaction, it will be a bit hacky and awkward to create a mixed application logic/JS form like you're describing -- lots of mixed Knockout bindings and field manipulations just to temporarily hide the fields you're creating.

Is there a strong reason we even want multiple fields for teams? This feels like maybe a separate change. I feel the display of the current field isn't great, but it doesn't feel like the primary issue on this view.

However, if you are just trying to visually separate the teams, I'd suggest using a category search field instead:

https://fomantic-ui.com/modules/search.html#category

This does require template work.

I think an interstitial form wizard page would be best. There are a few cases where we can skip this page -- linked directly from and org team page "Add project" button, owner has one org/team, etc.

@humitos
Copy link
Member

humitos commented Aug 14, 2024

This feels like a good place to just use an API interaction

We don't have an API endpoint to list all the teams for all the organizations. In fact, we deleted the teams expandable field due to security reasons in #11471. We currently don't have a way to get this data through the API.

Is there a strong reason we even want multiple fields for teams?

No, we only need 1 team field. I'm creating multiple fields just as a way to separate/isolate what teams belong/are available to each of the organizations and swap them based on the organization selected.

I think an interstitial form wizard page would be best. There are a few cases where we can skip this page -- linked directly from and org team page "Add project" button, owner has one org/team, etc.

I mentioned the reasons why I found this not working great in my previous comment. Even people with 1 organization need to select the team. So, having a whole new page just to select the team is bad UX, in my opinion.

With the current approach proposed (keeping the organization/team field in the same page) we can show/hide one or both fields based on the authentication method.

@humitos
Copy link
Member

humitos commented Aug 14, 2024

I like the UX pattern I'm proposing here, but I'm not sure how to continue working on this regarding the technical aspects. I will open a new PR that makes usage of search category for now and get some feedback there.

Edit: PR is at readthedocs/ext-theme#450

humitos added a commit to readthedocs/ext-theme that referenced this issue Aug 14, 2024
@stsewd
Copy link
Member

stsewd commented Aug 14, 2024

We don't have an API endpoint to list all the teams for all the organizations. In fact, we deleted the teams expandable field due to security reasons in #11471. We currently don't have a way to get this data through the API.

We do, a replacement was added in that same PR, https://docs.readthedocs.io/en/stable/api/v3.html#organization-teams-list.

@humitos
Copy link
Member

humitos commented Aug 14, 2024

Oh great! I missed that 👍

I'll update my PR to use those endpoints.

@agjohnson
Copy link
Contributor Author

Even people with 1 organization need to select the team.

Yeah I understand, I also mentioned above that we can automatically skip this form for a number of cases. This includes if we link to the project creation from the organization team page. We don't need to show this form if we already know the team.

So, having a whole new page just to select the team is bad UX, in my opinion.

It's not the best UX we could manage here, but I wouldn't agree this is bad UX in either case. A nicer UX would be to give the user dynamic response on why they can't add to a team/org, and put this right up front.

This is the first field you see on GitHub repository creation, and this helps avoid wasting the user's time of filling out the rest of the form:

image

This is possible for us, but if we instead make this an intermediate form we can at least rely on form validation and application logic instead of making this client side validation of the field.


The reason we started discussing an intermediate form seems it may have been lost in some of our discussions here -- I mentioned this in the PR:

A primary problem with our team selection pattern now is that the team selection comes at the very end of the form. I think this maybe got lost from conversations here and why it needs to be first in the project creation UI. There are few scenarios where this is crummy UX for the user:

  • The user doesn't have admin access to a team only finds out at the end when the team isn't listed
  • Or the user fails validation on form submit, which I believe this is the case for SSO read only users trying to add into an SSO organization

Either way, it's the last or second to last interaction the user has with this view, which does feel like bad UX. It would be nicer UX to give this failure as soon as we can, and avoid wasting the user's time. Both an intermediate form and a dynamic field up front in the form address this, but an intermediate form might come even before repository selection.

@humitos
Copy link
Member

humitos commented Aug 19, 2024

I'm getting confused about this and I'm not sure how to move forward here. I think our conversation will benefit from having the use cases well defined. How would be the UX (wizard steps) for the following cases?

  1. the user has only one organization and is admin of a team (most common case)
  2. the user has only one organization and is not admin of any team
  3. the user has only one organization and it has VCS SSO enabled (no teams)
  4. the user has multiple organizations and is admin of multiple teams
  5. the user has multiple organizations and is not admin of any team
  6. the user has multiple organizations and one of them has VCS SSO enabled (no teams)
  7. ... are there more cases that I may be missing?

Could you describe the steps/wizard form for each of these cases clearly? Also, would you show the "Select organization/team" page before selecting the repository?

@agjohnson
Copy link
Contributor Author

Could you describe the steps/wizard form for each of these cases clearly?

I don't think we should have differing forms, fields, or UX for these use cases. We should show the same single field for every use case. This keeps it easy for us to maintain and predictable and consistent for the user.

We can play more with skipping this field later, in another iteration. We don't skip this field for many users now, so this optimization doesn't feel like a requirement for this issue. This feels like separate work entirely.

A category search dropdown like you started to implement still seems like a good way to go. I also noted earlier that we can also accomplish this without replacing the field just yet.

I think the one thing to do here is to make this one, unified field for org and team selection. We'd need to add options for SSO organizations in the dropdown -- under each SSO organization heading a option: "Any user with permissions in this organization". Ultimately, this requires no dynamic front end fields.

Without this, some organizations seemingly attach projects to an organization field and some organizations to a team field. We don't want this confusion, and we already sort of got confused by this ourselves.

The selection for "attach this project to an SSO organization" feels unified and consistent because it's alongside team selection UI instead of a separate, conditional organization field.

Also, would you show the "Select organization/team" page before selecting the repository?

Yes, it feels like it should be the first step. Choosing a team for the project is low effort in comparison to automatically/manually connecting a repository. We want the user to clear the low effort bars before giving them harder work, otherwise they might just waste their time. Choosing a org/team is very likely to block a user from adding a project, repo connection is as well.

We should work on skipping/hiding this step, but that comes next iteration.

@humitos
Copy link
Member

humitos commented Aug 20, 2024

I don't think we should have differing forms, fields, or UX for these use cases. We should show the same single field for every use case. This keeps it easy for us to maintain and predictable and consistent for the user.

Do we want a field like this one?

Screenshot_2024-08-20_12-35-30

Take a look at all the options since there are considered all the auth mechanism we currently support. There should be considered all the possible scenarios, making the user to always have an option to choose. The field will be required.

  1. the user has only one organization and is not admin of any team
  2. the user has multiple organizations and is not admin of any team

Are not contemplated in the previous image, but I imagine we could show a note similar to the one when the user doesn't have an account connected using the pre-validation method for the form:

Screenshot_2024-08-20_12-44-13

@agjohnson
Copy link
Contributor Author

Do we want a field like this one?

That is looking great. We can build on top of this later to communicate these are teams, show org SSO status, etc.

Are not contemplated in the previous image, but I imagine we could show a note similar to the one when the user doesn't have an account connected using the pre-validation method for the form

For cases where we can give the user an error, a validation error will definitely be a better experience than and empty list of teams/orgs 👍

We already do have validation errors for some of these use cases:

if settings.RTD_ALLOW_ORGANIZATIONS:
if self.user_is_nonowner_with_sso:
raise RichValidationError(
_(
"Only organization owners may create new projects "
"when single sign-on is enabled."
),
header=_("Organization single sign-on enabled"),
)
if not self.user_has_admin_permission:
raise RichValidationError(
_(
"You must be on a team with admin permissions "
"in order to add a new project to your organization."
),
header=_("Admin permission required"),
)

These rules did note some tuning too:

# TODO there should be some way to initially select the organization
# and maybe the team too. It's mostly safe to automatically select
# the first organization, but explicit would be better. Reusing the
# organization selection UI works, we only really need a query param
# here.
self.user_is_nonowner_with_sso = all(
[
AdminPermission.has_sso_enabled(self.user),
AdminPermission.organizations(
user=self.user,
owner=False,
).exists(),
]
)
# TODO this logic should be possible from AdminPermission
# AdminPermssion.is_admin only inspects organization owners, so the
# additional team check is necessary
self.user_has_admin_permission = any(
[
AdminPermission.organizations(
user=self.user,
owner=True,
).exists(),
Team.objects.admin(self.user).exists(),
]
)

@humitos humitos moved this from In progress to Needs review in 📍Roadmap Aug 21, 2024
@github-project-automation github-project-automation bot moved this from Needs review to Done in 📍Roadmap Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants