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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

added incidents menu and notifications #14

Merged
merged 19 commits into from Mar 18, 2019

Conversation

Projects
None yet
3 participants
@adamwatters
Copy link
Contributor

commented Feb 21, 2019

Needs tests, and I'm really not sure if I'm doing TS correctly 馃檲 (actually quite sure I'm not lol), but creating a PR so you can take a look before I get lost tinkering.

committed with the --no-verify because the same test that were failing before and after my changes
image

Let me know what you think 馃槃

@stefanjudis

This comment has been minimized.

Copy link
Owner

commented Feb 21, 2019

Woooohooooo! It works! Thanks so much for this. 馃帀

I only have a few moments now (I'm onboarding a new job) but I'll leave some comments.

and I'm really not sure if I'm doing TS correctly 馃檲 (actually quite sure I'm not lol)

No worries. I know starting TS is rough. I'll leave some comments. :)

because the same test that were failing before and after my changes

I fixed the snapshot tests in master. You can update the PR.

@stefanjudis
Copy link
Owner

left a comment

This is super cool!

I did a high-level check and left some comments. Overall I feel a little bit like the feed class does a little too much. I would extract the time and notification handling. This way the center of the app (menubar) would decide when to fetch the feed and when to notify.

a getFeed function could flag new incidents with a boolean so that the check for notifications could be very straightforward.

What do you think? Thanks again! 馃槉

shell.openExternal(link);
});

// notifications with an attached click handler

This comment has been minimized.

Copy link
@stefanjudis

stefanjudis Feb 21, 2019

Owner

Maybe we should extract this functionality and wrap notification in its own file separately. I feel like this handling would be easier to grasp somewhere else?

This comment has been minimized.

Copy link
@adamwatters

adamwatters Feb 21, 2019

Author Contributor

yeah - I was thinking it would be good to have a notifications.js, but I decided to hold off on any refactoring in menubar.js. I'll see what I can do 馃檶

This comment has been minimized.

Copy link
@stefanjudis

stefanjudis Feb 21, 2019

Owner

That's fair. :) As you prefer we could also leave it as is and create another issue for the notification handling. I have no strong feelings here. :)

What I would like to include are tests, though. And the feed class is way harder to test when it includes notification handling (and the repeating). :)

This comment has been minimized.

Copy link
@adamwatters

adamwatters Feb 23, 2019

Author Contributor

Notifications refactored into notify.js, and tested. Mocking out electron kicked my butt - I ended up doing something related but different from whats described in electron/electron#3909 https://www.jamestease.co.uk/blether/mock-es6-imports-with-jest. Let me know if know of a more straight forward way 馃檹

notify.ts

notify.test.ts

This comment has been minimized.

Copy link
@stefanjudis

stefanjudis Feb 23, 2019

Owner

Did you try this approach? I struggled initially, too, but found this okay'ish.

This comment has been minimized.

Copy link
@adamwatters

adamwatters Feb 23, 2019

Author Contributor

Hmmm - nope, I missed that! I just looked for imports from 鈥渆lectron鈥 in the existing files and when I didn鈥檛 see any I figured it wasn鈥檛 being mocked anywhere.

does that jest.mock(鈥榚lectron鈥) replace the electron module imported in netlify.ts?

This comment has been minimized.

Copy link
@stefanjudis

stefanjudis Feb 23, 2019

Owner

does that jest.mock(鈥榚lectron鈥) replace the electron module imported in netlify.ts?

Yes. This way you can control what will be required. DO you think that would work for you?

