Skip to content

Commit 9b1af95

Browse files
fix(organizations): address CodeRabbit review comments
- SKILL.md: comment out raw API commands in 6b (source of truth is monitoring.md) - SKILL.md: resolve upstream default branch dynamically instead of hardcoding master - SKILL.md: use explicit base branch for rebase instead of origin/HEAD - Controller: fix JSDoc @returns to Promise<void> for all async handlers - Repository: fix JSDoc @returns to Promise<Object|null> for get() - Repository: throw on missing filter in deleteMany instead of silent undefined - Service: parallelize user org reassignment with Promise.all in remove() - Service: remove redundant userDoc fetch in acceptInvite, reuse existing user - Tests: add non-owner access denial test for pending requests endpoint
1 parent 80dc0bf commit 9b1af95

6 files changed

Lines changed: 41 additions & 23 deletions

File tree

.claude/skills/pull-request/SKILL.md

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -153,13 +153,14 @@ If `no checks reported`, retry up to 5 times (30s apart). If still no checks aft
153153

154154
Grace period: `sleep 180`. If 0 bot comments after 3 min, wait 2 more min.
155155

156-
Read **only unresolved threads** (resolved = ignored). See `references/monitoring.md` for exact commands.
156+
Read **only unresolved threads** (resolved = ignored). Use `references/monitoring.md` as source of truth.
157157

158158
```bash
159-
gh pr view $PR --json reviews,comments
160-
gh api repos/$OWNER/$REPO/pulls/$PR/comments --paginate | jq 'map({id, user: .user.login, body})'
161-
gh api repos/$OWNER/$REPO/issues/$PR/comments --paginate | jq 'map({id, user: .user.login, body})'
162-
# Unresolved threads → see references/monitoring.md
159+
# Optional context only (do not drive action from these):
160+
# gh pr view $PR --json reviews,comments
161+
# gh api repos/$OWNER/$REPO/pulls/$PR/comments --paginate | jq 'map({id, user: .user.login, body})'
162+
# gh api repos/$OWNER/$REPO/issues/$PR/comments --paginate | jq 'map({id, user: .user.login, body})'
163+
# Action source of truth: unresolved threads query in references/monitoring.md
163164
```
164165

165166
**Actionable** (must fix): change requests, bug reports, missing tests, security issues, code suggestions.
@@ -174,9 +175,11 @@ For each actionable comment, check if the file exists unmodified in upstream:
174175

175176
```bash
176177
STACK_REPO=$(git remote get-url devkit-node 2>/dev/null | sed 's|.*github.com[:/]||;s|\.git$||')
177-
git fetch devkit-node master --quiet 2>/dev/null
178+
STACK_DEFAULT_BRANCH=$(git remote show devkit-node 2>/dev/null | sed -n '/HEAD branch/s/.*: //p')
179+
git fetch devkit-node "$STACK_DEFAULT_BRANCH" --quiet 2>/dev/null
178180
# STACK if exists in upstream AND no local diff; else DOWNSTREAM
179-
git cat-file -e devkit-node/master:<file-path> 2>/dev/null && git diff --quiet devkit-node/master -- <file-path> 2>/dev/null
181+
git cat-file -e "devkit-node/$STACK_DEFAULT_BRANCH:<file-path>" 2>/dev/null && \
182+
git diff --quiet "devkit-node/$STACK_DEFAULT_BRANCH" -- <file-path> 2>/dev/null
180183
```
181184

182185
- **Stack-level** → create issue on stack repo with review comment details, reply with issue link, resolve thread. If `gh issue create` fails, fix locally instead.
@@ -235,7 +238,8 @@ REPEAT:
235238
## 8. Conflict resolution
236239

237240
```bash
238-
git fetch origin && git rebase origin/HEAD
241+
BASE_BRANCH=$(git remote show origin | sed -n '/HEAD branch/s/.*: //p')
242+
git fetch origin "$BASE_BRANCH" && git rebase "origin/$BASE_BRANCH"
239243
# Resolve conflicts, then: git add <files> && git rebase --continue
240244
git push --force-with-lease origin HEAD
241245
```

