fix: Toggl API url #20
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The code incorrectly used "www.api.track.toggl.com" which would cause the application to crash on launch (e.g. `npm start`). This commit changes the API url to "api.track.toggl.com". Testing done: * `npm start`: Confirmed app runs and loads my data. * `npm test`: Confirmed all tests pass.
Without this commit, using the `b` command to open Toggl in the browser would fail by having the browser opening a 404 page. This commit involves a very minor rearchitecting. Previously, there was one URL "www.toggl.com" that could be used for both the JSON-based API endpoint, and the for-humans web browser endpoint. Toggl has since split this into "www.toggl.com" for humans and "api.track.toggl.com" for API access. The concept of the `URL` thus no longer makes sense, and I've removed that. The concepts of the `API_URL` and the `TIMER_URL` remain. I've updated the tests to not assert the existence of the `URL` definition, under the reasoning that this is an implementation detail that should be free to change, and not part of the `src.toggl.js` file's public API. Similarly, I've changed the tests on the `API_URL` and `TIMER_URL` constants to not assert that the URL has a specific value (e.g. that it contains "/api") because that too is an implementation detail that may change (if Toggl decides to change its URLs around). Instead, the tests just assert that we're using an https connection (although that too may be an implementation detail? No other code relies on that property, but perhaps the maintainers would like to be alerted via a test failure if that ever changes...) Testing done: * `npm start b`: Confirmed that using the command line to open the browser now works. * `npm start`: Confirmed that pressing `b` in the interactive UI to open the browser no works. * `npm test`: Confirmed all test pass.
open 0.0.5 has an arbitrary code execution vulnerability https://www.npmjs.com/advisories/663 This codebase was unlikely to be affected because we only ever pass constant strings (e.g. "https://www.toggl.com/timer") to `open`, but I upgraded the dependency anyway for good housekeeping. Testing done: * `npm start b` confirmed opening browser from command line still works. * `npm start` confirmed opening browser from interactive UI still works. * `npm test` confirmed tests still pass.
mocha 3.0.2 has a dependency on an older version of `growl` which allowed for arbitrary code execution. https://www.npmjs.com/advisories/146 Testing done: * `npm start`: Confirmed interactive UI correctly loads my data. * `npm start c`: Confirmed command line UI correctly loads my data. * `npm test`: Confirmed tests still pass.
The last commit on https://github.com/douglasduteil/isparta is from 2019-Dec-31, and it says that the package is deprecated, and you should use `nyc` instead. The dev experience is largely the same (you still use `npm test` to run linting, unit tests and code coverage), but the output is formatted slightly differently. Testing done: * `npm test` confirmed code coverage information is still displayed.
This repo was already using coveralls.io to generate its code coverage badge. It's unclear how the badge value was being updated (manually?) but there is now an npm script you can use to run update the code coverage badge. Testing done: * `npm run coveralls.io`: Confirmed that the badge is updated at https://coveralls.io/github/NebuPookins/toggl-cli
Looks like nethunter did not run `npm test`, so this commit also fixes some of the lint issues and tests so that they pass again. Minor behaviour changes from nethunter's code: * Previously the project property could be parsed as hexdecimal, e.g. if the user entered in `proj:FF`. I suspect this was unintentional. Now project numbers must be in decimal. `proj:FF` will NOT be interpreted as referring to project number 255, but instead be interpreted as the project whose name starts with "FF". * Previously there was no handling for unrecognized properties, e.g. (`foo:bar`). Now, if we encounter an unrecognized property, we assume it's actually part of the name of the task, thus allowing for tasks to contain colons in them. Testing done: * `npm start pr`: Confirmed my projects were printed. * `npm start cl`: Confirmed my clients were printed. * `npm start start proj:7 Test` confirmed the task was started with project 7. * `npm start stop proj:7 Test` confirmed the task was stopped with project 7. * `npm start start proj:MyProj Test` confirmed the task was started with project whose name starts with "MyProj". * `npm start stop proj:MyProj Test` confirmed the task was stopped with project whose name starts with "MyProj". * `npm start -- --help`: Confirmed help info is displayed. * `npm start -- --examples`: Confirmed examples are displayed. * `npm start foo:bar Test`: Confirmed this starts a task with the name "foo:bar Test" (i.e. unknown properties are interpreted as part of the task name.)
coveralls 2.11.14 depended on an older version js-yaml which had two security issues: 1. https://www.npmjs.com/advisories/788 Denial of service from a specifically crafted YAML file. 2. https://www.npmjs.com/advisories/813 Arbitrary code execution from a specifically crafted YAML file. Testing done: * `npm test` confirmed code coverage is still calculated. * `npm run coveralls.io` confirmed coverage data is still sent to coveralls.io.
The code base uses some features only available in NodeJS 6: * Destructuring * Default parameters Testing done: * `npm start` configured UI loads up my data. * `npm test` confirmed all tests pass.
This is in preparation to upgrade xo. The latest version of the xo linter enforces that we use JavaScript modules instead of CommonJS style modules. Testing done: * `npm start` Confirmed loads up my data. * `npm test` Confirmed still have 140 tests passing.
There's a security vulnerability with the Buffer constructor. See https://nodejs.org/en/docs/guides/buffer-constructor-deprecation/ for more details. Testing done: * `npm start list` confirmed my data still loads. * `npm test` confirmed all tests still pass.
This is part of an effort to upgrade the `xo` linter. The current version of `xo` pulls in an old version of trim-newlines which contains a security vulnerability. In upgrading `xo`, the linter enforces several new things, one of which is the use of ESM style modules (as opposed to AMD or CommonJS style modules, for example). This commit makes the switch to ESM modules in isolation from all the other changes, to make it easier to trace any bugs this refactoring may have introduced. Notable changes: * The project previously used a custom module system https://github.com/meeDamian/mee that builds upon the CommonJS module system. I've written a small module system `meeEsm` which mimics what `mee` did, but using the ESM module system as its base. * The `mee` module system expects you to place the object containing all the fields you want to export in some local variable (by convention, named `me`), and the `mee` module will mutate that object, and return a new object, which it expects you to reassign to the `me` variable. That new object is also the object that actually gets exported. This means that functions that wish to act on the exported object can reference the `me` local variable. In contrast, the `meeEsm` module system does not mutate the passed in object, and simply returns an object which you are then responsible for exporting (e.g. via the `export default` keywords). Functions which wish to act on that object should use the `this` pointer. TL;DR, all the code that previously had method invocations like `me.someMethod()` now look like `this.someMethod()`. * CommonJS style modules have a `require` function, and the old code used this function to read in the `package.json` file (e.g. to report version numbers). For ESM style modules, I created a new module called `pkg` which exposes the data from the `package.json` file without relying on the `require` function. * The test for `core` asserted that a couple specific constants were exported (e.g. the number of seconds in a day). These constants were only used within the `core` module itself. I've removed these assertions from the test, as I believe the existence of these constants are an implementation detail, and not part of its public API. * Similarly, one of the test for `config` was asserting some internal implementation details, such that `config`'s `open` method must call its own `getPath` method. What methods config calls to implement its functionality should not be part of its API, so these tests were removed. Testing done: * `npm test` all tests passed (but note that I removed a couple of tests). * `npm start list` confirmed my data is still loaded. * `npm start` confirmed interactive UI still loads and is navigatable.
This is in preparation to upgrade the `xo` linter from 0.16 to 0.44. The new version of the linter enforces several new things. This commit contains a bunch of the "easy fixes" that the new linter enforces, so that the difficult changes (e.g. switching from Promises to await/async) can be done in their own commit. Testing done: * `npm test` confirmed tests still pass. * `npm start` confirmed interactive UI loads data. * `npm start list` confirmed CLI UI loads data.
… 5.3.0 -> 7.1.1, chai-spies 0.7.1 -> 1.0.0.
This is in preparation to upgrade the `xo` linter. One of the
enforcements in the newest version of `xo` is that you should use
`async/await` instead of promises. Older versions of `chai` incorrectly
consider async functions to not be a function (i.e. `let f = async
function() {}; f.should.be.a('function');` fails).
Upgrading chai and related libraries allows the test to continue passing
after we change some functions to use the async/await style instead of
directly returning promises.
Testing done:
* `npm test` confirmed all tests still pass.
This is in preparation to ugprade the `xo` linter. One of the enforcements in the newest version of `xo` is to use the async/await style instead of explicit promises. Version 0.30.0 of `fs-extra` does not have good support for the async/await style, but 10.0.0 does. Testing done: * `npm test` Confirmed all tests still pass. * `npm start` Confirmed interactive UI loads my data. * `npm start list` Confirmed CLI UI loads my data.
In the git commit 76d171a I performed "miscellaneous" fixes to satisfy the `xo` linter. One of the fixes xo suggested was to prefer `Number.isNaN` over `isNaN`. I applied that fix blindly, not realizing the two functions have slightly different behaviour. This led to a bug which can be replicated by running the command `npm start list last friday`. `isNaN('friday')` returns `true` because "friday" is not a number. However, Number.isNaN('friday') returns false, because "friday" is not the special IEEE value NaN. This commit simply reverts the change that introduced the bug. A future commit will be needed to add code coverage to ensure this doesn't come back as a regression. Testing done: * `npm test` confirmed all tests still pass. * `npm start` confirmed interactive UI loads my data. * `npm start list` confirmed CLI UI loads my data. * `npm start list last friday` confirms specifying a relative date works.
`nyc` seems to have trouble generate coverage report for code using ESM (see https://github.com/andrezero/boilerplate-esm-nyc-mocha as an example workaround attempts). `c8` seems to just work out of the box, and its output is identical to what `nyc` produced for our use case. Testing done: * `npm test` confirmed coverage reports are generated. * `npm run coveralls.io` confirmed coverage report is uploaded to coveralls correctly.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The code incorrectly used "www.api.track.toggl.com" which would cause
the application to crash on launch (e.g.
npm start). This commitchanges the API url to "api.track.toggl.com".
Testing done:
npm start: Confirmed app runs and loads my data.npm test: Confirmed all tests pass.This change is