New issue

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

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

Already on GitHub? Sign in to your account

fix_function_room.member_#173 #211

Merged
merged 29 commits into from Jan 24, 2017

Conversation

Projects
None yet
2 participants
@lijiarui
Member

lijiarui commented Jan 17, 2017

#173
#104

  1. change parseNickMap, add remark value
  2. add contact.ready() to get full contact, and get contact's remark
@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Jan 17, 2017

Member

Sorry, I found I fix what I need, but bring in other bug, so please wait until I fix all

Member

lijiarui commented Jan 17, 2017

Sorry, I found I fix what I need, but bring in other bug, so please wait until I fix all

@lijiarui lijiarui closed this Jan 17, 2017

@lijiarui lijiarui reopened this Jan 17, 2017

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Jan 18, 2017

Member

this commit can help to find member in room by nick/display/remark

  1. add following type:
type MemberQueryFilter = {
  nick?: string
  remark?: string
  display?: string
}
  1. add following Map:
nickMap,
remarkMap,
displayMap,
  1. add following Functions:
parseNickMap()
parseRemarkMap()
parseDisplayMap()
  1. change the parameter in function room.member(name: string) from public member(quaryArg: MemberQueryFilter)
    we can find member by nick, display, remark, code as follows:
room.member({nick: '李佳芮'})
room.member({remark: '哈哈哈'})
room.member({display: '土'})
Member

lijiarui commented Jan 18, 2017

this commit can help to find member in room by nick/display/remark

  1. add following type:
type MemberQueryFilter = {
  nick?: string
  remark?: string
  display?: string
}
  1. add following Map:
nickMap,
remarkMap,
displayMap,
  1. add following Functions:
parseNickMap()
parseRemarkMap()
parseDisplayMap()
  1. change the parameter in function room.member(name: string) from public member(quaryArg: MemberQueryFilter)
    we can find member by nick, display, remark, code as follows:
room.member({nick: '李佳芮'})
room.member({remark: '哈哈哈'})
room.member({display: '土'})
@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Jan 18, 2017

Member

@zixia
I have tested it, it works well with the following function:

async function RoomTest() {
  let room = await Room.find({topic: 'test'})
  await room.ready()
  console.log('member1: ===========')
  console.log(room.member({nick: '李佳芮'}))
  console.log('member2: ')
  console.log(room.member({remark: '哈哈哈'}))
  console.log('member3: ')
  console.log(room.member({display: '土'}))
}

I didn't modify getContact and its related functions, but test shows:

test » room » Room smoking test Error: Cannot read property 'getContact' of undefined

Could you help me to fix? Thanks.

Member

lijiarui commented Jan 18, 2017

@zixia
I have tested it, it works well with the following function:

async function RoomTest() {
  let room = await Room.find({topic: 'test'})
  await room.ready()
  console.log('member1: ===========')
  console.log(room.member({nick: '李佳芮'}))
  console.log('member2: ')
  console.log(room.member({remark: '哈哈哈'}))
  console.log('member3: ')
  console.log(room.member({display: '土'}))
}

I didn't modify getContact and its related functions, but test shows:

test » room » Room smoking test Error: Cannot read property 'getContact' of undefined

Could you help me to fix? Thanks.

@zixia

zixia requested changes Jan 18, 2017 edited

Your PR looks workable, it's good.

However, part of them need to be redesign, please follow my reviews and follow them.

Thanks.

Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/puppet-web/firer.ts Outdated
Show outdated Hide outdated src/puppet-web/firer.ts Outdated
Show outdated Hide outdated src/puppet-web/firer.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated

lijiarui added some commits Jan 18, 2017

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Jan 18, 2017

Member

@zixia done for what you request

Member

lijiarui commented Jan 18, 2017

@zixia done for what you request

@zixia

Please follow my new reviews.

Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Jan 19, 2017

Member

@zixia done as what you said.

function room.member() can find contact by nick/remark/display,

use the order : remark> display>nick

Member

lijiarui commented Jan 19, 2017

@zixia done as what you said.

function room.member() can find contact by nick/remark/display,

use the order : remark> display>nick

@zixia

zixia requested changes Jan 19, 2017 edited

You are almost done! Please apply the new suggestions.

Please also pay attention to the CI tests: at least you should pass one of them(Circle/Travis/Appveyor).

Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Jan 19, 2017

Member

@zixia Thanks for you advice, and I have done what you said.

