feat: infer userPrefix when update maintainers #496
Conversation
elrrrrrrr
commented
Jun 6, 2023
•
elrrrrrrr
commented
- 🧶 When adding a member, query the userPrefix corresponding to the registry.
- 🧶 添加成员时，查询 registry 对应的 userPrefix
elrrrrrrr
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -266,6 +266,7 @@ export class TestUtil { | |||
| } | |||
| return { | |||
| name: user.name, | |||
| displayName: cleanUserPrefix(user.name), | |||
| token, | |||
| authorization: `Bearer ${token}`, | |||
| password, | |||
There was a problem hiding this comment.
这段代码主要是一个针对某个npm仓库的测试工具的实现，以下是代码评审和建议:
第一次修改对应于从 'PackageUtil' 模块中新增了 'cleanUserPrefix' 函数的导入, 但是并没有看到其它相关代码片段。
第二次修改使用了上述新增的函数 'cleanUserPrefix' 来处理用户邮箱 'email' 或是默认的 '${user.name}@example.com' ，以确保把前缀的敏感信息清除掉。这里需要确认清楚 'cleanUserPrefix' 中具体做了什么事情，用例子或注释来规范这个 function。
第三次修改增加了 'displayName' 字段返回结果中，也被 'cleanUserPrefix' 函数处理过。这样可以生成更直观、友好的测试报告。
整体上需要注意的点：
- 是否在维护特定的文档和注释，对于函数 'cleanUserPrefix' （如果未存在）需要补充它的作用。
- 若其他代码调用了 'cleanUserPrefix'， 需要正确地传递参数，以防出现错误或者意外（例如邮箱地址不完整）。
最后，我们建议根据包含 'TestUtil' 类的文件的大小，将其分拆成更小的类 (Class) 或是模块 (Module) 格式，有助于提升代码可读性和可维护性。
elrrrrrrr
force-pushed
the
update-maintainers
branch
from
e8c583b to
1da5842
Compare
| const user = await this.userRepository.findUserByName(maintainer.name); | ||
| if (!user) { | ||
| throw new UnprocessableEntityError(`Maintainer "${maintainer.name}" not exists`); | ||
| } | ||
| users.push(user); | ||
| } | ||
|
|
||
| await this.packageManagerService.replacePackageMaintainers(pkg, users); | ||
| return { ok: true }; | ||
| } |
There was a problem hiding this comment.
这段代码主要是更新一个包的维护者信息，以下是我的一些建议和改进点：
-
在第 5 行处，
pkg是使用ensureRes.pkg!赋值得到的。这里我们可以将其改为可选链（optional chaining）：const pkg = ensureRes?.pkg;，以处理在ensureRes未定义或ensureRes.pkg为 null/undefined 时的异常情况。 -
第 8 行获取源码仓库地址的方法
getSourceRegistry()可能会抛出异常，需要进行错误处理。 -
第 10 行循环维护者列表前，应该判断 registry 和 registry.userPrefix 是否存在；然后，如果存在且维护者列表中的名字没有按照规则设置，则添加正确的前缀。可能更好的方式是调用
this.packageManagerService中的函数来完成这项工作。 -
第 15 行的
throw new UnprocessableEntityError(...)可能引发异常，我们可以将它转为 Promise 返回一个拒绝状态的 promise 来处理可能产生的错误。 -
最后一行不需要最后的空行。
根据以上建议，可以做出如下优化的代码：
export class UpdatePackageController extends AbstractController {
async updateMaintainers(ctx: Context, fullname: string, data: MaintainerDto) {
ctx.tValidate(MaintainerDataRule, data);
let ensureRes;
try {
ensureRes = await this.ensurePublishAccess(ctx, fullname, true);
} catch (error) {
throw new Error(`Unable to publish package ${fullname}: ${error.message}`);
}
const pkg = ensureRes?.pkg;
let users = await Promise.all(data.maintainers.map(async (maintainer) => {
let userPrefix = "";
let name = maintainer.name;
if (pkg) {
try {
const registry = await this.packageManagerService.getSourceRegistry(pkg);
if (registry && registry.userPrefix && !maintainer.name.startsWith(registry.userPrefix)) {
userPrefix = registry.userPrefix;
}
} catch (error) {
throw new Error(`Failed to get registry for package ${pkg.name}: ${error.message}`);
}
}
name = `${userPrefix}${name}`;
const user = await this.userRepository.findUserByName(name);
if (!user) {
return Promise.reject(new Error(`Maintainer "${name}" not exists`));
}
return user;
}));
try {
await this.packageManagerService.replacePackageMaintainers(pkg, users);
} catch (error) {
throw new Error(`Failed to replace maintainers for package ${pkg.name}: ${error.message}`);
}
return { ok: true };
}
}
| @@ -266,6 +266,7 @@ export class TestUtil { | |||
| } | |||
| return { | |||
| name: user.name, | |||
| displayName: cleanUserPrefix(user.name), | |||
| token, | |||
| authorization: `Bearer ${token}`, | |||
| password, | |||
There was a problem hiding this comment.
此代码补丁包含以下更改：
-
新增了清除用户前缀的方法
cleanUserPrefix，并在第 7 行的导入语句和第 242 行的代码中进行了修改。 -
在第 242 行的代码中，使用了
cleanUserPrefix方法来清除用户电子邮件中的前缀。 -
在第 266 行的代码中，在返回对象中添加了新键值对
displayName: cleanUserPrefix(user.name)。
对于此代码补丁，建议进行以下改进：
-
需要检查
cleanUserPrefix()方法的实现以及其他相关方法，并确保它们满足代码的需求。 -
建议在处理敏感信息时采用最佳实践，并考虑使用 password hashing 和安全存储等技术。
Codecov Report
@@ Coverage Diff @@
## master #496 +/- ##
=======================================
Coverage 97.45% 97.45%
=======================================
Files 168 168
Lines 15691 15769 +78
Branches 2018 2027 +9
=======================================
+ Hits 15291 15368 +77
- Misses 400 401 +1
|
[skip ci] ## [3.29.0](v3.28.0...v3.29.0) (2023-06-06) ### Features * infer userPrefix when update maintainers ([#496](#496)) ([e02ea2a](e02ea2a))
