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

Iteration 1: allow private repo to be viewed #1253

Closed
2 tasks done
Tracked by #324
codecovdesign opened this issue Feb 23, 2024 · 24 comments · Fixed by codecov/gazebo#2685
Closed
2 tasks done
Tracked by #324

Iteration 1: allow private repo to be viewed #1253

codecovdesign opened this issue Feb 23, 2024 · 24 comments · Fixed by codecov/gazebo#2685
Assignees
Labels
Dev-Ready This means the UX is reviewed and ready for prioritization scheduling.

Comments

@codecovdesign
Copy link
Contributor

codecovdesign commented Feb 23, 2024

Problem Statement

Currently, users are unable to view private repositories in both scenarios: 1) that are not configured or 2) that are configured. This can be caused due to user not being activated or part of org free plan. The implications are:

  • they will not be able to see repo guide (to configure repo)
  • the new bundles section
Screenshot 2024-02-23 at 7 33 06 AM

Proposed Update

Enable viewing of all repositories regardless of their configuration status or plan authorization. This way users have access to repo guide and bundles guide.

In the scenario that it's a private repo and that both coverage and bundles is not configured these tabs/guides should be visible at all times:

Coverage Bundle
show_1a show_1b

In the scenario that it is configured, leverage existing messaging:

show_2a

Tasks

  1. rohitvinnakota-codecov
  2. rohitvinnakota-codecov
@codecovdesign codecovdesign added Dev-Ready This means the UX is reviewed and ready for prioritization scheduling. in discovery The design, product, and specifications require refinement and removed Dev-Ready This means the UX is reviewed and ready for prioritization scheduling. labels Feb 23, 2024
@codecovdesign codecovdesign added Dev-Ready This means the UX is reviewed and ready for prioritization scheduling. and removed in discovery The design, product, and specifications require refinement labels Feb 23, 2024
@codecovdesign codecovdesign changed the title Iteration 1a: allow private repo to be viewed Iteration 1: allow private repo to be viewed Feb 29, 2024
@nicholas-codecov
Copy link

I do not understand why a user who is not activated should be able to view a repo, does that not defeat the whole point of paying for more seats? So you can have more activated users

@codecovdesign
Copy link
Contributor Author

codecovdesign commented Mar 12, 2024

@nicholas-codecov we shouldn't block them from configuring a repo. yes, they will be prompted to adjust their seats post-configuration. cc'ing @RulaKhaled we did this from breakdown epic.

here is a related video where it shows user stopping configuration completely due to this:

gh_1253.mp4

we want to keep the momentum going and this is a blocker.

@nicholas-codecov
Copy link

Why can we not prompt them beforehand?

@codecovdesign
Copy link
Contributor Author

@nicholas-codecov it runs into the same issue as seen in the video. what is the downside of allowing configuration?

@nicholas-codecov
Copy link

nicholas-codecov commented Mar 12, 2024

If you let them configure it, they'd most likely assume it would work. You're kind of rug pulling them if you say "Hey you can use this .... actually just kidding pay us"

IMO you shouldn't allow the user to take an action that leads to nothing actually happening

@codecovdesign
Copy link
Contributor Author

@nicholas-codecov understood. worth seeing more context: this is something @RulaKhaled and I broke down iteratively. There is iteration 3 that is looking at your suggestion to pre warn:
Screenshot 2024-03-12 at 11 29 21 AM

@nicholas-codecov
Copy link

Then why don't we just skip this and move onto that, since the user won't be able to see it anyways?

@nicholas-codecov
Copy link

To me this change creates more issues and questions then it does solve anything

@codecovdesign
Copy link
Contributor Author

codecovdesign commented Mar 12, 2024

@nicholas-codecov it's a tradeoff, no perfect answer:

notify at repo level and block ability to configure
- tradeoff 1: user is aware at repo, but unable to move forward with configuration, until resolving the member issues
OR
allow configuration
- tradeoff 2: user is able to configure repo, but member management needs to be resolve

we went with tradeoff 2 as it better supports our product growth / self serve model, since they are only one step away from resolving the issue.

@nicholas-codecov
Copy link

I can see this only to us losing trust with our users. I cannot see any way that this will have a positive effect in anyway as this feels like we're only creating negative user experiences, and I don't see how creating negative experiences leads to growth.

Maybe I'm just missing something major here, but as this currently stands, I do not feel comfortable with this at all.

@nicholas-codecov
Copy link

Also if users only care about PR comments and not the UI, this just let's them never pay us

@codecovdesign
Copy link
Contributor Author

codecovdesign commented Mar 12, 2024

@nicholas-codecov we're definelty aligned in the direction we'd like to go, it's just a matter of the steps/iterations. and for this one it still comes back to to tradeoff 1 or 2.

I'm hoping iteration 3 will help them ahead of the configuration page. You raise a great point, as another iteration, wdyt about confirming that note on the configuration page itself? This way they can proceed with configuration, while also resolving the member / plan problem.