I'm sorry for not passing CI tests, I didn't use function getContact and I found it is used in function room.ready() in the following function:

    if (!contactGetter) {
      contactGetter = Config.puppetInstance()
                            .getContact.bind(Config.puppetInstance())
    }

Could you help me? Thanks!

Member

lijiarui commented Jan 19, 2017

@zixia Thanks for you advice, and I have done what you said.

I'm sorry for not passing CI tests, I didn't use function getContact and I found it is used in function room.ready() in the following function:

    if (!contactGetter) {
      contactGetter = Config.puppetInstance()
                            .getContact.bind(Config.puppetInstance())
    }

Could you help me? Thanks!

@zixia

Your code looks cleaner now!

Please make the last change, and confirm the code is work without problem(also pass at least one CI test).

Then I'm going to merge it to master.

Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated

lijiarui added some commits Jan 20, 2017

zixia added a commit that referenced this pull request Jan 20, 2017

@zixia

zixia requested changes Jan 20, 2017 edited

The problem with contactGetter is solved by the latest master with commit 279ff97

Now there's another bug with your PR at here: https://ci.appveyor.com/project/Wechaty/wechaty/build/283#L80

 1. test » room » Room smoking test
  AssertionError: should get nick1 from DisplayName   
    ""              "田美坤"
        test/room.spec.ts:104:5

Please fix it, and then check the CI result again.

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Jan 20, 2017

Member

the following bug is caused by room.nick(contact: Contact), you have defined its return value is displayName before, but after I changed the code, its return value is nickName.

 1. test » room » Room smoking test
  AssertionError: should get nick1 from DisplayName   
    ""              "田美坤"
        test/room.spec.ts:104:5

And I changed as you designed before, function room.nick() return displayName, if user doesn't have displayName, it will return nickName.

Member

lijiarui commented Jan 20, 2017

the following bug is caused by room.nick(contact: Contact), you have defined its return value is displayName before, but after I changed the code, its return value is nickName.

 1. test » room » Room smoking test
  AssertionError: should get nick1 from DisplayName   
    ""              "田美坤"
        test/room.spec.ts:104:5

And I changed as you designed before, function room.nick() return displayName, if user doesn't have displayName, it will return nickName.

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Jan 20, 2017

Member

After the commit, the error above disappear, but it still has an error:

✖ test › room › Room smoking test should get nick2 from NickName because there is no DisplayName,    
  t.is(nick2, EXPECTED.memberNick2, 'should get nick2 from NickName because there is no DisplayName, ')
       |               |                                                                               
       ""              "李竹~英诺天使(此号已满)"          

Actually, I have tested, using function room.nick() if one doesn't have a displayName in the room, it will return nickName correctly. But it still failed this test.

I thought because I changed some structure of the member obj, so the test data maybe outdated...
@zixia need your help, thanks.

Member

lijiarui commented Jan 20, 2017

After the commit, the error above disappear, but it still has an error:

✖ test › room › Room smoking test should get nick2 from NickName because there is no DisplayName,    
  t.is(nick2, EXPECTED.memberNick2, 'should get nick2 from NickName because there is no DisplayName, ')
       |               |                                                                               
       ""              "李竹~英诺天使(此号已满)"          

Actually, I have tested, using function room.nick() if one doesn't have a displayName in the room, it will return nickName correctly. But it still failed this test.

I thought because I changed some structure of the member obj, so the test data maybe outdated...
@zixia need your help, thanks.

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Jan 20, 2017

Member

After you fixed this, I believe this PR could be ready for merging.

Member

zixia commented Jan 20, 2017

After you fixed this, I believe this PR could be ready for merging.

lijiarui added some commits Jan 22, 2017

@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Jan 22, 2017

Member

@zixia Finally, I found the reason for not passing the test....

I added contact.ready() in new PR, because the contact data in Class Contact is not the same as the the member in room, so contact.ready() cannot get the full contact data, then it cannot get the correct nickMap and remarkMap...

Maybe you should mock some contact data corresponding the room member, then it can get the full contact data, and get the nickMap, then pass the test...

Member

lijiarui commented Jan 22, 2017

@zixia Finally, I found the reason for not passing the test....

I added contact.ready() in new PR, because the contact data in Class Contact is not the same as the the member in room, so contact.ready() cannot get the full contact data, then it cannot get the correct nickMap and remarkMap...

Maybe you should mock some contact data corresponding the room member, then it can get the full contact data, and get the nickMap, then pass the test...

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Jan 22, 2017

