refs https://linear.app/tryghost/issue/CORE-49/fix-errors-in-utils-repo-limit-service
- As I've touched these files did a little refactor and changed where the error messages are stored to keep it up with our lates coding standard - having "messages" hash defined in the module storing all messages that have pottential for i18y in the future.
refs https://github.com/TryGhost/Team/issues/870
- currently, a handful of packages in this repo generate type
declarations when building
- we're aiming to go more in the direction of better inline jsdoc, and
we want to clean up all generated files and use of external types
- this commit removes the `typescript` dependency, removes the pretest
commands that run the builds, and cleans up anywhere where we
configure types
refs https://github.com/TryGhost/Team/issues/870
- using `c8` allows us to see test coverage for all packages in the repo
- this commit adds `c8` as a dev dependency and prepends the `mocha`
command with `c8` so it runs on all tests
refs https://github.com/TryGhost/Team/issues/727
- The latest code standards require using @tryghost/logging module instead of injecting it as a parameter in the construcotr
- Left it as an optional constructor parameter to make testing easier
refs https://github.com/TryGhost/Team/issues/727
- The latest code standards require using tpl module in combination with local messages object instead of i18n injected in the constructor
refs https://github.com/TryGhost/Team/issues/754
- When there are no message objects coming from the external update check service in the response there is no need to continue to process the data and fire off unneeded queries
refs https://github.com/TryGhost/Team/issues/754
- The api call can also generate an error which would bubble up to a client. This is not a good design if the client has to know to handle internal errors of the module
- By having try block wrap around whole block it makes the error handling behave the same way throughout the check process
- added getSubdir, getSiteUrl and getAdminUrl methods from url-utils
- refactored them so they are designed to be bound by nconf
- exposed a bindAll method
- moved over the tests and refactor from a class instance to nconf bindings
no issue
Replaces the usage of ghost-ignition with @tryghost/ignition-errors,
and switched to using the more modern ES6 class syntax over the
`.call(this, ...)` syntax used previously in both Ignition and here.
- fixed type warnings
- introduced explicit use of bluebird to help with type warnings and also make it clear when we're actually using Bluebird features
- removed the class pattern in the simplest possible way
- have package-json expose only the public methods
- move parse to an independent, not public file so that we can test it independently still
- We are going to get rid of the internal i18n tool because it doesn't solve a real use case
- Instead, we have a new tpl utility that does basic string interpolation
- This makes this module less complex as it no longer requires dependency injection to work
- ts complains the value may be undefined but in reality it never is
- plus we don't care about this error in this context as the test is checking that the value is set to its default
- This mini helper wraps lodash template and supports `{...}` as the delimiters
- It's designed to use and support the exact same patterns we already have in our en.json strings
E.g. The {flagName} flag must be enabled in labs if you wish to use the \\{\\{{helperName}\\}\\} helper.
- This allows us to get rid of our old, broken i18n helper and still keep some of the smart messaging we have setup
- It also keeps the refactor surface area minimal
refs TryGhost/Team#728
- These are dependancies that have to come with the update check service
- Used exact same versions of Bluebird/moment/lodash as in current
TryGhost/Ghost main
refs https://github.com/TryGhost/Team/issues/728
- This is continuation of the previous commit. TLDR: Passing only the necessary parameter data makes it easier to reason about what dependencies the UpdateCheckService has to deal with
- Instead of passing in a whole GhostMailer instance passing only an email sending function, which again - makes things way more manageable to reason about
- The end of refactor, next will be a move of the UpdateCheckService into a separate module in tryghost/core
refs https://github.com/TryGhost/Team/issues/728
- This is continuation of the previous commit. TLDR: Passing only the necessary parameter data makes it easier to reason about what dependencies the UpdateCheckService has to deal with
- Burned ghostVersion module passing in vafor of just one additional config parameter. Now the module along with unit tests can be easily extracted out of the codebase!
refs https://github.com/TryGhost/Team/issues/728
- This is continuation of the previous commit. TLDR: Passing only the necessary API endpoint function makes it easier to reason about what dependencies the UpdateCheckService has to deal with
- Substituted a parameter with already existing 'siteUrl' config value. No need to duplicate work!
refs https://github.com/TryGhost/Team/issues/728
- This is continuation of the previous commit. TLDR: Passing only the necessary API endpoint function makes it easier to reason about what dependencies the UpdateCheckService has to deal with
- Limited urlUtils to only one function as that's all the UpdateCheck uses. Next step will be removing the function completely as and passing a 'blogURL' as a config value (way better readability this way)
refs https://github.com/TryGhost/Team/issues/728
- This is continuation of the previous commit. TLDR: Passing only the necessary API endpoint function makes it easier to reason about what dependencies the UpdateCheckService has to deal with
- There are 8 different configs that NotificationService depends upon it will need some further investigation around which ones are even needed anymore and the naming is not the best. To keep the time cap at bay leaving it at what it is.
refs https://github.com/TryGhost/Team/issues/728
- Passing only the necessary API endpoint function makes it easier to reason about what dependencies the UpdateCheckService has to deal with
- The instance initialization had to be moved insided the module's exports to resolve "models" module initialization failure
refs https://github.com/TryGhost/Team/issues/728
- Previous name was after the do-all-the-things mega module that have now become an "initializer" for the UpdateCheckService class. The unit tests are testing the latter, so the rename is a cleanup from the previous ways
refs https://github.com/TryGhost/Team/issues/728
- This is a continuation of the test coverage for the UpdateCheckService.
- Covers scpecial cases of notification processing within Update Check
- The refactor inside the update check service was a convenience to get rid or the Bluebird dependency completely. Also, some minor preventative code added to avoid errors from referencing undefined objects
refs https://github.com/TryGhost/Team/issues/728
- In additions to easier tracking of "this" context in the unit tests it gets rid of unnecessary Bluebird's "reflect" method which was making unit test dependent on Bluebird's specific Promise implementation
refs https://github.com/TryGhost/Team/issues/728
- This is a first step before moving update check code into an outside codebase.
- The aim is to have a self-contained module which could be unit tested and have a very clear API
https://github.com/TryGhost/Team/issues/663
- When there is no parameter passed at all it was a generic 'Cannot read property 'value' of undefined' message which wasn't helpful in recognizing what the actual problem was
- Have added additional guarding logic to throw a descriptive error
no issue
- I've discovered the "IncorrectUsageError" error was silently swallowed and the method returned a false positibe when an allowlist limit type was called with incorrect parameters
- In cases like this it's best to surface the real error early otherwise the logic might produce unsafe results!
refs https://github.com/TryGhost/Team/issues/662
- There is a need to check if any of the current limits are over limit in Daisy. This method is the simplest possible implementation to check if any of them are over limit
- Possible future iterations might include a list of names of the limits that have been acceded and their error messages
- The `checkIfAnyOverLimit` method should be treated as a starter to work up the complexity as needed
refs https://github.com/TryGhost/Team/issues/510
- There's a limited type of limit "names" supported by the limit service, so worth specifying them upfront. Also some limits are univerally aplicable like "flag" or "allowlist" and some are restricted like "max" and "maxPeriodic"
refs https://github.com/TryGhost/Team/issues/588
- The "emails" limit was added with recent changes and could be configured as either "flag" or "maxPeridoci" type of limit
- More docs on different types of limits to follow
refs https://github.com/TryGhost/Team/issues/588
- The `addedCount` parameter in `errorIfWouldGoOverLimit` method allows to specify a custom resource count that is about to be added. Example usecase is when we'd want to send a 100 emails and current limit is 99, and none have been sent so far. With previous implementation the check would've passed because it only checked for single resource that would be added through "+1". Current implementation allows to specify the amount of recources to be added
refs https://github.com/TryGhost/Team/issues/588
- The previous query was quickly copied from stats-service which was using incorrect table for the count
- Updated version sums up email_count values for emails in given period of time
no issue
- this commit adds the c8 dependency to the `package-json`, and prepends
it to the test alias so we can see the coverage in tests
- note: we're apparently already at 100%!
refs https://github.com/TryGhost/Team/issues/588
refs 6a1e722648
- date-fns proved to be unable to manipulate dates in consistent UTC format and was substitured with luxon in referenced commit. Removing it from tests for consistency
refs https://github.com/TryGhost/Team/issues/588
- date-fns proved to be unable to manipulate dates consistently in UTC timezone. Keeping all calculations and formatting in UTC is key to have consistency in dates when dealing in inter-system dates
- day.js also failed the test for correct UTC manipulation. See https://github.com/iamkun/dayjs/issues/1271 for example bug which prevents from consistent correct calculation
- luxon was the best option which WORKED. It's also a recommended successor for moment.js with really nice docs and active support
refs https://github.com/TryGhost/Team/issues/588
- This is a basic implementation which needs a review. Implemented it to fix failing tests in main
- Start date is expected to come formatted for DB's needs
no issue
- `Error` is very generic for this case and `IncorrectUsageError`
will populate the resulting error with the correct error code
- the `message` was pulled out to its own statement so we can avoid long
lines
no issue
- we're preparing the `package-json` lib to be extracted out of Ghost into
its own package so moving the initialization wrapper outside of the
folder makes the process a lot easier
refs https://github.com/TryGhost/Team/issues/588
- The limit service can now be initialized with a config which has a 'maxPeriodic' key identifying it's a special type of limit taking subscription cycles into account
- Example configuration can be found in the included unit tests
refs https://github.com/TryGhost/Team/issues/588
- There's a need to calculate when the last period has started to be able to generate correct counting queries for the "maxPeriodical" limit
- It operatest on ISO strings as an input and output in UTC timezone to take timezone calculations out of the equation
- Refer to inclucded unit tests for example calculations
refs https://github.com/TryGhost/Team/issues/588
- This is a scaffolding for a new limit type which should allow to check limits based on periods (for example related to billing, subscription cycles)
refs 90ca836cb6
- i18n is used everywhere but only requires shared or external packages, therefore it's a good candidate for living in shared
- this reduces invalid requires across frontend and server, and lets us use it everywhere until we come up with a better option
- Having these as destructured from the same package is hindering refactoring now
- Events should really only ever be used server-side
- i18n should be a shared module for now so it can be used everywhere until we figure out something better
- Having them seperate also allows us to lint them properly
refs https://github.com/TryGhost/Team/issues/588
- This is by no means an thorought test coverage but ensures the basics work and provides examples of how the limit should be used. To be continued :)
refs https://github.com/TryGhost/Team/issues/588
- This is a step 1 in the introduction of email limits. Next step would be allowing this limit to support "periodical limit checks"
no issue
- I was a little confused seeing an empty object in the config moduele - `customThemes: {}` and initially thought we could get rid of it to reduce the amount of code. Afte quick dig found out that there's a purpuse behind it being there! It's an allowlist of the properites that can be defined within the limit service
- Added notes to clarify the usecase and avoid ambiguity in the future
no issue
- later updates of this package contain different types that we haven't
changed our code for yet, so I need to revert the pinning to force
this specific version for now
no issue
- `extract-zip` v2.0.1 currently requires Node 10.17.0, which we're not
ready to bump our minimums to
- we'll probably bump this when we drop Node 10 at the end of April 2021
no issue
- this Utils repo contains libraries, whose dependencies should not be
pinned in order to reduce multiple versions of the same package
appearing for consumers
refs https://github.com/TryGhost/Team/issues/599
- There are cases when there'a a need to reload limits with a new set of configuration. For example, when Ghost is run in a test environment is a soft reboot is done
- Resetting previous value of limits avoids having conflicting state after multiple calls
refs https://github.com/TryGhost/Team/issues/510
- Flag limits are impossible to check if they are "over a limit already" as they are just that - on/off flags. Therefore it should be directly noted that the method is there to keep the "Limit" interface and not be relied upon
refs https://github.com/TryGhost/Team/issues/587
- Improved description and provided example use of error message template variables that are available for "MaxLimit" types of limits
no issue
- These files kept generating a new ouput when trying to publish an unrelated package. Assuming the generation is orrect and commiting this just to get things out of the way (type definition files should not break any functionality)
refs https://github.com/TryGhost/Team/issues/510
- {{max}} and {{count}} variable usage was not covered but had valid usecases in the library client's, so considered to "document" them through tests
- For more context these variables are available in custom `error` templates that are provided with each limit
refs https://github.com/TryGhost/Team/issues/597
- When the library is used on a client without a DB connection (e.g. frontend client running in a browser) the library needs to expose a way to override count queries.
- The way these can be used is giving a count based on a HTTP request or some other data provider
- Example use with max limit like "staff" would be loading the limit servcie if following way:
```
const limitService = new LimitService();
let limits = {
staff: {
max: 2,
currentCountQuery: () => 5
}
};
limitService.loadLimits({limits, errors});
await limitService.checkIsOverLimit('staff')
```
refs https://github.com/TryGhost/Team/issues/597
- To be able to transpile the library for different runtimes (make it polymorphic) had to get rid of dependencies that were not compatible with ES Modules
- By making errors an injectable constructor option it removes the depencency and allows to transpile the library for multiple targets
- The `errors` option is now a required parameter for `loadLimits` method. It errors if it's missing (error message copy inspired by content api error 69fcea0582/packages/content-api/lib/index.js (L21))
refs https://github.com/TryGhost/Team/issues/597
- Before adding more parameters documented existing ones
- Created LimitConfig type definition to have easier look into the structure of limit conifiguration
refs https://github.com/TryGhost/Team/issues/587
- Documented common usecases such as:
1. initialization and configuration of limit service
2. usage of "max" types of limits
refs https://github.com/TryGhost/Team/issues/587
- There was no test coverage for MaxLimit's errorIfIsOverLimit check. Added basic test to make sure upcoming modifications don't break existing functionality
refs https://github.com/TryGhost/Team/issues/587
- The optional {max} passed as an option allows to override currently configured limit and do a theoretical new limit check. For example: check if the max limit will be exceeded if the limit changes (user changes plans)
refs https://github.com/TryGhost/Team/issues/587
- Having a JSDoc gives better intellisense when the class is instantiated and provides clues about what each parameter might be used for
refs https://github.com/TryGhost/Team/issues/587
- When the 'max' configuration is missing the instance of the class breaks when used unexpectedly. Followed similar approach to currentCountQuery check by failing fast in the constructor
refs https://github.com/TryGhost/Team/issues/587
- Test were missing for class initialization and around how the limit currently works.
- Before extending it's behavior throught its valuable to cover current functionality to not accidentally break anything
refs: https://github.com/TryGhost/Team/issues/510
- Ghost config always uses camelcase. This was incorrectly implemented with snake case originally
- Swap to use camelCase by default, which is desirable, but support both
- It's really easy to support both in the loader and isLimited check, so we do this to stop ourselves tripping on this later
refs: https://github.com/TryGhost/Team/issues/510
- we need to make sure we take into account any invites that could be accepted at any time
- this counts all invites for non-contributor roles as well as all users who aren't contributors
- this should stop there being loop holes to inviting staff users
refs: https://github.com/lodash/lodash/issues/705
- Was seeing unexpected token = errors when using lodash templates in Ghost
- This is because we're setting template settings globally in this dependency and it affects every other user of lodash
- Using runInContext keeps this templateSettings change local to this lib
- Test proves that after requiring limits we can require lodash and have the default values again
refs https://github.com/TryGhost/Ghost/issues/12496
- `workerMessageHandler` option allows for custom worker message handling and allows to eliminate a need for loggers of any type inside of jobs.
- removing loggers from jobs solves file hanle leak which used to cause Ghost process to crash (see referenced issue)
refs #122
- In future changes there's a plan to add "inline" scheduled jobs, which would conflict current method naming.
- The amount of parameters in the methods was more than 3, so it made sense to transform them into an options object
- Scheduling still doesn't work for "inline" jobs. This should be solved as a part of upstream library (https://github.com/breejs/bree/issues/68)
closes https://github.com/TryGhost/Ghost-Utils/issues/118
- Custom error handling is needed to be able to override default bree
error handling logic.
- bree bump to 4.1.0 also fixed logging errors (object Object fix in
tests)
- The handler function receives two parameters. First contains an error
that has been thrown by the job. Second, job and worker metadata
