Allow members to view resources from other teams #251

gazayas · 2023-04-18T10:20:15Z

Context: https://discord.com/channels/836637622432170028/1096469430101344378/1096469437860814930

The goal

To allow members to view resources from other teams. For example, say we have a model Foo, and we add the following in ability.rb (we could probably edit config/models/roles.rb instead, but writing this to match the discussion on Discord):

can :show, Foo

The error

current_membership is called here, but it returns nil resulting in the following error:

The fix

On the ensure_onboarding_is_complete step, we were updating current_user.current_team_id with @team, the team resource being loaded on the page. Since this is originally intended for use within teams current_user already belongs to, we didn't have anything in place to prevent this id from being updated when the member is visiting a team they don't belong to, so the id was being updating with a team they shouldn't be a part of. With this fix, we'll only update the current team if the member belongs to the team.

Navigation bar updates

I'm opening this up as a draft because there is an issue with the navigation bar links. Basically, the links point to the team being loaded, not the current_user's current_team. So, for example, the team name should be Another Team but is Your Team, and if we click on Team Members...

We get redirected to the current team (You can see Another Team on the right side) with an unauthorized error:

gazayas · 2023-04-19T05:21:53Z

Ok, the last commit fixes the navbar issue, and now we display the user's current team as opposed to the loaded resource's team. Marking this one as ready.

The CI tests are failing due to some Firefox installation issue which I noticed happened on another PR too, but all the tests are running locally so we should be ok.

gazayas · 2023-04-21T13:19:39Z

Yeah, just re-ran the tests and everything's fine.

jagthedrummer

@gazayas I just rebased this PR and force pushed to get tests running and passing. It's a pretty small PR but seems potentially risky. Just wanted to double check that you're feeling good about it.

gazayas · 2023-08-04T23:41:41Z

@gazayas I just rebased this PR and force pushed to get tests running and passing. It's a pretty small PR but seems potentially risky. Just wanted to double check that you're feeling good about it.

@jagthedrummer Totally understandable. I submitted this PR mainly as a response to a couple of developers asking for the feature, but I agree that it could potentially be risky so I'd be more than glad to wait on another set of eyes. I saw @kaspth agreed to check, so I'll wait until then!

kaspth · 2023-08-07T14:50:47Z

@gazayas I think I was talking about something else there, but I'm trying to look now 😊

kaspth

I tried to add some comments here but it's mostly questions because I don't know this code.

I also don't know what the ramifications are of these changes could, so I'm unsure of it. But if you're confident of this change, then please go ahead 😄

kaspth · 2023-08-07T15:04:40Z

bullet_train/app/controllers/concerns/account/controllers/base.rb

@@ -64,7 +64,7 @@ def ensure_onboarding_is_complete_and_set_next_step
  def ensure_onboarding_is_complete
    # This is temporary, but if we've gotten this far and `@team` is loaded, we need to ensure current_team is
    # updated for the checks below. This entire concept of `current_team` is going away soon.


This entire concept of current_team is going away soon.

Interesting! @gazayas it looks like this was added 2 years ago, do you know what the plans were for this?

I'd love to see user.current_team go away. That kind of thing has the potential to cause unexpected behavior if someone has multiple tabs open on the app and are jumping back and forth between them.

Yeah, that's where I like a Current class to maintain that for us. We might not need to save current_team on the user, it's more of a transient attribute, right?

It looks like we've got a sorta setup Current object (though there's a bug in update_membership, where -> find_by) https://github.com/bullet-train-co/bullet_train-core/blob/3a46ebe4d6992908274e132833c0acfb6dacf40b/bullet_train/app/models/concerns/current_attributes/base.rb

If we know what the behavior is that we want, I can look at updating/fixing Current since I've worked with those before.

I honestly don't know enough about user.current_team and how/where/why we depend on it to be confident that I could describe what behavior we want in any more detail than "do the right thing", which... isn't very helpful.

This entire concept of current_team is going away soon.

Interesting! @gazayas it looks like this was added 2 years ago, do you know what the plans were for this?

@kaspth To be 100% honest I'm not sure entirely what the plans for this were, but using Current to maintain this makes sense to me. If we do remove current_team I could see it having a lot of effects on this PR, but implementing that first and then revisiting this PR might be in our best interest.

@gazayas allright, I'll try to dig into this code and see what I can do with Current.

kaspth · 2023-08-07T15:06:35Z

bullet_train/app/helpers/account/teams_helper.rb

@@ -2,7 +2,7 @@ module Account::TeamsHelper
  def current_team
    # TODO We do not want this to be based on the `current_team_id`.
    # TODO We want this to be based on the current resource being loaded.
-    @team || current_user&.current_team
+    current_user&.current_team || @team


I'm wondering how wide ranging the effects of this could be, but I don't really know. Are there a few specific call sites that we need to update for this, if so what are they? I don't know what they are, but I'm curious if that might mean we could make changes that are perhaps lesser reaching.

It's been a while since I last looked at this PR so I can't tell for sure, but the reason I prioritized current_user&.current_team here is for a reason similar to what I commented about ... && current_user.teams.include?(@team) below. Basically, we were just setting whatever @team showed up as a resource to current_team, when I figured we should return the current_user's current team first. Looking at this again though I'm not entirely sure what the intention of the original setup was, but if we decide to use Current to handle this then maybe we can rethink this portion altogether.