Member

Great. Could you modify the mock data in the unit test to add support for your new code?

Member

zixia commented Jan 22, 2017

Great. Could you modify the mock data in the unit test to add support for your new code?

lijiarui added some commits Jan 23, 2017

@zixia

zixia requested changes Jan 23, 2017 edited

Congratulations, you had your first version PR which passed CI test.

However, your unit test has not set the mock data correctly(some test code also need to be corrected).

Please follow all the reviews, and correct the mock data format.

Tip: Normally, I just use a console.log(JSON.stringify(contact)) to generate the json data, then I use this REAL data to be my mock data.

Looking forward to seeing your new unit test with the correct mock data!

Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated test/room.spec.ts Outdated
Show outdated Hide outdated test/room.spec.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
Show outdated Hide outdated src/room.ts Outdated
@lijiarui

This comment has been minimized.

Show comment
Hide comment
@lijiarui

lijiarui Jan 23, 2017

Member

Done as what you said, but I'm a little bit confused by the following code:

  const mockContactGetter = function (id) {
    return new Promise((resolve, reject) => {
      // if (id !== EXPECTED.id && !(id in CONTACT_LIST)) return resolve({})
      if (id === EXPECTED.id) {
        setTimeout(() => {
          return resolve(RAW_OBJ)
        }, 10)
      }
      if (id in CONTACT_LIST) {
        setTimeout(() => {
          return resolve(CONTACT_LIST[id])
        }, 10)
      }
      return resolve({})
    })
  }

it gets the following error:

✖ Room smoking test Error: Cannot read property 'Symbol(Symbol.iterator)' of undefined

But when I change the code to the following:

  const mockContactGetter = function (id) {
    return new Promise((resolve, reject) => {
      if (id !== EXPECTED.id && !(id in CONTACT_LIST)) return resolve({})
      if (id === EXPECTED.id) {
        setTimeout(() => {
          return resolve(RAW_OBJ)
        }, 10)
      }
      if (id in CONTACT_LIST) {
        setTimeout(() => {
          return resolve(CONTACT_LIST[id])
        }, 10)
      }
    })
  }

all works well.

What I change is just adjust return resolve({}) order.

I googled the error and find this: this

Initialize items to an empty array.

But I still fell confused..

Member

lijiarui commented Jan 23, 2017

Done as what you said, but I'm a little bit confused by the following code:

  const mockContactGetter = function (id) {
    return new Promise((resolve, reject) => {
      // if (id !== EXPECTED.id && !(id in CONTACT_LIST)) return resolve({})
      if (id === EXPECTED.id) {
        setTimeout(() => {
          return resolve(RAW_OBJ)
        }, 10)
      }
      if (id in CONTACT_LIST) {
        setTimeout(() => {
          return resolve(CONTACT_LIST[id])
        }, 10)
      }
      return resolve({})
    })
  }

it gets the following error:

✖ Room smoking test Error: Cannot read property 'Symbol(Symbol.iterator)' of undefined

But when I change the code to the following:

  const mockContactGetter = function (id) {
    return new Promise((resolve, reject) => {
      if (id !== EXPECTED.id && !(id in CONTACT_LIST)) return resolve({})
      if (id === EXPECTED.id) {
        setTimeout(() => {
          return resolve(RAW_OBJ)
        }, 10)
      }
      if (id in CONTACT_LIST) {
        setTimeout(() => {
          return resolve(CONTACT_LIST[id])
        }, 10)
      }
    })
  }

all works well.

What I change is just adjust return resolve({}) order.

I googled the error and find this: this

Initialize items to an empty array.

But I still fell confused..

@zixia

zixia approved these changes Jan 24, 2017

@zixia

This comment has been minimized.

Show comment
Hide comment
@zixia

zixia Jan 24, 2017

Member

Great work!

It is not perfect, but I think it mergeable now.

BTW:

  • You will only feel confusing when you know you do not know.
  • You will never feel confusing when you do not know you do not know.
Member

zixia commented Jan 24, 2017

Great work!

It is not perfect, but I think it mergeable now.

BTW:

  • You will only feel confusing when you know you do not know.
  • You will never feel confusing when you do not know you do not know.

@zixia zixia merged commit 87f94d3 into Chatie:master Jan 24, 2017

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
security/snyk No new vulnerabilities
Details

@lijiarui lijiarui deleted the lijiarui:fix_function_room.member_#173 branch Feb 17, 2017

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