-
-
Notifications
You must be signed in to change notification settings - Fork 585
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
Add erase
option to deactivateAccount
#649
Conversation
For erasing messages etc. after deactivation.
src/base-apis.js
Outdated
body = { | ||
auth: auth, | ||
}; | ||
MatrixBaseApis.prototype.deactivateAccount = function(auth, erase, callback) { |
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.
this should never need three parameters? if you want to pass erase, you should use promises instead of callbacks
src/base-apis.js
Outdated
auth: auth, | ||
}; | ||
MatrixBaseApis.prototype.deactivateAccount = function(auth, erase, callback) { | ||
if (typeof(erase) === 'function') { |
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.
elsewhere we have a comment along the lines of "erase used to be callback" or words to that effect
src/base-apis.js
Outdated
callback = erase; | ||
erase = undefined; | ||
} | ||
if (typeof(auth) === 'function') { |
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.
why would auth be a function?
src/base-apis.js
Outdated
} | ||
|
||
const body = {}; | ||
if (typeof(auth) === 'object') { |
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.
what was wrong with if (auth)
?
src/base-apis.js
Outdated
); | ||
} | ||
|
||
const body = { auth, erase }; |
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.
auth
shouldn't be here?
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.
actually, neither should erase... gah my brain is melting over simple changes
src/base-apis.js
Outdated
return this._http.authedRequestWithPrefix( | ||
callback, "POST", '/account/deactivate', undefined, body, httpApi.PREFIX_UNSTABLE, | ||
undefined, "POST", '/account/deactivate', undefined, body, | ||
httpApi.PREFIX_UNSTABLE, |
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.
can you fix this while you are here please
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.
fix what?
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.
PREFIX_UNSTABLE
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.
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've moved it to R0, which I think is correct.
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.
well you could just use authedRequest
and let it use the default prefix (which is indeed R0) but this is better at least
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.
lgtm
For erasing messages etc. after deactivation.