-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
support brand checking of contact #404
Conversation
JasLin
commented
Apr 12, 2017
upgrade to latest code
…m, 2. some times we can got owneruin or ChatRoomOwner . as those reason, first member from memberlist should not be here any more
merge base
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work bro, It seems like a really tricky magic number inside.
LGTM and please get at least 3 approvements from our contributors before merge for enhancing our community communication.
Thanks!
tslint.json
Outdated
@@ -36,7 36,7 @@ | |||
"member-ordering": [false], | |||
"no-any": false, | |||
"no-arg": true, | |||
"no-bitwise": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep this because the bitwise operation is very rare in our code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, u r right:)
src/contact.ts
Outdated
* @see 1. https://github.com/nodeWechat/wechat4u/blob/master/src/interface/contact.js#L65 | ||
* @see 2. https://github.com/Urinx/WeixinBot/blob/master/README.md | ||
*/ | ||
brand: !!rawObj.UserName && !rawObj.UserName.startsWith('@@') && (rawObj.VerifyFlag & 8) !== 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to use this line to disdable tslint temporary
// tslint:disable-next-line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// tslint:disable-next-line
will disables all rules for the following lines, it seems it's not a good choose for this case.
how about this?
!!rawObj.UserName && !rawObj.UserName.startsWith('@@') && [8, 24, 56].indexOf(rawObj.VerifyFlag) > -1,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that will only disable the following ONE line?
Could you please have a check about it, because the new version of your code is difficult to read...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, ...next-line... u r right:)
src/contact.ts
Outdated
* ``` | ||
*/ | ||
public brand(): boolean|null { | ||
if (!this.obj) return null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our framework should always to make sure a contact
is ready
, which means this.obj
should not be empty.
If so, it's more like a bug and should throw an exception.
So my suggestion is to change this function to:
public brand(): boolean {
return this.obj && this.obj.brand
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public brand(): boolean {
return !!this.obj && this.obj.brand
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes your code is correct. :)
example/contact-bot.ts
Outdated
*/ | ||
for (let i = 0; i < contactList.length; i ) { | ||
const contact = contactList[i] | ||
if (contact.brand() === null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The contact
here should already ready()
-ed, because Wechaty should make sure it is.
If it is not, then Wechaty must have a bug in somewhere...
…se to false. 2. contact is ready always
1 similar comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool!
Btw, could you help to review the add mention
and room leave event
feature for me ? Thanks!
@lijiarui thanks , i am trying to review them |
@dcsan I guess maybe you need this feature most, could you help to review this please? Thanks |
@lijiarui I'm not sure what this feature does?
what is a |
Tencent has this title at https://admin.wechat.com/:
|
ok mingbai! so this is in the case where chatie bot is following an official account. so we have:
then service accounts, subscription accounts, personal OAs - not sure if the wechaty client gets any information about the type of brand/ official account, or if it matters.
then we could have a
as a string? If we use enums btw I think we need to make these more easily exportable/importable to work with JS. Now we have |
@dcsan IMO stay with TypeScript is the bright decision, I love it to death now. :P And for this Now we can get all contact after login by Add a if (contact.official()) {
// is Official Account
} else {
// is Personal Account
} Or let contactList = await Contact.findAll()
let officialAccountList = contactList.filter(c => c.official())
let personalAccountList = contactList.filter(c => !c.officlal()) |
I think it's also useful to know if its a group(room) or an individual user, as some of the behavior is different, eg I don't think there is a I think i have stuff like:
So when you have three major possible types, a boolean check method isn't as useful? Personally I like Typescript a lot, but it is hard just getting new developers up and running, especially we have some team in Russia and other places. typescript, tslint, wechat api, docker... hours of setup to even get to helloworld. |
Yes, I agree with you that TypeScript has a huge study curve. It seems things getting better after TypeScript v2.3 because they became treating unknown things to be And I do not fully understand you, as in the reply you said:
To clarify, In Wechaty:
For your code:
I'd like to rewrite it to: const room = msg.room()
let topic: string
if (room) {
// this message is received in a room
assert(room instanceof Room)
topic = room.topic()
} else {
// this message is received from a indivial user
// WHY do you set room topic
// when you are receiving a direct message
// from an individual user?
topic = 'dm' // ???
} |
hi - thanks for the above.
yeah, it's a bit hacky agreed. it is so we can render all messages and handle group|dm display based on a single (cached) property rather than another chain of method calls.
If we had switch (msg.type() ):
case 'room':
// responding in a group
case 'dm':
//..
case 'oa':
// brand / official account Just a thought. This maybe still a hacky way to do this, as they are still different types of objects. Perhaps we'd be better with something like the re TypeScript, I think the language is easy. |
@JasLin Hi bro, I suggest:
Because we could use this method to identify whether the contact is an individual personal user, future design/changes could be discussed later. How do you think? |
@zixia sorry check this so later, I will try to finish this tomorrow. And, happy birthday bro!:) |
@zixia Hi bro, I just check the wxwebapp's source and the rawObj again . the following info is confirmed
and two more thing :
|
I think we could translate them into bit position:
How about let's redesign enum OfficialType {
OFFICIAL,
BUSINESS,
TENCENT,
}
official(type = OfficialType.OFFICIAL): boolean By default,
Update: I saw you have already implemented the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use https://github.com/Chatie/webwx-app-tracker to reference web wechat source.
Thanks!
src/contact.ts
Outdated
@@ -61,6 64,16 @@ export type ContactQueryFilter = { | |||
} | |||
|
|||
/** | |||
* @see https://github.com/Chatie/wechaty/blob/master/doc/webwxapp.js#L4057 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not use wechaty/doc
, because this directory is deprecated.
Should change refer to https://github.com/Chatie/webwx-app-tracker with the exactly commit hash.
src/contact.ts
Outdated
@@ -110,6 123,16 @@ export class Contact implements Sayable { | |||
star: !!rawObj.StarFriend, | |||
stranger: !!rawObj.stranger, // assign by injectio.js | |||
avatar: rawObj.HeadImgUrl, | |||
/** | |||
* @see 1. https://github.com/Chatie/wechaty/blob/master/doc/webwxapp.js#L3368 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change refer to https://github.com/Chatie/webwx-app-tracker with the exactly commit hash.
src/contact.ts
Outdated
// tslint:disable-next-line | ||
official: !!rawObj.UserName && !rawObj.UserName.startsWith('@@') && !!(rawObj.VerifyFlag & 8), | ||
/** | ||
* @see 1. https://github.com/Chatie/wechaty/blob/master/doc/webwxapp.js#L4187 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change refer to https://github.com/Chatie/webwx-app-tracker with the exactly commit hash.
src/contact.ts
Outdated
* 'masssendapp', 'meishiapp', 'feedsapp', 'voip', 'blogappweixin', 'weixin', 'brandsessionholder', | ||
* 'weixinreminder', 'wxid_novlwrv3lqwv11', 'gh_22b87fa7cb3c', 'officialaccounts', 'notification_messages', | ||
* ``` | ||
* @see https://github.com/Chatie/wechaty/blob/master/doc/webwxapp.js#L4057 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change refer to https://github.com/Chatie/webwx-app-tracker with the exactly commit hash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
had changed it with the latest commit
here are all flags defined in source code: CONTACTFLAG_CONTACT: 1,
CONTACTFLAG_CHATCONTACT: 2,
CONTACTFLAG_CHATROOMCONTACT: 4,
CONTACTFLAG_BLACKLISTCONTACT: 8,
CONTACTFLAG_DOMAINCONTACT: 16,
CONTACTFLAG_HIDECONTACT: 32,
CONTACTFLAG_FAVOURCONTACT: 64,
CONTACTFLAG_3RDAPPCONTACT: 128,
CONTACTFLAG_SNSBLACKLISTCONTACT: 256,
CONTACTFLAG_NOTIFYCLOSECONTACT: 512,
CONTACTFLAG_TOPCONTACT: 2048,
... ...
MM_USERATTRVERIFYFALG_BIZ: 1
, MM_USERATTRVERIFYFALG_FAMOUS: 2
, MM_USERATTRVERIFYFALG_BIZ_BIG: 4
, MM_USERATTRVERIFYFALG_BIZ_BRAND: 8
, MM_USERATTRVERIFYFALG_BIZ_VERIFIED: 16 so I suggest we design
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @JasLin ,
I hope you have a good weekend!
Your new code looks good for me, and I agree with you about the magic number and method name opinion.
Please get 3 approving from our @Chatie/contributor before I could merge it.
Thanks!
it seems we need more contributors ... |
Yes, I definitely agree with you bro... @_@ |