going back to the video, it does still feel like a step forward to not block the momentum of onboarding/configuration of the repo, albeit we need to get better at informing the user about this - that is the overall goal of this epic. Thanks for suggestions 🙏

@codecovdesign
Copy link
Contributor Author

codecovdesign commented Mar 12, 2024

@nicholas-codecov the other part of this is that is allows the ability to see bundles config page and/or bundles related repo generally is blocked today.

In terms of iteration 3, we were looking at adding start-trial too for eligible org/repos. Also, seen here is confirming the message like I showed you earlier or org level, but on repo too something like this:

trial eligible no seats / plan
Screenshot 2024-03-12 at 12 16 58 PM Screenshot 2024-03-12 at 12 16 51 PM

@RulaKhaled
Copy link

@nicholas-codecov I think we should still go with showing onboarding page to non activated users, the main reason is we don't want to block users ability to upload reports to Codecov and actually viewing these results in the dashboard when they are able to pay us.

That being said, I see why this might be seen as tricky; thanks for pointing it out. To solve this problem, I think we can display a banner or message informing users that they can upload reports, but they can only view the repository page if they are activated. Can you help draft a quick banner for this @codecovdesign? Thanks!

@codecovdesign
Copy link
Contributor Author

codecovdesign commented Mar 13, 2024

@RulaKhaled were you thinking something like this: #1253 (comment)? We can tweak the copy, but the idea is it helps reinforce the upgrade/member changes needed.

Also, just to reinforce a point that may be missing is the other tradeoff @RulaKhaled reviewed in breakdown: today these repos/scenarios are not able to see the bundle configuration page (which is free to any org/repo regardless of members/plan etc), nor able to get their repo token - therefore blocked to configure. This helps open up that gate, which is very important priority for feature adoption atm.

@RulaKhaled
Copy link

@codecovdesign yes Kyle thanks, I think we can draft a quick ticket for #1253 (comment) in this iteration to resolve the above concerns.

Also, just to reinforce a point that may be missing is the other tradeoff @RulaKhaled reviewed in breakdown: today these repos/scenarios are not able to see the bundle configuration page (which is free to any org/repo regardless of members/plan etc), nor able to get their repo token - therefore blocked to configure. This helps open up that gate, which is very important priority for feature adoption atm.

yes! Nick feel free to read https://www.notion.so/sentry/Seats-Allocations-Epic-733b98e82b2345c59e830e65d70ea8b1 it describes the issues this iteration is trying to resolve.

@eliatcodecov
Copy link

eliatcodecov commented Mar 13, 2024

Thanks for the discussion here, everyone. I wanted to weigh in on this from a user flow perspective just to make sure I'm understanding the problem and solution correctly. I also wanted to provide some definite ''no go's'' that we shouldn't do because it's unclear to me if they're being considered in the proposal.

First the definite non-starter: users shouldn't be able to see any repos in Codecov that they cannot see on github. I don't think the proposal is recommending that all users can see every repo no matter what, but I wanted to make sure that was clear. We cannot diverge from the SCMs ACL or this will cause big security/compliance problems for us.

Now talking through user flows a bit as I understand them based on this proposal.

  1. A deactivated user clicks on a private repo in Codecov's UI and gets access to the /new page that contains onboarding instructions.
  2. They make the code changes in a PR necessary to integrate codecov into the repo.
  3. Since they're deactivated they will not get the ''first time setup'' codecov PR comment. They will instead get the PR comment that tells them they aren't activated and they need to activate. Is this correct? I think just by default this would be the case.
    a. We could go a step farther and do a "First time but deactivated" PR comment that basically says ''you setup codecov properly and everything is working, but since you're not activated we cannot display coverage information, go here to activate". This would make it abundantly clear to the user they did it right and are just limited by seat count.
  4. The user doesn't activate, but the PR gets merged anyway, and other activated users who open PRs will see the correct Codecov comment, etc.

Based on how our systems work today, I believe this is what the onboarding flow for a new repo would look like if it was performed by a deactivated user. What's interesting is with this change the deactivated user can, from start to finish, onboard a repo. The results aren't particularly interesting, because coverage isn't shown in the PR comment, but they can at least rest assured they did it right.

Some questions I have:

  1. Will this change allow deactivated users to see active private repos in the codecov UI? It probably shouldn't. In other words, as a deactivated user you can onboard a private repo, but you can't actually see the coverage in the codecov ui (or get coverage in pr comments) afterward. This may not apply to bundle analysis today, but eventually will once it's post-beta. So we need that 404 page when a deactivated user visits an otherwise functional repo that ''everything is working, you just can't see it because you aren't activated. Go here to activate"
  2. We have some messaging about the user needing to activate in these UI designs. but we should probably be abundantly clear on /new that the user isn't active. I'm not sure this design is clear enough in that regard, but perhaps I'm missing it. For example, we can display a callout to the effect of "you're not active. While you can continue setting up this repo using the instructions below, you will need to activate to get coverage information on your PRs and see coverage information for this repo in the Codecov UI" In other words, make it really clear they can setup, but there will be some limitations afterwards.

