Skip to content

chore: Sync main with dev - #25

Merged
nfebe merged 3 commits into
mainfrom
dev
Sep 20, 2026
Merged

nfebe merged 3 commits into
mainfrom
dev

Conversation

@nfebe

@nfebe nfebe commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Releases 1.2.0.

Role lookups compare the polymorphic context_type against the model's morph class instead of the configured class name, and member lookups match the user side on the name their role assignment was stored under. A host whose subclass keeps the name its table already holds, or that registers a morph map, no longer sees empty workspace and member lists.

Role lookups compared a polymorphic column against the configured class name,
so a host that points the package at a subclass has to store that subclass
name in every row. A host with rows already written under the base name, or
one that registers a morph map, matches nothing and sees no workspaces and no
members.

The comparisons now ask the model through getMorphClass, which answers with the
class name unless the host says otherwise, so what is stored today keeps
working and an override or a morph map is honoured as well.
A role assignment is written through the user's own morph relation, so the
user side of the member lookup holds whatever that answers. Comparing it
against the configured class instead returned no members for a host that maps
its user model, while the same host's inverse lookup worked.

Both sides of the lookup now ask the model, and the controller uses the
workspace it already holds rather than resolving the name a second time.
Releases 1.2.0.
@nfebe
nfebe merged commit 438bef3 into main Sep 20, 2026
3 checks passed
@sourceant

sourceant Bot commented Sep 20, 2026

Copy link
Copy Markdown

Code Review Summary

✨ Release 1.2.0. The change makes role and membership lookups resolve the polymorphic context_type (and the user side assignable_type) through getMorphClass() instead of comparing against the configured class name. This fixes empty workspace and member lists for hosts whose subclass keeps the name already stored in its table, or that register a morph map. Version bumped to 1.2.0 in composer.json with a matching CHANGELOG.md entry.

Read paths are updated consistently in src/Models/Workspace.php (members(), roleAssignments(), getOwnersAttribute(), getAdminsAttribute()), src/Http/Controllers/WorkspaceController.php (members(), userCanAccess(), userCanManage(), userIsOwner(), getUserRole()), and src/Traits/HasWorkspaces.php (workspaces()), with a new private userMorphClass() helper on the model and a public workspaceMorphClass() on the trait. Coverage is added in tests/Feature/SubclassedWorkspaceTest.php for a subclass that renames its morph class and for a user model behind a morph map, including teardown that clears the globally registered morph map.

The write paths in the trait still resolve the morph class from configuration rather than from the instance passed in, so a stored assignment name can still differ from the name every read path uses.

🚀 Key Improvements

  • src/Models/Workspace.php and src/Http/Controllers/WorkspaceController.php resolve context_type via $workspace->getMorphClass(), so hosts using a morph map or a subclass that preserves the stored name no longer see empty member and workspace lists.
  • src/Models/Workspace.php matches the user side on the user model's own morph class via the new userMorphClass() helper, matching how role assignments are written.
  • tests/Feature/SubclassedWorkspaceTest.php adds coverage for a renamed morph class and for a morph-mapped user model, and resets the global morph map in tearDown() so it cannot leak into later tests.

📉 Regressions

  • src/Traits/HasWorkspaces.php: joinWorkspace() writes assignable_type/context_type from the configured model's morph class (static::workspaceMorphClass()) rather than from the $workspace instance it was given, while all read paths use $workspace->getMorphClass(). When the configured class and the instance disagree, the assignment is stored under one name and looked up under another, reproducing the empty-list behaviour this release set out to fix.
  • src/Traits/HasWorkspaces.php: leaveWorkspace() has the same mismatch, deleting role assignments under the configured model's morph class instead of the instance's, so a mismatched morph class can leave the assignment in place and the user still appearing as a member.

🚨 Critical Issues

  • joinWorkspace() receives the concrete $workspace instance, but writes context_type using the morph class of the configured model (static::workspaceMorphClass()). Every read path (Workspace::members(), Workspace::roleAssignments(), WorkspaceController) resolves context_type from $workspace->getMorphClass(). When the configured class and the passed instance disagree (e.g. config still points at the base Workspace while a subclass instance overrides getMorphClass()), the assignment is stored under one name and looked up under another, reproducing exactly the empty-list bug this PR fixes. Resolve the morph class from the instance being joined.
  • Same inconsistency as joinWorkspace(): leaveWorkspace() deletes role assignments using the configured model's morph class instead of the morph class of the $workspace instance it was handed. Removal must target the same context_type value that lookups use ($workspace->getMorphClass()), otherwise a mismatched morph class leaves the assignment in place and the user keeps appearing as a member.

@sourceant sourceant Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review complete. See the overview comment for a summary.


if (method_exists($this, 'assignRole')) {
$this->assignRole($role, static::workspaceModel(), $workspace->id);
$this->assignRole($role, static::workspaceMorphClass(), $workspace->id);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

joinWorkspace() receives the concrete $workspace instance, but writes context_type using the morph class of the configured model (static::workspaceMorphClass()). Every read path (Workspace::members(), Workspace::roleAssignments(), WorkspaceController) resolves context_type from $workspace->getMorphClass(). When the configured class and the passed instance disagree (e.g. config still points at the base Workspace while a subclass instance overrides getMorphClass()), the assignment is stored under one name and looked up under another, reproducing exactly the empty-list bug this PR fixes. Resolve the morph class from the instance being joined.

Suggested change
$this->assignRole($role, static::workspaceMorphClass(), $workspace->id);
$this->assignRole($role, $workspace->getMorphClass(), $workspace->id);

if (method_exists($this, 'removeRole')) {
foreach (Role::values() as $role) {
$this->removeRole($role, static::workspaceModel(), $workspace->id);
$this->removeRole($role, static::workspaceMorphClass(), $workspace->id);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same inconsistency as joinWorkspace(): leaveWorkspace() deletes role assignments using the configured model's morph class instead of the morph class of the $workspace instance it was handed. Removal must target the same context_type value that lookups use ($workspace->getMorphClass()), otherwise a mismatched morph class leaves the assignment in place and the user keeps appearing as a member.

Suggested change
$this->removeRole($role, static::workspaceMorphClass(), $workspace->id);
$this->removeRole($role, $workspace->getMorphClass(), $workspace->id);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant