Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Cache aspect writers #341

Merged
merged 5 commits into from
May 2, 2017
Merged

Cache aspect writers #341

merged 5 commits into from
May 2, 2017

Conversation

shriramshankar
Copy link
Contributor

No description provided.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 89.566% when pulling 5e55802 on cache-aspect-writers into 80de52c on master.

@shriramshankar
Copy link
Contributor Author

There are 4 parts to this PR and each of the part is in its own commit.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 89.493% when pulling 673a0ed on cache-aspect-writers into 80de52c on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 89.559% when pulling 81d8244 on cache-aspect-writers into 80de52c on master.

pallavi2209
pallavi2209 previously requested changes Apr 20, 2017
View reviewed changes
Copy link
Contributor

@pallavi2209 pallavi2209 left a comment

Choose a reason for hiding this comment

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

In tests, when we check for errors, do you think it would be better to check for the error messages which is returned in responses? Previously, I have found some errors to return right status but unexpected/no response message. So, I thought I will just mention, fyi.

cache/sampleStore.js
constants.ISOfields.forEach((field) => {
if (!obj[field]) {
obj[field] = new Date().toISOString();
} else if (obj[field] && obj[field].toISOString) {
Copy link
Contributor

@pallavi2209 pallavi2209 Apr 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

obj[field].toISOString() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, I just checked obj[field].toISOString will return undefined for something other than Date.

api/v1/controllers/aspects.js Outdated
.catch((err) => Promise.resolve(err));
}

return Promise.resolve(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Some comments would be a plus!

cache/redisOps.js
* @param {String} name - Name of the key
* @returns {Promise} - which resolves to the value associated with the key
*/
function getValue(type, name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is already a function getHashPromise with same intent. This method converts arrayStringsToJson as well. Any way we can avoid code duplication? If you insist on keeping the function, we should atleast change the name and comments to reflect how this function is different from other.

api/v1/controllers/aspects.js
.catch((err) => u.handleError(next, err, helper.modelName));
} else {
doDeleteAllAssoc(req, res, next, helper, helper.belongsToManyAssoc.users);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Some comments if you can.

api/v1/controllers/aspects.js
.then(() => doDeleteOneAssoc(req, res, next, helper,
helper.belongsToManyAssoc.users, userNameOrId))
.catch((err) => u.handleError(next, err, helper.modelName));
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Inline Comments or update function comment please!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What comments do you need ??

Copy link
Contributor

Choose a reason for hiding this comment

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

I just thought its better to add comments whenever we are adding a chunk of code, on why/what new code is. But you can ignore this if you feel this is unnecessary.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 89.565% when pulling e96711c on cache-aspect-writers into 15cd869 on master.

annyhe
annyhe reviewed Apr 21, 2017
View reviewed changes
tests/cache/sampleStore.js
expect(aspect.writers).to.have
.members([user4.name]);
})
.then(() => samstoinit.init())
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the reason for initializing the sample store at the end of the test?

annyhe
annyhe reviewed Apr 21, 2017
View reviewed changes
tests/cache/models/aspects/deleteWriters.js

return done();
});
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason for returning null? do we need a return here at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@annyhe Will correct it, we do not need it.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 89.565% when pulling cef0b79 on cache-aspect-writers into f6bcdf6 on master.

@pallavi2209
Copy link
Contributor

@shriramshankar Just to confirm, my understanding is that I am not required to review this PR.

@shriramshankar shriramshankar mentioned this pull request May 1, 2017
Merged
@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 89.202% when pulling 79726f9 on cache-aspect-writers into 10e100d on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 89.096% when pulling 79726f9 on cache-aspect-writers into 10e100d on master.

@shriramshankar
Copy link
Contributor Author

I have resolved the conflicts, added a few more tests and some comments. Can we merge this, please?

@iamigo
Copy link
Contributor

iamigo commented May 2, 2017

ok by me, need's @pallavi2209 to approve the changes since she requested changes

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 89.158% when pulling 207c2cc on cache-aspect-writers into fb16ff8 on master.

@shriramshankar
Copy link
Contributor Author

@iamigo I added the third reviewer by mistake and there is nothing I can do to remove the third reviewer. If I start working on my bulkUpsert story without this in the master, I will have to resolve the conflict again. Can we please merge this?

@iamigo iamigo merged commit 1b8cd36 into master May 2, 2017
@iamigo iamigo deleted the cache-aspect-writers branch May 2, 2017 20:58
tausifmuzaffar added a commit that referenced this pull request May 3, 2017
@tausifmuzaffar
Merge branch 'roomswagger' into WarRoomAPI
03d01cf 
* roomswagger:
  Feature flag for swagger for rooms testing
  Cache aspect writers (#341)
  Add kue stats logging (#348)
  changes to eradicate keys prefixed with smsto:subaspmap (#349)
  Connection pooling (#347)
  Sample create fix (#346)
@meghasfdc meghasfdc mentioned this pull request Feb 15, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pallavi2209 pallavi2209 pallavi2209 left review comments

@iamigo iamigo iamigo approved these changes

@annyhe annyhe annyhe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@shriramshankar @coveralls @pallavi2209 @iamigo @annyhe