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.

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

See host even if only editor buckets exist #186

Merged
merged 2 commits into from Apr 21, 2020

Conversation

johan-bjareholt
Copy link
Member

Fixes #184

// getters
const getters = {
afkBuckets(state) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@@ -25,6 +25,20 @@ function timeperiodToStr(tp: TimePeriod): string {
return [start, end].join('/');
}

function get_buckets_by_type(buckets, type) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice, but duplicated code (also in modules/buckets.js).

Move all of it there?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's the plan, hence the comment below

// TODO: re-use modules/buckets.js instead of doing a second getBuckets call

Copy link
Member

Choose a reason for hiding this comment

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

Ah, could still have done an import for this particular helper function in the meantime, but whatever if you're going to fix it before merging.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Apr 19, 2020

This pull request introduces 2 alerts when merging 31aec5f into 3451196 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@@ -21,7 +21,7 @@ div
| Next
icon(name="arrow-right")
div.p-1
b-select(:value="periodLength", :options="['day', 'week', 'month']",
b-select(:value="periodLength", :options="['day', 'week', 'month', 'year']",
Copy link
Member

Choose a reason for hiding this comment

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

Oooof, going to get some HTTP timeouts until aw-server-rust becomes the default.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, commited by mistake.

@@ -103,7 +103,7 @@ export default {
//console.log(types_by_host);

_.each(types_by_host, (types, hostname) => {
if (types.afk && types.window) {
if (hostname != 'unknown') {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this makes the _.each over types unnecessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure due to the android related code, don't want to break it by mistake.

Copy link
Member

Choose a reason for hiding this comment

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

The Android view is getting merged with the normal view in #183 anyway, so feel free to remove that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it'll be easier if you fix it in that change instead.

@ErikBjare ErikBjare added this to In progress in Road to 1.0 Apr 19, 2020
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Apr 19, 2020

This pull request introduces 2 alerts when merging 0402013 into 3451196 - view on LGTM.com

new alerts:

  • 2 for Unused variable, import, function or class

@johan-bjareholt johan-bjareholt force-pushed the dev/make-queries-less-dependent branch 2 times, most recently from 82c9817 to 1dfa250 Compare April 19, 2020 17:28
@johan-bjareholt johan-bjareholt changed the title WIP: See host even if only editor buckets exist See host even if only editor buckets exist Apr 19, 2020
@johan-bjareholt
Copy link
Member Author

@ErikBjare Seems to work fine now and fixed some minor oddities, do you have any more comments or should I merge?

commit('query_window_completed', data[0]);
async query_window({ state, commit }, { host, timeperiod, filterAFK, filterCategories }: QueryOptions) {
if (state.buckets.afk_buckets.length > 0 &&
state.buckets.window_buckets.length > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Not really a fan of this check being here. Bucket-requirements should imo be checked in ensure_loaded and the start_loading thing should be called in the individual queries instead of one-fits-all.

Not a blocker for this PR, but would be nice if you could add that as a TODO comment in the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really a fan of this check being here. Bucket-requirements should imo be checked in ensure_loaded

Done

the start_loading thing should be called in the individual queries instead of one-fits-all.

Not sure what you mean by this?

Copy link
Member

Choose a reason for hiding this comment

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

Meant exactly what you changed it to, nice!

Road to 1.0 automation moved this from In progress to Reviewer approved Apr 20, 2020
@johan-bjareholt johan-bjareholt merged commit 6379134 into master Apr 21, 2020
Road to 1.0 automation moved this from Reviewer approved to Done Apr 21, 2020
@johan-bjareholt johan-bjareholt deleted the dev/make-queries-less-dependent branch April 21, 2020 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Road to 1.0
  
Done
Development

Successfully merging this pull request may close these issues.

Why do I need an AFK and window watcher to get an activity report
2 participants