Yeah, of the 3 lines changed in this PR this is the one that gives me pause. I do believe it will change some behavior, and maybe not in a way that we want.

For instance, imagine you belong to Team A & Team B. The last team that you manually changed to is Team A, so current_user.current_team is Team A.

A collaborator on Team B sends you a link to a project that Team B owns. Something like /teams/team_b_id/projects/42.

Currently/previously (before this PR) the current_team would be set to Team B because that's what's been loaded as @team in the process of retrieving the Project.

I think this PR would make it so that current_team is Team A, since that's the last thing that's been persisted to current_user.current_team. I don't think that's what we want.

I'll try to confirm or disconfirm that theory.

I think this PR would make it so that current_team is Team A, since that's the last thing that's been persisted to current_user.current_team. I don't think that's what we want.

Good point, @jagthedrummer!

I pulled down this branch and did some testing, and the concern that I expressed doesn't appear to be valid.

It looks like we set current_user.current_account on every page load. Which seems a little surprising to me.

The reason we do so is that in the starter repo the Account::ApplicationController does include Account::Controllers::Base. And then scaffolded controllers inherit from Account::ApplicationController. In Account::Controllers::Base there's a declaration for before_action :ensure_onboarding_is_complete_and_set_next_step, which calls ensure_onboarding_is_complete. And in ensure_onboarding_is_complete we do:

current_user.update(current_team: @team) if @team&.persisted? && current_user.teams.include?(@team)

I kind of wouldn't expect that we'd call ensure_onboarding_is_complete on literally every signed-in page load, but I could easily imagine how/why it ended up that we do.

I think that if we're confident that calling ensure_onboarding_is_complete on every page load is what we want to be doing, and not just a thing that we happen to be doing, then I think this might be fine. (Though messing with something as foundational as current_team does still give me a bit of the willies.)

kaspth · 2023-08-07T15:07:18Z

bullet_train/app/controllers/concerns/account/controllers/base.rb

@@ -64,7 +64,7 @@ def ensure_onboarding_is_complete_and_set_next_step
  def ensure_onboarding_is_complete
    # This is temporary, but if we've gotten this far and `@team` is loaded, we need to ensure current_team is
    # updated for the checks below. This entire concept of `current_team` is going away soon.
-    current_user.update(current_team: @team) if @team&.persisted?
+    current_user.update(current_team: @team) if @team&.persisted? && current_user.teams.include?(@team)


Is the intent here that we update the current_team when a user visits the show action (or similar) of one of their teams?

The reason I added this conditional here was because since I allowed users to visit other teams that aren't theirs, it would add the team they're visiting as their current team 😬

So I used this as a safeguard to ensure only teams they belong to are updated to current_team.

gazayas marked this pull request as ready for review April 19, 2023 05:22

jagthedrummer force-pushed the fixes/shared-team-resources branch from 4f825b0 to 75555b8 Compare August 4, 2023 17:24

jagthedrummer approved these changes Aug 4, 2023

View reviewed changes

kaspth approved these changes Aug 7, 2023

View reviewed changes

gazayas mentioned this pull request Aug 17, 2023

Super Scaffold with custom namespaces properly #242

Draft

gazayas and others added 3 commits August 31, 2023 15:02

Only update current team if User belongs to it

bfabae1

Give precendence to current user's team in current_team

8abce66

empty commit to trigger CI

24c57be

jagthedrummer force-pushed the fixes/shared-team-resources branch from 75555b8 to 24c57be Compare August 31, 2023 20:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow members to view resources from other teams #251

Allow members to view resources from other teams #251

gazayas commented Apr 18, 2023

gazayas commented Apr 19, 2023

gazayas commented Apr 21, 2023

jagthedrummer left a comment

gazayas commented Aug 4, 2023

kaspth commented Aug 7, 2023

kaspth left a comment

kaspth Aug 7, 2023

jagthedrummer Aug 7, 2023

kaspth Aug 7, 2023

kaspth Aug 7, 2023

jagthedrummer Aug 7, 2023

gazayas Aug 9, 2023

kaspth Aug 9, 2023

gazayas Aug 9, 2023

kaspth Aug 7, 2023

gazayas Aug 9, 2023

jagthedrummer Aug 9, 2023

kaspth Aug 15, 2023

jagthedrummer Aug 31, 2023 •

edited

Loading

kaspth Aug 7, 2023

gazayas Aug 9, 2023 •

edited

Loading

Allow members to view resources from other teams #251

Are you sure you want to change the base?

Allow members to view resources from other teams #251

Conversation

gazayas commented Apr 18, 2023

The goal

The error

The fix

Navigation bar updates

gazayas commented Apr 19, 2023

gazayas commented Apr 21, 2023

jagthedrummer left a comment

Choose a reason for hiding this comment

gazayas commented Aug 4, 2023

kaspth commented Aug 7, 2023

kaspth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagthedrummer Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gazayas Aug 9, 2023 • edited Loading

Choose a reason for hiding this comment

jagthedrummer Aug 31, 2023 •

edited

Loading

gazayas Aug 9, 2023 •

edited

Loading