const publicationDate = new Date(item.pubDate);
return isToday(publicationDate) || isYesterday(publicationDate);
});
recentIncidents.forEach(item => {

This comment has been minimized.

Copy link
@stefanjudis

stefanjudis Feb 21, 2019

Owner

This would start a wave of notification, wouldn't it? I think the last one would be enough. What do you think?

This comment has been minimized.

Copy link
@adamwatters

adamwatters Feb 21, 2019

Author Contributor

yep - I think one notification is better 馃憤

This comment has been minimized.

Copy link
@adamwatters

adamwatters Feb 23, 2019

Author Contributor

just showing the one now

if (recentIncidents.length) {
notify(
{
body: recentIncidents[0].title,
title: 'Recently reported incident'
},
() => {
shell.openExternal(recentIncidents[0].link);
}
);
}


// main update loop for checking Netlify Incidents RSS
const repeat = () => {
setTimeout(

This comment has been minimized.

Copy link
@stefanjudis

stefanjudis Feb 21, 2019

Owner

In the long run, I was planning to have a separated scheduler for everything. Do you think it would be alright to have all the "repeat" handling in the same place with a comment like "soon there will be a scheduler".

I'm dreaming of something like

scheduler.repeat( () => {
  feed.getFeed()
}, 5000)

this would take care of online/offline, too.

Could IncidentFeed expose one function that is called around here?

In my mind, it feels a little bit that it could be

repeat( () => {
  const incidents = feed.getNewItems()

  if (incidents.filter(isNew)) // notify
})

This way the feed class would deal with the fetching and figuring out what's new and notifications and scheduling happen outside.

This comment has been minimized.

Copy link
@adamwatters

adamwatters Feb 21, 2019

Author Contributor

Love the scheduler idea 馃憤Will move the repeat loop to menubar in the meantime.

@adamwatters

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

Also...

(I'm onboarding a new job)

Congrats!

@codecov

This comment has been minimized.

Copy link

commented Feb 21, 2019

Codecov Report

Merging #14 into master will increase coverage by 3.3%.
The diff coverage is 44.3%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #14     +/-   ##
=========================================
+ Coverage   33.96%   37.27%   +3.3%     
=========================================
  Files           8       10      +2     
  Lines         262      330     +68     
  Branches       34       44     +10     
=========================================
+ Hits           89      123     +34     
- Misses        171      205     +34     
  Partials        2        2
Impacted Files Coverage 螖
src/menubar.ts 0% <0%> (酶) 猬嗭笍
src/index.ts 0% <0%> (酶) 猬嗭笍
src/incidentFeed.ts 100% <100%> (酶)
src/notify.ts 100% <100%> (酶)
src/util.ts 88.57% <25%> (-8.21%) 猬囷笍
src/menus.ts 48.48% <26.66%> (-19.94%) 猬囷笍

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 2544628...b5b123b. Read the comment docs.

}));
jest.doMock('electron-settings', () => ({
get: mockGetSettings
}));

This comment has been minimized.

Copy link
@adamwatters

adamwatters Feb 24, 2019

Author Contributor

What do you think of this approach? This test no longer uses a special mocked out 'electron' module, which I think makes it a lot more straight forward. This case is a little different from netlify.test.ts because the tests here need to keep a handle on the mock functions to make assertions about whether they were called.

Seems reasonable to me, but I haven't written all that many tests, so let me know if you think I'm doing something naive/foolish/crazy here 馃槅

This comment has been minimized.

Copy link
@stefanjudis

stefanjudis Mar 4, 2019

Owner

Looks reasonable! 馃憣

@adamwatters
Copy link
Contributor Author

left a comment

moved notifications and scheduling from incidentFeed.ts to menubar.ts and wrote a test for incidentFeed

@adamwatters
Copy link
Contributor Author

left a comment

I think this covers all requested changes - let me know if I missed anything!

this.setup().then(() => {
// Scheduler should have some method like: doOnce()
let first = true;

This comment has been minimized.

Copy link
@adamwatters

adamwatters Feb 24, 2019

Author Contributor

I don't feel awesome about adding this flag to the setup function, but I didn't want to pollute the menubar state object. I'm imagining the scheduler can have a "doOnce()" method for handling this sort of thing.

});
test('fourth update', async () => {
await incidentFeed.update();
expect(incidentFeed.getFeed()).toBe(fourthResponse.items);

This comment has been minimized.

Copy link
@adamwatters

adamwatters Feb 24, 2019

Author Contributor

You were right - stripping back incidentFeed made it much easier to write a test 馃憤

@stefanjudis

This comment has been minimized.

Copy link
Owner

commented Feb 25, 2019

Hey @adamwatters, unfortunately, I won't make it to look at this before Sunday (I flew over to SF yesterday and have a packed schedule), but I'll have a detailed look then. Let's get this out! 馃帀

@adamwatters

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

@stefanjudis no sweat - good luck on your first week!

@stefanjudis
Copy link
Owner

left a comment

Nice 鈥 thanks so much for the additional work! 馃帀 That's great!

I went over it a little bit more in details and left some comments. :) Please feel free to push back for nits. :)

On a trickier note, i checked out the branch and running npm run dev results in a bunch of typescript errors. Is it working for you? Can I help somehow?

Edited: something is odd, because travis is failing with a weird dep error, too.

.mockReturnValueOnce(secondResponse)
.mockReturnValueOnce(thirdResponse)
.mockReturnValueOnce(fourthResponse)
};

This comment has been minimized.

Copy link
@stefanjudis

stefanjudis Mar 4, 2019

Owner

Nit: could this maybe be done a bit more elegantly? I'm thinking of a reduce.

responses = [
  { items: [ ... ] },
  { items: [ ... ] },
  ...
]

{
  parseURL: responses.reduce(
    (acc, response) => acc.mockReturnValueOnce(response),
    jest.fn()
  )
}
}
);

