You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by vladimir-kotikov <gi...@git.apache.org> on 2015/12/01 14:07:14 UTC
[GitHub] cordova-plugin-contacts pull request: CB-8115 Save contact birthda...
GitHub user vladimir-kotikov opened a pull request:
https://github.com/apache/cordova-plugin-contacts/pull/95
CB-8115 Save contact birthday properly
This is a fix for [CB-8115](https://issues.apache.org/jira/browse/CB-8115)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/MSOpenTech/cordova-plugin-contacts CB-8115
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-plugin-contacts/pull/95.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #95
----
commit 9321c0d66106b6e9ffe22e62039a040d02c58297
Author: Vladimir Kotikov <v-...@microsoft.com>
Date: 2015-11-30T15:20:26Z
CB-8115 Save contact birthday properly
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-contacts pull request: CB-8115 Save contact birthda...
Posted by riknoll <gi...@git.apache.org>.
Github user riknoll commented on the pull request:
https://github.com/apache/cordova-plugin-contacts/pull/95#issuecomment-161745285
@vladimir-kotikov makes sense to me! Thanks for the clarification!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-contacts pull request: CB-8115 Save contact birthda...
Posted by jasongin <gi...@git.apache.org>.
Github user jasongin commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-contacts/pull/95#discussion_r46598813
--- Diff: src/android/ContactAccessorSdk5.java ---
@@ -1854,6 +1859,39 @@ public boolean remove(String id) {
return (result > 0) ? true : false;
}
+ /**
+ * Gets birthday date from contact JSON object
+ * @param contact an object to get birthday from
+ * @return birthday or null, if the field isn't present or
+ * is malformed in the contact
+ */
+ private Date getBirthday(JSONObject contact) {
+ try {
+ Long timestamp = contact.getLong("birthday");
+ return new Date(timestamp);
+ } catch (JSONException e) {
+ Log.d(LOG_TAG, "Could not get birthday: " + e.getMessage());
+ return null;
+ }
+ }
+
+ /**
+ * Gets birthday from contacts database cursor object
+ * @param c cursor for the contact
+ * @return birthday or null, if birthday column is empty or
+ * the value can't be parsed into valid date object
+ */
+ private Date getBirthday(Cursor c) {
+ int colBirthday = c.getColumnIndex(CommonDataKinds.Event.START_DATE);
+
+ try {
+ return Date.valueOf(c.getString(colBirthday));
+ } catch (IllegalArgumentException e) {
+ Log.d(LOG_TAG, "Failed to get birthday for contact: " + e.getMessage());
--- End diff --
As explained previously: Log.e(LOG_TAG, "Failed... ", e);
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-contacts pull request: CB-8115 Save contact birthda...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-plugin-contacts/pull/95
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-contacts pull request: CB-8115 Save contact birthda...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-plugin-contacts/pull/95#issuecomment-161271526
I think this is not possible to cover this issue with our automated tests, because, strictly speaking, the plugin code stores and reads birthdays correctly - Android just requires that the date should be stored as a string in [ISO format](http://dotat.at/tmp/ISO_8601-2004_E.pdf) instead of timestamp.
IMO the only way to test this, is to manually check, if birthday is displayed correctly in device's phonebook.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-contacts pull request: CB-8115 Save contact birthda...
Posted by dblotsky <gi...@git.apache.org>.
Github user dblotsky commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-contacts/pull/95#discussion_r46360939
--- Diff: src/android/ContactAccessorSdk5.java ---
@@ -451,7 +452,13 @@ else if (mimetype.equals(CommonDataKinds.Website.CONTENT_ITEM_TYPE)
else if (mimetype.equals(CommonDataKinds.Event.CONTENT_ITEM_TYPE)) {
if (isRequired("birthday", populate) &&
CommonDataKinds.Event.TYPE_BIRTHDAY == c.getInt(colEventType)) {
- contact.put("birthday", c.getString(colBirthday));
+
+ try {
+ long timestamp = Date.valueOf(c.getString(colBirthday)).getTime();
+ contact.put("birthday", timestamp);
+ } catch (IllegalArgumentException e) {
+ Log.d(LOG_TAG, "Failed to get birthday for contact: " + e.getMessage());
+ }
--- End diff --
This code is duplicated below. Please place it in a function. Also, if there is an `IllegalArgumentException`, a birthday won't be set at all; is this what we define as expected behaviour for this plugin's API?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-contacts pull request: CB-8115 Save contact birthda...
Posted by riknoll <gi...@git.apache.org>.
Github user riknoll commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-contacts/pull/95#discussion_r46483328
--- Diff: src/android/ContactAccessorSdk5.java ---
@@ -1854,6 +1859,38 @@ public boolean remove(String id) {
return (result > 0) ? true : false;
}
+ /**
+ * Gets birthday date from contact JSON object
+ * @param contact an object to get birthday from
+ * @return birthday or null, it the field doesn't present in contact or malformed
--- End diff --
Grammar nitpick: "If the field isn't present or is malformed in the contact"
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-contacts pull request: CB-8115 Save contact birthda...
Posted by dblotsky <gi...@git.apache.org>.
Github user dblotsky commented on the pull request:
https://github.com/apache/cordova-plugin-contacts/pull/95#issuecomment-161140513
Please note that the tests do test for the bug in CB-8115, and _don't catch it_. Here is an example: https://github.com/MSOpenTech/cordova-plugin-contacts/blob/CB-8115/tests/tests.js#L359. Please also fix all occurrences of that incorrect check. It might help to write a helper function.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-contacts pull request: CB-8115 Save contact birthda...
Posted by dblotsky <gi...@git.apache.org>.
Github user dblotsky commented on the pull request:
https://github.com/apache/cordova-plugin-contacts/pull/95#issuecomment-161391018
Is it possible to check by loading the contact and comparing the date in a human-readable format with a fixed string like `"16-May-1984"`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-contacts pull request: CB-8115 Save contact birthda...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-plugin-contacts/pull/95#issuecomment-161548065
@dblotsky, as @purplecabbage mentioned, there is no way to cover this by autotests, because from JS perspective everything works well - we convert a date to timestamp and send it to native, then we get _exactly the same_ timestamp from native (because there was no conversion made on native side).
BTW, this might be a perfect case to cover with UI autotest, @alsorokin working on.
@riknoll, The issue is in format which the date should be stored in contacts database. The android docs doesn't specify this format (except from the fact that [it should be a 'TEXT'](http://developer.android.com/reference/android/provider/ContactsContract.CommonDataKinds.Event.html#START_DATE)), but the birthday in contact card is _displayed correctly_ only if specified as `dd-mm-YYYY` (from my experience).
There is no inconsistence in Contact objects structure on JS side - the `birthday` field is always a valid `Date` object or `null`, despite of current platform.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-contacts pull request: CB-8115 Save contact birthda...
Posted by jasongin <gi...@git.apache.org>.
Github user jasongin commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-contacts/pull/95#discussion_r46598691
--- Diff: src/android/ContactAccessorSdk5.java ---
@@ -1854,6 +1859,39 @@ public boolean remove(String id) {
return (result > 0) ? true : false;
}
+ /**
+ * Gets birthday date from contact JSON object
+ * @param contact an object to get birthday from
+ * @return birthday or null, if the field isn't present or
+ * is malformed in the contact
+ */
+ private Date getBirthday(JSONObject contact) {
+ try {
+ Long timestamp = contact.getLong("birthday");
+ return new Date(timestamp);
+ } catch (JSONException e) {
+ Log.d(LOG_TAG, "Could not get birthday: " + e.getMessage());
--- End diff --
As a general good practice, instead of appending the exception message to the error string, pass the exception object to the log method overload that takes a throwable as the third parameter. The exception message and stack trace will then be printed to logcat.
Also, it would be more appropriate to use Log.e() instead of Log.d() for this kind of unexpected error.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
Re: [GitHub] cordova-plugin-contacts pull request: CB-8115 Save
contact birthda...
Posted by Jesse <pu...@gmail.com>.
The issue is that the date object is not stored in a method that is
compatible with other applications on the android device.
This is strictly a native issue, and as Vlad said, not testable via our own
automated tests.
We get back exactly what we store, so our current tests all pass. The
native code just needs to store/retrieve it in a format compatible with
other apps. This should have zero impact on other platforms and zero
impact on the js code.
@purplecabbage
risingj.com
On Wed, Dec 2, 2015 at 2:14 PM, riknoll <gi...@git.apache.org> wrote:
> Github user riknoll commented on the pull request:
>
>
> https://github.com/apache/cordova-plugin-contacts/pull/95#issuecomment-161450864
>
> @vladimir-kotikov is the issue that Android saves the date in a
> different format from other platforms? If that is the case, I guess the
> greater issue is lack of consistency in the API as to how contacts are
> presented in the JS. That sounds like a separate issue to me, but one that
> should be addressed.
>
>
> ---
> If your project is set up for it, you can reply to this email and have your
> reply appear on GitHub as well. If your project does not have this feature
> enabled and wishes so, or if the feature is enabled but not working, please
> contact infrastructure at infrastructure@apache.org or file a JIRA ticket
> with INFRA.
> ---
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
> For additional commands, e-mail: dev-help@cordova.apache.org
>
>
[GitHub] cordova-plugin-contacts pull request: CB-8115 Save contact birthda...
Posted by riknoll <gi...@git.apache.org>.
Github user riknoll commented on the pull request:
https://github.com/apache/cordova-plugin-contacts/pull/95#issuecomment-161450864
@vladimir-kotikov is the issue that Android saves the date in a different format from other platforms? If that is the case, I guess the greater issue is lack of consistency in the API as to how contacts are presented in the JS. That sounds like a separate issue to me, but one that should be addressed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-contacts pull request: CB-8115 Save contact birthda...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-plugin-contacts/pull/95#discussion_r46402705
--- Diff: src/android/ContactAccessorSdk5.java ---
@@ -451,7 +452,13 @@ else if (mimetype.equals(CommonDataKinds.Website.CONTENT_ITEM_TYPE)
else if (mimetype.equals(CommonDataKinds.Event.CONTENT_ITEM_TYPE)) {
if (isRequired("birthday", populate) &&
CommonDataKinds.Event.TYPE_BIRTHDAY == c.getInt(colEventType)) {
- contact.put("birthday", c.getString(colBirthday));
+
+ try {
+ long timestamp = Date.valueOf(c.getString(colBirthday)).getTime();
+ contact.put("birthday", timestamp);
+ } catch (IllegalArgumentException e) {
+ Log.d(LOG_TAG, "Failed to get birthday for contact: " + e.getMessage());
+ }
--- End diff --
> Also, if there is an IllegalArgumentException, a birthday won't be set at all; is this what we define as expected behaviour for this plugin's API?
I don't see any other appropriate options here. If there is an exception, this means that the date is probably malformed, and we can't convert it into a valid timestamp, which JS side expects to get.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org