refactor: post topic/create #69

forl · 2018-03-13T08:28:18Z

Checklist

npm test passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

topic/create

Description of change

refactor: finish post topic/create

codecov · 2018-03-13T08:32:26Z

Codecov Report

❗ No coverage uploaded for pull request base (master@bf13548). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master      #69   +/-   ##
=========================================
  Coverage          ?   89.25%           
=========================================
  Files             ?       35           
  Lines             ?     1238           
  Branches          ?        0           
=========================================
  Hits              ?     1105           
  Misses            ?      133           
  Partials          ?        0

Impacted Files	Coverage Δ
app/router.js	`96.61% <100%> (ø)`
app/controller/topic.js	`90.9% <100%> (ø)`
config/config.default.js	`100% <100%> (ø)`
config/plugin.js	`100% <100%> (ø)`
app/middleware/topic_per_day_limit.js	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf13548...7d8bbb9. Read the comment docs.

forl · 2018-03-13T08:37:58Z

app/middleware/topic_per_day_limit.js

+
+    if (ctx.status === 302) {
+      // 新建话题成功
+      // TODO: 此处正确做法应该是使用redis的原子操作


这个地方应该用incr操作的，对redis库不熟，就暂时这样实现了，后面抽空看一下。

incr 后面不是实现了吗。可以改改。

cache.incr之前的实现不对，我提了个单独的pr来实现cache.incr，#73

#73 没有问题就merge一下吧

JacksonTian · 2018-03-13T12:53:22Z

app/controller/topic.js

+    const { body } = ctx.request;
+    const title = (_.isString(body.title) && body.title.trim()) || '';
+    const tab = (_.isString(body.tab) && body.tab.trim()) || '';
+    const content = (_.isString(body.t_content) && body.t_content.trim()) || '';


减少对 lodash 的依赖

参数验证还是用 egg-validate 吧

JacksonTian · 2018-03-13T12:55:05Z

app/middleware/topic_per_day_limit.js

+
+    if (ctx.status === 302) {
+      // 新建话题成功
+      // TODO: 此处正确做法应该是使用redis的原子操作


incr 后面不是实现了吗。可以改改。

thonatos · 2018-03-13T12:51:14Z

app/controller/topic.js

+    const { body } = ctx.request;
+    const title = (_.isString(body.title) && body.title.trim()) || '';
+    const tab = (_.isString(body.tab) && body.tab.trim()) || '';
+    const content = (_.isString(body.t_content) && body.t_content.trim()) || '';


#70 看下这里的示例。

thonatos · 2018-03-13T12:54:10Z

app/controller/topic.js

+    const { body } = ctx.request;
+    const title = (_.isString(body.title) && body.title.trim()) || '';
+    const tab = (_.isString(body.tab) && body.tab.trim()) || '';
+    const content = (_.isString(body.t_content) && body.t_content.trim()) || '';

    // 得到所有的 tab, e.g. ['ask', 'share', ..]
    const allTabs = tabs.map(function(tPair) {


const allTabs = tabs.map(tPair => tPair[0])

JacksonTian · 2018-03-14T06:19:44Z

麻烦做下 rebase 吧。

JacksonTian · 2018-03-14T07:38:15Z

$ git checkout topic/create
$ git remote add cnode https://github.com/cnodejs/egg-cnode.git
$ get fetch cnode
$ git rebase -i cnode/master

除了第一条外，其余全部用 f

forl · 2018-03-14T08:40:04Z

@JacksonTian ,好的，你说的这个方法太好用了!

JacksonTian · 2018-03-14T09:34:30Z

app/controller/topic.js

    // 通知被@的用户
    await service.at.sendMessageToMentionUsers(
      content,
      topic._id,
      ctx.user._id
    );
-
    await ctx.redirect('/topic/' + topic._id);


redirect 可以不用 await 的。不过这个好像不是你写的。

JacksonTian · 2018-03-14T09:34:46Z

app/controller/topic.js

-    const tab = ctx.request.body.tab.trim();
-    const content = ctx.request.body.t_content.trim();
+    const { body } = ctx.request;
+    const title = (_.isString(body.title) && body.title.trim()) || '';


减少对 lodash 的依赖。

JacksonTian · 2018-03-14T09:37:34Z

app/middleware/topic_per_day_limit.js

+    const YYYYMMDD = moment().format('YYYYMMDD');
+    const key = `topic_per_user_per_day_${user._id}_${YYYYMMDD}`;
+
+    let currentCount = await service.cache.get(key) || 0;


这个好像有歧义吧。

sinchang · 2018-03-14T11:55:09Z

app/middleware/topic_per_day_limit.js

+module.exports = options => {
+
+  return async function(ctx, next) {
+    const { perDayPerUserLimitCount = 10 } = options || {};


这里改成箭头函数吧，默认值 {} 移动到参数里面

试过这样，但看起来并不简洁呢

const perDayPerUserLimitCount = ({ perDayPerUserLimitCount = 10 }) => (perDayPerUserLimitCount)(options);

改成了现在这样

JacksonTian · 2018-03-14T14:04:14Z

app/controller/topic.js


    // 储存新主题帖
    const topic = await service.topic.newAndSave(
      title,
-      content,
+      t_content,


这个地方叫 t_content 的原因是？

最初的代码是这样，前端是这样传的吧，我可以把前端一起改了

JacksonTian · 2018-03-14T14:04:33Z

app/controller/topic.js

      tab,
      ctx.user._id
    );

+    ctx.logger.info(topic);


这句日志是必要的吗

忘记去掉了

forl commented Mar 13, 2018

View reviewed changes

forl changed the title ~~Topic/create~~ refactor: topic/create Mar 13, 2018

forl changed the title ~~refactor: topic/create~~ refactor: post topic/create Mar 13, 2018

JacksonTian reviewed Mar 13, 2018

View reviewed changes

JacksonTian requested changes Mar 13, 2018

View reviewed changes

thonatos requested changes Mar 13, 2018

View reviewed changes

forl force-pushed the topic/create branch from d8426b1 to 5713ade Compare March 14, 2018 08:37

forl mentioned this pull request Mar 14, 2018

[TODO] egg-validate 集成 #70

Open

JacksonTian requested changes Mar 14, 2018

View reviewed changes

forl force-pushed the topic/create branch from 5713ade to 5a48e92 Compare March 14, 2018 10:35

thonatos approved these changes Mar 14, 2018

View reviewed changes

sinchang suggested changes Mar 14, 2018

View reviewed changes

forl force-pushed the topic/create branch from 5a48e92 to e3dcf0c Compare March 14, 2018 13:02

JacksonTian requested changes Mar 14, 2018

View reviewed changes

refactor: topic/create

a460ab5

forl force-pushed the topic/create branch from e3dcf0c to a460ab5 Compare March 14, 2018 15:17

JacksonTian approved these changes Mar 15, 2018

View reviewed changes

JacksonTian added 2 commits March 15, 2018 11:07

Merge branch 'master' into topic/create

f727a9c

Merge branch 'master' into topic/create

7d8bbb9

JacksonTian merged commit dd2c5df into cnodejs:master Mar 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: post topic/create #69

refactor: post topic/create #69

forl commented Mar 13, 2018

codecov bot commented Mar 13, 2018 •

edited

Loading

forl Mar 13, 2018

JacksonTian Mar 13, 2018

forl Mar 13, 2018

JacksonTian Mar 13, 2018

thonatos Mar 13, 2018

JacksonTian Mar 13, 2018

thonatos Mar 13, 2018

thonatos Mar 13, 2018

JacksonTian commented Mar 14, 2018

JacksonTian commented Mar 14, 2018

forl commented Mar 14, 2018

JacksonTian Mar 14, 2018

JacksonTian Mar 14, 2018

JacksonTian Mar 14, 2018

forl Mar 14, 2018

sinchang Mar 14, 2018

forl Mar 14, 2018

JacksonTian Mar 14, 2018

forl Mar 14, 2018

JacksonTian Mar 14, 2018

forl Mar 14, 2018

refactor: post topic/create #69

refactor: post topic/create #69

Conversation

forl commented Mar 13, 2018

Checklist

Affected core subsystem(s)

Description of change

codecov bot commented Mar 13, 2018 • edited Loading

Codecov Report

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

JacksonTian commented Mar 14, 2018

JacksonTian commented Mar 14, 2018

forl commented Mar 14, 2018

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

codecov bot commented Mar 13, 2018 •

edited

Loading