import IncidentFeed from './incidentFeed';

This comment has been minimized.

Copy link
@stefanjudis

stefanjudis Mar 4, 2019

Owner

Nitpick: Can we move imports to the top? 馃槉

describe('IncidentFeed', () => {
test(':getFeed returns current feed without fetching and parsing an update', () => {
expect(incidentFeed.getFeed()).toMatchObject([]);
expect(incidentFeed.getFeed()).not.toMatchObject(['somee value']);

This comment has been minimized.

Copy link
@stefanjudis

stefanjudis Mar 4, 2019

Owner

I'm not sure, I get this check... :/ Can you explain?

this.currentFeed = fetchedFeed;
}

public newIncidents() {

This comment has been minimized.

Copy link
@stefanjudis

stefanjudis Mar 4, 2019

Owner

Can we add types to this and the following functions?

@@ -360,6 +409,11 @@ export default class UI extends EventEmitter {
]
},
{ type: 'separator' },
{

This comment has been minimized.

Copy link
@stefanjudis

stefanjudis Mar 4, 2019

Owner

I have no strong feelings here, but I envisioned this between version number and email. :) What do you think?

const recentIncidents = incidentFeed
.getFeed()
// remove incidents in distant past
.filter(incident => {

This comment has been minimized.

Copy link
@stefanjudis

stefanjudis Mar 4, 2019

Owner

Nit: You could put this function into a variable outside of the menu and then we could spare the comment. :)

const isNotOlderThanAMonth = (incident) => { /* all the logic */ }

const recentIncidents = incidentFeed
  .getFeed()
  .filter(isNotOlderThanAMonth)
  ...
}));
jest.doMock('electron-settings', () => ({
get: mockGetSettings
}));

This comment has been minimized.

Copy link
@stefanjudis

stefanjudis Mar 4, 2019

Owner

Looks reasonable! 馃憣


const notify = (
options: NotificationConstructorOptions,
clickHandler: () => void

This comment has been minimized.

Copy link
@stefanjudis

stefanjudis Mar 4, 2019

Owner

Nit: can we change that to an options object? When I saw this code being used the callback wasn't really clear to me.

notify({}, {onItemClick: () => { ... })

would make it easier to grasp from the other side. :)

const publicationDate = new Date(item.pubDate);
return isToday(publicationDate) || isYesterday(publicationDate);
});
if (recentIncidents.length) {

This comment has been minimized.

Copy link
@stefanjudis

stefanjudis Mar 4, 2019

Owner

These three if conditions do more or less the same. Would a method like

notifyAboutIncident({
  incident,
  title: 'New incident reported'
})

make sense?

@adamwatters

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

On a trickier note, i checked out the branch and running npm run dev results in a bunch of typescript errors. Is it working for you? Can I help somehow?

Edited: something is odd, because travis is failing with a weird dep error, too.

Strange? I just made a fresh clone, ran npm install, and npm run dev worked fine for me. I'll keep digging.

@stefanjudis

This comment has been minimized.

Copy link
Owner

commented Mar 5, 2019

Hmm... let me check that tmrw. Maybe it's my machine that has npm hassle... 馃檲

@stefanjudis

This comment has been minimized.

Copy link
Owner

commented Mar 6, 2019

Okay, I had a deeper look. Here's what I did:

Locked deps with Corrupted electron?

rm -rf node_modules && npm i
npm run dev

this leads to the following thing

npm WARN tarball tarball data for electron@4.0.2 (sha512-UWFH6SrzNtzfvusGUFYxXDrgsUEbtBXkH/66hpDWxjA2Ckt7ozcYIujZpshbr7LPy8kV3ZRxIvoyCMdaS5DkVQ==) seems to be corrupted. Trying one more time.
npm WARN tarball tarball data for electron@4.0.2 (sha512-UWFH6SrzNtzfvusGUFYxXDrgsUEbtBXkH/66hpDWxjA2Ckt7ozcYIujZpshbr7LPy8kV3ZRxIvoyCMdaS5DkVQ==) seems to be corrupted. Trying one more time.

100% drop and install

rm -rf node_modules && rm package-lock.json && npm i
npm run dev

leads to

src/netlify.test.ts:33:18 - error TS2352: Conversion of type 'typeof fetch' to type 'Mock<typeof fetch, any>' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
  Type 'typeof fetch' is missing the following properties from type 'Mock<typeof fetch, any>': getMockName, mock, mockClear, mockReset, and 11 more.

33   const mFetch = fetch.default as jest.Mock<typeof fetch.default>;
                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...

and a bunch of other ts errors.


Travis is failing with the first case, too. I have honestly no clue. But I guess something is wrong with the lock file and it works for you because you have something sitting in your cache?

The complete reset might then update ts or tslint and this leads to new errors. Could this make sense?

BTW: thanks a bunch that you're still around. This goes for a little bit and I really appreciate it!

@adamwatters

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

BTW: thanks a bunch that you're still around. This goes for a little bit and I really appreciate it!

No worries! I'm enjoying working on this. Hope it hasn't created too much of a hassle for you.

I just ran the 100% drop and install and am looking at the ts errors now. maybe they're reasonable errors worth fixing?

@stefanjudis

This comment has been minimized.

Copy link
Owner

commented Mar 6, 2019

No worries! I'm enjoying working on this. Hope it hasn't created too much of a hassle for you.

Not at all. I'm happy about every help with this! 馃槉

maybe they're reasonable errors worth fixing?

Yeah... they most likely show up due to a dep update that was locked. Fixing now might be the way to go. 馃

@adamwatters

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

alls seem to be related to import * as fetch from 'node-fetch'; in netlify.test
maybe it node-fetch was updated?

@stefanjudis

This comment has been minimized.

Copy link
Owner

commented Mar 6, 2019

Could be... Here the day's over now (I'm in Berlin, Germany) but if you figure that out I guess I owe you a 鈽 in NYC when I'm around. 馃檲

@adamwatters

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

Could be... Here the day's over now (I'm in Berlin, Germany) but if you figure that out I guess I owe you a 鈽 in NYC when I'm around. 馃檲

鈽曪笍 馃 馃椊 馃槃

@adamwatters

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

@stefanjudis just a friendly poke :)

@stefanjudis

This comment has been minimized.

Copy link
Owner

commented Mar 14, 2019

Aaaaah.... so sorry Adam. I'm currently in talk prep. Sunday, Monday latest, we'll get this baby shipped.

@johnnyxbell

This comment has been minimized.

Copy link
Collaborator

commented Mar 14, 2019

Aaaaah.... so sorry Adam. I'm currently in talk prep. Sunday, Monday latest, we'll get this baby shipped.

@stefanjudis do you want me to merge this one?

@stefanjudis

This comment has been minimized.

Copy link
Owner

commented Mar 14, 2019

I'd like to give it a final round of testing before. But thanks for offering. 馃槉

Btw. @adamwatters I don't mind giving away merge permissions (as long as we follow PRs) if you want to have it. :)

@adamwatters

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2019

@stefanjudis no worries!

it鈥檚 ok - i dont mind not having the permissions - maybe down the road if i keep contributing to the project 馃憤

@stefanjudis

This comment has been minimized.

Copy link
Owner

commented Mar 18, 2019

Apart from my last question I'm happy to merge. :)

Forget that... 馃檲 Didn't include an .nvmrc in the project.

@stefanjudis stefanjudis merged commit 1d2c496 into stefanjudis:master Mar 18, 2019

3 checks passed

codecov/patch 44.3% of diff hit (target 33.96%)
Details
codecov/project 37.27% (+3.3%) compared to 2544628
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@stefanjudis

This comment has been minimized.

Copy link
Owner

commented Mar 18, 2019

馃帀 Thanks so much @adamwatters!

@stefanjudis

This comment has been minimized.

Copy link
Owner

commented Mar 18, 2019

FYI i started implementing the scheduler. Will ship both in the upcoming days. :)

@adamwatters

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

@stefanjudis 馃檶 thank you! hope the talk went well. was it recorded?

@stefanjudis

This comment has been minimized.

Copy link
Owner

commented Mar 19, 2019

Alright. I implemented the scheduler. 馃帀

I'll run with the newly built version for a day and then let's release this!

hope the talk went well. was it recorded?

Yep. Talk went great. And it was recorded. I also give it again at JSConf EU. 馃帀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.