modules/organizations/controllers/organizations.membershipRequest.controller.js

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import MembershipService from '../services/organizations.membership.service.js';
1010
* @description Endpoint to create a pending membership (join request) for an organization.
1111
* @param {Object} req - Express request object
1212
* @param {Object} res - Express response object
13-
* @returns {void}
13+
* @returns {Promise<void>}
1414
*/
1515
const create = async (req, res) => {
1616
try {
@@ -29,7 +29,7 @@ const create = async (req, res) => {
2929
* @description Endpoint to list pending join requests for an organization (owner/admin).
3030
* @param {Object} req - Express request object
3131
* @param {Object} res - Express response object
32-
* @returns {void}
32+
* @returns {Promise<void>}
3333
*/
3434
const listPending = async (req, res) => {
3535
try {
@@ -67,7 +67,7 @@ const approve = async (req, res) => {
6767
* @description Endpoint to reject a pending membership request.
6868
* @param {Object} req - Express request object
6969
* @param {Object} res - Express response object
70-
* @returns {void}
70+
* @returns {Promise<void>}
7171
*/
7272
const reject = async (req, res) => {
7373
try {
@@ -83,7 +83,7 @@ const reject = async (req, res) => {
8383
* @description Endpoint to list the authenticated user's own pending requests.
8484
* @param {Object} req - Express request object
8585
* @param {Object} res - Express response object
86-
* @returns {void}
86+
* @returns {Promise<void>}
8787
*/
8888
const listMine = async (req, res) => {
8989
try {
@@ -99,7 +99,7 @@ const listMine = async (req, res) => {
9999
* @description Endpoint to invite a user to an organization by email.
100100
* @param {Object} req - Express request object
101101
* @param {Object} res - Express response object
102-
* @returns {void}
102+
* @returns {Promise<void>}
103103
*/
104104
const invite = async (req, res) => {
105105
try {
@@ -121,7 +121,7 @@ const invite = async (req, res) => {
121121
* @description Endpoint to accept an organization invite by token.
122122
* @param {Object} req - Express request object
123123
* @param {Object} res - Express response object
124-
* @returns {void}
124+
* @returns {Promise<void>}
125125
*/
126126
const acceptInvite = async (req, res) => {
127127
try {
@@ -138,7 +138,7 @@ const acceptInvite = async (req, res) => {
138138
* @description Endpoint to get invite details by token.
139139
* @param {Object} req - Express request object
140140
* @param {Object} res - Express response object
141-
* @returns {void}
141+
* @returns {Promise<void>}
142142
*/
143143
const getInvite = async (req, res) => {
144144
try {
@@ -158,7 +158,7 @@ const getInvite = async (req, res) => {
158158
* @param {Object} res - Express response object
159159
* @param {Function} next - Express next middleware function
160160
* @param {String} id - The membership request ID
161-
* @returns {void}
161+
* @returns {Promise<void>}
162162
*/
163163
const requestByID = async (req, res, next, id) => {
164164
try {

modules/organizations/repositories/organizations.membership.repository.js

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ const create = (membership) => new Membership(membership).save().then((doc) => d
4040
* @function get
4141
* @description Data access operation to fetch a single membership by its ID.
4242
* @param {String} id - The ID of the membership to fetch.
43-
* @returns {Object|null} The retrieved membership or null if the ID is not valid.
43+
* @returns {Promise<Object|null>} The retrieved membership or null if the ID is not valid.
4444
*/
4545
const get = (id) => {
4646
if (!mongoose.Types.ObjectId.isValid(id)) return null;
@@ -86,7 +86,8 @@ const count = (filter) => Membership.countDocuments(filter).exec();
8686
* @returns {Promise<Object>} A confirmation of the deletion.
8787
*/
8888
const deleteMany = (filter) => {
89-
if (filter) return Membership.deleteMany(filter).exec();
89+
if (!filter) throw new Error('deleteMany requires a filter');
90+
return Membership.deleteMany(filter).exec();
9091
};
9192

9293
/**

modules/organizations/services/organizations.crud.service.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -167,13 +167,13 @@ const remove = async (organization) => {
167167
await MembershipRepository.deleteMany({ organizationId: orgId });
168168

169169
// For each affected user, switch to their next available org or set null
170-
for (const u of affectedUsers) {
170+
await Promise.all(affectedUsers.map(async (u) => {
171171
const remaining = await MembershipRepository.list({ userId: u._id, status: 'active' });
172172
const nextOrg = remaining.length > 0
173173
? (remaining[0].organizationId._id || remaining[0].organizationId)
174174
: null;
175175
await UserService.updateById(u._id, { currentOrganization: nextOrg });
176-
}
176+
}));
177177

178178
// Delete all tasks belonging to this organization (Task module may not exist in all projects)
179179
try {

modules/organizations/services/organizations.membership.service.js

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -355,9 +355,8 @@ const acceptInvite = async (token, userId) => {
355355
membership.inviteToken = null;
356356
const result = await MembershipRepository.update(membership);
357357

358-
const userDoc = await UserService.getBrut({ id: String(userId) });
359-
if (userDoc && !userDoc.currentOrganization) {
360-
await UserService.updateById(userDoc._id, {
358+
if (user && !user.currentOrganization) {
359+
await UserService.updateById(user._id, {
361360
currentOrganization: membership.organizationId._id || membership.organizationId,
362361
});
363362
}

modules/organizations/tests/organizations.domainJoin.e2e.tests.js

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,20 @@ describe('Organizations domain join E2E tests:', () => {
181181
expect(err).toBeFalsy();
182182
}
183183
});
184+
185+
test('member cannot list pending requests — returns empty array — 200', async () => {
186+
try {
187+
const result = await agentMember
188+
.get(`/api/organizations/${org._id}/requests`)
189+
.expect(200);
190+
191+
expect(result.body.data).toBeInstanceOf(Array);
192+
expect(result.body.data).toHaveLength(0);
193+
} catch (err) {
194+
console.log(err);
195+
expect(err).toBeFalsy();
196+
}
197+
});
184198
});
185199

186200
// Mongoose disconnect

0 commit comments

Comments
 (0)