Finally, it's worth thinking about who precisely this impacts and the likelihood of those impacts happening across the user base.

Who is impacted: deactivated users setting up private repos who can't be auto activated because there are no seats or auto activate is turned off.

In the grand scheme of things, this probably isn't an inordinately large number of users, but it's still worth considering. I think the areas we need to hone to make this experience feel not broken for these users are the following:

  1. A reworked first time PR comment in the case that the repo was setup by a deactivated user.
  2. A clear notification on the /new page that tells the deactivated user they can still set things up but will need to be activated to see coverage data.
  3. A 404 page when a deactivated user views an active, uploading repo that communicates ''this repo is working fine, you just can't see it because you need to be activated first."

Full disclosure, I have not deeply explored the designs yet to see if these three things are accounted for. I will do that next, but wanted to weigh in on the full experience here just to make sure we're all in alignment.

@codecovdesign
Copy link
Contributor Author

codecovdesign commented Mar 13, 2024

users shouldn't be able to see any repos in Codecov that they cannot see on github

confirming: this has no relation to this issue and would a bug if private repos were shown unauthorized ✅

Some context related to this iteration: it comes from the goal to breakdown these user blocking scenarios and move forward iteratively, roughly based on the overview in this epic. This issue is an imperfect stepping stone toward a better and wide ranging solution that accounts for all these cases. Also, notably one of the other reasons/tradeoffs it was prioritized is because bundle analysis for private repo with 1> members is blocked from configuration (and they will not be able to get their token for that repo). Are we ok with that happening?

A reworked first time PR comment in the case that the repo was setup by a deactivated user.

Great suggestion, issue captured here: #1414

A clear notification on the /new page that tells the deactivated user they can still set things up but will need to be activated to see coverage data.

part of the epic, captured here: #1320

A 404 page when a deactivated user views an active, uploading repo that communicates ''this repo is working fine, you just can't see it because you need to be activated first."

part of the epic, captured here: #1299

@RulaKhaled
Copy link

Thanks for weighing in here @eliatcodecov, yes that's the expected flow, and i like the PR comment idea we haven't put a lot of thought into it

To answer your questions more clearly:

Will this change allow deactivated users to see active private repos in the codecov UI?

No, we still aim to surface a 401 unauthorized error if the user is not activated. The second iteration aims to better handle these error messages by presenting specific messages that redirect users to the intended page. For example, if you're not activated and you have seats, go to the members page, OR if you are not activated and have no seats left, go to the upgrade page.

We have some messaging about the user needing to activate in these UI designs. but we should probably be abundantly clear on /new that the user isn't active.

I definitely agree and we are pushing to surface the banner shown in #1253 (comment) in this iteration

Thanks for the quick issue drafting Kyle @codecovdesign i think we will have enough room to tackle #1414 in the 3rd iteration

@eliatcodecov
Copy link

With #1320 and #1299, with #1414 to be added, I think this is a pretty robust solution to this use case. I feel comfortable moving forward with it

The second iteration aims to better handle these error messages by presenting specific messages that redirect users to the intended page. For example, if you're not activated and you have seats, go to the members page, OR if you are not activated and have no seats left, go to the upgrade page.

I think this is the right solution. What's the timeline between the first and second iteration? My intuition is telling me it would be helpful if this was a part of the first iteration, but I don't know how big the lift is. If providing a specific message is relatively straightforward in the 401 case, we may want to do it as part of the first iteration since it brings a ton of clarity to why that page is being shown. But if it's a big lift I can understand the desire to push it into a second iteration.

Thoughts on the difficulty of moving this into a first iteration?

@RulaKhaled
Copy link

The first iteration was already handed to the devs in this sprint. The second iteration is to be tackled in the next sprint. I'd say there is not a lot of time between these two iterations. In my opinion, it's good to handle them separately as they touch different areas of the code, and each iteration is trying to resolve a specific issue, we even have separate design links. I don't foresee any delays between these two iterations.

That being said, lmk if you still feel strongly about moving this to the first iteration, it shouldn't be too hard

@codecovdesign
Copy link
Contributor Author

+1, @RulaKhaled we can prioritize the discussed iterations as fast follows, also worth noting there is already a PR for iteration 1: codecov/gazebo#2685. if anything worth unblocking the bundle guide/token.

@eliatcodecov
Copy link

@RulaKhaled and @codecovdesign , got it. If iteration 2 will land on the upcoming sprint I think we're moving along just fine, here. I recommend that we keep pursuing this work.

@rohitvinnakota-codecov
Copy link

Closing this issue as iteration 1 has been merged in with codecov/gazebo#2794.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dev-Ready This means the UX is reviewed and ready for prioritization scheduling.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants