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

fix: fix incorrect usage of the new connections parameter #557

Merged
merged 3 commits into from
Nov 30, 2020

Conversation

arnaud-moncel
Copy link
Member

Pull Request checklist:

  • Write an explicit title for the Pull Request, following Conventional Commits specification
  • Create automatic tests
  • Test manually the implemented changes
  • Review my own code (indentation, syntax, style, simplicity, readability)
  • Wonder if you can improve the existing code

Copy link
Member

@jeffladiray jeffladiray left a comment

Choose a reason for hiding this comment

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

Nice 👍
This should fix a lot of potential issues related to multi database. Nice catch.

Maybe just fix: fix incorrect array usage of the new connections parameter would be better DYT ?

@@ -19,7 +19,7 @@ const HasManyDissociator = require('../src/services/has-many-dissociator');
const models = {};
const sequelizeOptions = {
Sequelize,
connections: [sequelize],
connections: { sequelize },
Copy link
Member

Choose a reason for hiding this comment

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

Totally my fault, did not catch that during the review. This would have save us from this whole PR ...

sort: '-id',
segmentQuery: 'select * from users\nwhere id in (100, 102);',
timezone: 'Europe/Paris',
};
try {
const result = await new ResourcesGetter(models.user, sequelizeOptions, params)
.perform();
expect(result).toHaveLength(2);
expect(result[0]).toHaveLength(2);
Copy link
Member

Choose a reason for hiding this comment

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

At least, we discovered that this test was not working for a while 🥇

@arnaud-moncel arnaud-moncel changed the title fix: fix database type detection fix: fix incorrect usage of the new connections parameter Nov 26, 2020
@arnaud-moncel arnaud-moncel merged commit 2840e41 into beta Nov 30, 2020
@arnaud-moncel arnaud-moncel deleted the fix/utils-database branch November 30, 2020 09:02
forest-bot added a commit that referenced this pull request Nov 30, 2020
# [7.0.0-beta.2](v7.0.0-beta.1...v7.0.0-beta.2) (2020-11-30)

### Bug Fixes

* fix incorrect usage of the new connections parameter ([#557](#557)) ([2840e41](2840e41))
@forest-bot
Copy link
Member

🎉 This PR is included in version 7.0.0-beta.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

forest-bot added a commit that referenced this pull request Feb 22, 2021
# [7.0.0](v6.7.11...v7.0.0) (2021-02-22)

### Bug Fixes

* **authentication:** error when authenticating with an invalid token in cookies ([#593](#593)) ([405feb4](405feb4))
* connect to liana through safari ([#590](#590)) ([6a0fcb3](6a0fcb3))
* fix incorrect usage of the new connections parameter ([#557](#557)) ([2840e41](2840e41))
* user being disconnected after 33min instead of 14 days ([#591](#591)) ([2e2dc81](2e2dc81))

### Features

* init function now uses connections & objectMapping instead of sequelize as parameter ([#539](#539)) ([74262ac](74262ac))
* return correct errors when the user needs to configure the 2FA ([#609](#609)) ([08b64a0](08b64a0))

### BREAKING CHANGES

* sequelize options is not supported anymore by Liana.init()
connections and objectMapping is now required on Liana.init().
Update forest-express dependency to 8.0.0-beta.1 (See https://github.com/ForestAdmin/forest-express/tree/v8.0.0-beta.1)

Co-authored-by: jeffladiray <ladirayjeff@gmail.com>
@forest-bot
Copy link
Member

🎉 This PR is included in version 7.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants