Skip to content
This repository has been archived by the owner on Sep 19, 2018. It is now read-only.

Added touch method to set last_used property #96

Merged
merged 3 commits into from
Mar 14, 2018
Merged

Conversation

sashei
Copy link
Contributor

@sashei sashei commented Mar 14, 2018

resolves #95

@sashei sashei requested a review from linuxwolf as a code owner March 14, 2018 17:09
@ghost ghost assigned sashei Mar 14, 2018
@ghost ghost added the in progress We are actively working on it. label Mar 14, 2018
Copy link
Contributor

@linuxwolf linuxwolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

almost w00t (-:

Inline marks the one change needed.

lib/datastore.js Outdated
};

await self.ldb.items.put(record);
self.recordMetric("touched", item.id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might require an update to the extension's metrics reporting ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment is really a note to go check the extension and ensure it won't cause additional failures.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might require an update to the extension's metrics reporting
this comment is really a note to go check the extension and ensure it won't cause additional failures

@linuxwolf Good call. @sashei is going to do a quick smoke test before merging and have a follow-up here to do corresponding follow-on work: mozilla-lockwise/lockbox-extension#661

lib/datastore.js Outdated
record = {
id,
encrypted,
last_used: record.last_used
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be last_modified not last_used (and the same with record.last_used).

@sashei sashei force-pushed the 95-item-use-tracking branch from 075bb00 to e74669e Compare March 14, 2018 17:53
@sashei sashei force-pushed the 95-item-use-tracking branch from e74669e to fe7e89d Compare March 14, 2018 20:17
Copy link
Contributor

@linuxwolf linuxwolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much closer. Something found, and something that starts as a question.

.eslintrc.json Outdated
@@ -23,6 +23,7 @@
"security/detect-non-literal-fs-filename": "off",
"security/detect-object-injection": "off",

"no-console": "off",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing this was put in place to troubleshoot tests, and just got missed with the last push. This option needs to be removed so we keep the default for "no-console" as "error".

lib/items.js Outdated
@@ -28,6 +28,7 @@ const SCHEMA = joi.object().keys({
origins: joi.array().items(STRING_500).max(5).default([]),
tags: joi.array().items(STRING_500).max(10).default([]),
entry: joi.alternatives(ENTRY_SCHEMAS).required(),
last_used: joi.date().allow(null).default(null),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is what we really want. Adding it to the schema allows the API user to define when last_used was, including removing it altogether (accidentally or otherwise).

Other read-only properties (e.g., modified, created, disabled) are handled as a separate step post Joi validation. That feels like the right behavior for this property, but I'm open to different opinions.

Copy link
Contributor

@linuxwolf linuxwolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+

¡Perfecto!

@sashei sashei merged commit 5dd3b60 into master Mar 14, 2018
@ghost ghost removed the in progress We are actively working on it. label Mar 14, 2018
@sashei sashei deleted the 95-item-use-tracking branch March 14, 2018 20:58
linuxwolf pushed a commit that referenced this pull request May 21, 2018
* Added `touch` method to set last_used property without altering `modified
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support item use tracking via timestamps
3 participants