Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(projectTransfer)!: fix asset visibility issue when user joins another organization #5551

Merged

Conversation

rajpatel24
Copy link
Contributor

πŸ—’οΈ Checklist

  1. run linter locally
  2. update all related docs (API, README, inline, etc.), if any
  3. draft PR with a title <type>(<scope>)<!>: <title> TASK-1234
  4. tag PR: at least frontend or backend unless it's global
  5. fill in the template below and delete template comments
  6. review thyself: read the diff and repro the preview as written
  7. open PR & confirm that CI passes
  8. request reviewers, if needed
  9. delete this section before merging

πŸ“£ Summary

Ensures that assets shared with an organization owner remain visible even after the asset owner joins a different organization.

πŸ“– Description

Previously, when an external user shared an asset with an organization owner and later joined a different organization, the asset became invisible to the original organization. This fix ensures that the asset remains visible in both organizations, maintaining expected access permissions. A test case has been added to verify this behavior.

@rajpatel24 rajpatel24 removed the request for review from jnm February 21, 2025 15:37
@rajpatel24 rajpatel24 self-assigned this Feb 21, 2025
@rajpatel24 rajpatel24 changed the title fix(projectTransfer)!: fix asset visibility issue when user transfers to another organization fix(projectTransfer)!: fix asset visibility issue when user joins another organization Feb 21, 2025
Comment on lines 935 to 939
def _add_user_to_organization(self, user, organization):
org_user = OrganizationUser.objects.get(user=user)
Organization.objects.filter(organization_users=org_user).delete()
org_user.organization = self.organization
org_user.organization = organization
org_user.save()
Copy link
Contributor

Choose a reason for hiding this comment

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

Could these lines not be replaced by organization.add_user(user)?
Moreover it would ensure that the org is really a MMO before adding the user to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct!

"""
# Step 1: Create an asset owned by an external user
self._create_asset_by_bob()
asset = Asset.objects.get(owner=self.external_user)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to use the uid of the response to ensure we retrieve the correct asset.

response = self._create_asset_by_bob()
asset = Asset.objects.get(uid=response.data['uid'])

)
another_org.add_user(self.thirduser)

self.client.force_login(self.org_owner)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we force_login again? Isn't self.org_owner already logged in (line 1021)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, self.org_owner is already logged in, so we don't need to force login again. Updated this as well.

Comment on lines 1029 to 1032
another_org = Organization.objects.create(
id='org1234', name='Another Organization', mmo_override=True
)
another_org.add_user(self.thirduser)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess, self.thirduser is the owner of another_org, right?
Would you mind to change the name of the another_org and use a more obvious variable name to represent that thirduser is the owner. Like thiruser_org ;-)

self.client.force_login(self.org_owner)
self._add_user_to_organization(self.external_user, another_org)

# Step 5: Verify that the asset is still visible in OrgA and OrgB
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose that, here, we want to test (only) that OrgB owner can see the asset. We already test the visibility for OrgA previously? Please update your comment. Otherwise, something is missing here.
We are still logged as OrgA owner and we only test the response of the "MyProjects" list.
OrgA owner should see the project in their "MyProjects" list, but OrgB owner should see the project in their "My Org Projects" list.

@rajpatel24 rajpatel24 force-pushed the fix-transferred-owner-shared-with-another-owner branch 3 times, most recently from 850f7b8 to 32a13c0 Compare February 24, 2025 11:59
@rajpatel24 rajpatel24 force-pushed the fix-transferred-owner-shared-with-another-owner branch from 32a13c0 to 0069202 Compare February 24, 2025 12:05
@rajpatel24 rajpatel24 merged commit 1f97bbe into release/2.025.02 Feb 26, 2025
4 checks passed
@rajpatel24 rajpatel24 deleted the fix-transferred-owner-shared-with-another-owner branch February 26, 2025 06:19
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants