Skip to content

fix(related-data): use same reference on record for dataValues and direct attributes#574

Merged
GuillaumeCisco merged 3 commits intomasterfrom
fix-smart-field-magic-accessors
Dec 8, 2020
Merged

fix(related-data): use same reference on record for dataValues and direct attributes#574
GuillaumeCisco merged 3 commits intomasterfrom
fix-smart-field-magic-accessors

Conversation

@GuillaumeCisco
Copy link
Contributor

@GuillaumeCisco GuillaumeCisco commented Dec 2, 2020

Pull Request checklist:

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

For not dealing with too much modification, I choose to directly overwrite the magic accessor on a field if one of its smart field has the same name.
As we are in a serialization process, this will not introduce side effects.
Not using dataValues (which in sequelize allow to isolate a context without magic accessors) will make it works with forest-mongoose too.

Others options are:

@forest-bot
Copy link
Member

Copy link
Contributor

@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.

Peer reviewed with Steve, but I think he will also add a few comments.

Manual tests on sequelize seems ok to me. Added a few comments/suggestion, but feature-wise it LGTM.

@jeffladiray jeffladiray removed their assignment Dec 3, 2020
Copy link
Contributor

@SteveBunlon SteveBunlon left a comment

Choose a reason for hiding this comment

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

Looks good to me, I've tested it using mongoose (smart field + smart relationship).

I've written some comments but most of them are [OPINION] so skippable.

@qlty-cloud-legacy
Copy link

Code Climate has analyzed commit 4e7a54d and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

The test coverage on the diff in this pull request is 100.0% (56% is the threshold).

This pull request will bring the total coverage in the repository to 51.4%.

View more on Code Climate.

Copy link
Contributor

@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.

LGTM

@SteveBunlon SteveBunlon removed their assignment Dec 8, 2020
@GuillaumeCisco GuillaumeCisco merged commit c65588e into master Dec 8, 2020
@GuillaumeCisco GuillaumeCisco deleted the fix-smart-field-magic-accessors branch December 8, 2020 09:36
forest-bot added a commit that referenced this pull request Dec 8, 2020
## [7.8.9](v7.8.8...v7.8.9) (2020-12-08)

### Bug Fixes

* **related-data:** use same reference on record for dataValues and direct attributes ([#574](#574)) ([c65588e](c65588e))
@forest-bot
Copy link
Member

🎉 This PR is included in version 7.8.9 🎉

The release is available on:

Your semantic-release bot 📦🚀

forest-bot added a commit that referenced this pull request Dec 8, 2020
# [8.0.0-beta.5](v8.0.0-beta.4...v8.0.0-beta.5) (2020-12-08)

### Bug Fixes

* **related-data:** use same reference on record for dataValues and direct attributes ([#574](#574)) ([c65588e](c65588e))
* **smart-action:** do not mutate hooks on schema generation ([#580](#580)) ([dd2aee3](dd2aee3))
* **smart-action:** widgetEdit should not be erased when change hook is triggered ([#579](#579)) ([1014ade](1014ade))
* **smart-actions:** error message details missing for hooks ([#582](#582)) ([d2edf35](d2edf35))
* **smart-actions:** reset value when not present in enums in hook response ([#584](#584)) ([0f57a46](0f57a46))
* **smart-actions:** use changedField instead of comparing values to trigger the correct change hook ([#583](#583)) ([54d536b](54d536b))
* record not found in hooks (recordsId replaced with recordIds) ([#578](#578)) ([ccf6a8f](ccf6a8f))

### Features

* **role-permissions:** support the new role ACL format ([#577](#577)) ([4aed30f](4aed30f))
@forest-bot
Copy link
Member

🎉 This PR is included in version 8.0.0-beta.5 🎉

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.

4 participants

Comments