Skip to content

Conversation

rustagir
Copy link
Contributor

@rustagir rustagir commented Dec 2, 2024

Pull Request Info

PR Reviewing Guidelines

JIRA - https://jira.mongodb.org/browse/DOCSP-45361
Staging - https://deploy-preview-75--docs-mongoid.netlify.app/data-modeling/callbacks/

Self-Review Checklist

  • Is this free of any warnings or errors in the RST?
  • Did you run a spell-check?
  • Did you run a grammar-check?
  • Are all the links working?
  • Are the facets and meta keywords accurate?

Copy link

netlify bot commented Dec 2, 2024

Deploy Preview for docs-mongoid ready!

Name Link
🔨 Latest commit 5766da5
🔍 Latest deploy log https://app.netlify.com/sites/docs-mongoid/deploys/67644201b1f2e800081c1f48
😎 Deploy Preview https://deploy-preview-75--docs-mongoid.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@rustagir rustagir marked this pull request as ready for review December 2, 2024 21:39
Copy link
Contributor

@norareidy norareidy left a comment

Choose a reason for hiding this comment

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

Looks really good! I left some suggestions / a question, so I'll take another look

<https://api.rubyonrails.org/{+rails-8-version-docs+}/classes/ActiveRecord/Callbacks.html>`__
reference in the Rails API documentation.

You can implement a callback both top-level and embedded documents.
Copy link
Contributor

Choose a reason for hiding this comment

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

S:

Suggested change
You can implement a callback both top-level and embedded documents.
You can implement a callback on both top-level and embedded documents.

Comment on lines 77 to 80
Take precautions and ensure testability when implementing callbacks for
domain logic, because these designs can lead to unexpected errors when
callbacks in the chain halt execution. We recommend using callbacks for
cross-cutting concerns, such as queueing up background jobs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: there's a couple terms in this paragraph (domain logic, cross-cutting) that I'm not familiar with - is it safe to assume readers should know these concepts, or do you think they're worth briefly explaining?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add some elaboration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Domain logic should be a phrase users are familiar with

This example demonstrates how to register callbacks on the ``Contact``
model class in the following ways:

- Includes the ``before_save`` class method which triggers the
Copy link
Contributor

Choose a reason for hiding this comment

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

S:

Suggested change
- Includes the ``before_save`` class method which triggers the
- Includes the ``before_save`` class method, which triggers the

Comment on lines 149 to 151
The following code demonstrates how to register an association callback
on a ``User`` model class that embeds multiple ``SavedArticle``
instances:
Copy link
Contributor

Choose a reason for hiding this comment

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

S: might be helpful to describe this code more in-depth, similar to the description for the "Document Callbacks" example. Right now it's not super clear how it works

following references in the Active Record documentation:

- `Skipping Callbacks <{+active-record-docs+}/active_record_callbacks.html#skipping-callbacks>`__
- `Suppressing Callbacks <{+active-record-docs+}/active_record_callbacks.html#suppressing-callbacks>`__
Copy link
Contributor

Choose a reason for hiding this comment

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

I: Looks like there isn't a "suppressing callbacks" section on this page, did you mean "suppressing saving"?

Suggested change
- `Suppressing Callbacks <{+active-record-docs+}/active_record_callbacks.html#suppressing-callbacks>`__
- `Suppressing Saving <{+active-record-docs+}/active_record_callbacks.html#suppressing-saving>`__

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops!

@rustagir rustagir requested a review from norareidy December 19, 2024 18:03
Copy link
Contributor

@norareidy norareidy left a comment

Choose a reason for hiding this comment

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

LGTM!

@rustagir rustagir merged commit 39bdd5b into mongodb:standardized Dec 19, 2024
4 of 5 checks passed
rustagir added a commit that referenced this pull request Jan 28, 2025
* DOCSP-42732: qs

* fix

* wip

* remove action

* NR suggestion

* DOCSP-42732: qs download

* wip

* adapt for sinatra

* vale fix

* snooty landing page

* MW PR fixes 1

* DOCSP-42733: atlas prep qs

* MW PR fixes 1

* DOCSP-42735: configure cxn

* small fix - vale

* JS PR fixes 1

* DOCSP-44008: read/write sinatra quickstart

* fixes

* NR PR fixes 1

* DOCSP-43961: rails qs

* vale

* ordering

* small fix

* MW PR fixes 1

* small fixes

* MW PR fixes 2

* DOCSP-44647: add to existing app

* fix vale action

* vale fixes

* depth

* MW PR fixes 1

* fixes

* DOCSP-42745: interact with data drawer

* tags

* fix vale action

* remove extra word

Co-authored-by: Nora Reidy <[email protected]>

* DOCSP-42753: specify query part 1

* vale

* title

* code edits

* MW PR fixes 2

* DOCSP-44849: modify results

* vale

* JS PR fixes 1

* fix

* fix

* list fixes

* MW PR fixes 1

* DOCSP-44954: scoping

* add landing page

* link

* vale

* highlighting

* DOCSP-44821: specify a query pt 2

* MR PR fixes 1

* wip

* wip

* wip

* small fixes

* DOCSP-42767 Aggregation (#57)

* DOCSP-42774: transactions

* vale

* link text

* MW PR fixes 1

* MR PR fixes 1

* DOCSP-45306: model data drawer

* DOCSP-45330: inheritance (WIP)

* MR PR fixes 2

* try using roles

* wip

* vale

* add label

* fixes

* fix

* small fixes - MW

* DOCSP-45358: documents

* fix

* wip

* wip

* DR tech review 1

* page fmt

* page fmt

* SA PR fixes 1

* MR PR fixes 1

* DR small fix

Co-authored-by: Dmitry Rybakov <[email protected]>

* DOCSP-45360: nested attributes (#71)

* DOCSP-45360: nested attributes

* vale + fixes

* fixes

* NR PR fixes 1

* DOCSP-45362: text search (#72)

* DOCSP-45362: text search

* wip

* vale

* MM PR fixes 1

* DOCSP-45436 Field Behaviors page (#68)

* DOCSP-45363: validation (#73)

* DOCSP-45363: validation

* keywords

* wip

* SA PR fixes 1

* DOCSP-44794 Field Types (#69)

* DOCSP-45357 Sharding Configuration (#76)

* DOCSP-42762: Indexes (#74)

* DOCSP-45367 Associations pt. 1 (#79)

* DOCSP-45368: Persistence Configuration (#77)

* DOCSP-45361: callbacks (#75)

* DOCSP-45361: callbacks

* wip

* wip

* wip

* NR PR fixes 1

* DOCSP-45364: CRUD pt 1 (#81)

* checkpoint

* checkpoint 2

* woohoo first pass

* indent

* Edits

* updates

* vale chekcs

* RR PR fixes 1

* fix code file

* code fixes

* RM PR fixes 1

---------

Co-authored-by: rustagir <[email protected]>

* DOCSP-45110: queries subsections (#80)

* DOCSP-45110: queries misc sections

* wip:

* vale

* MR PR fixes 1

* GM PR fixes 1

* DOCSP-46072 Associations part 2 (#82)

* DOCSP-46394: CRUD remaining sections (#83)

* DOCSP-46394: CRUD remaining sections

* vale fixes

* JS PR fixes 1

* DOCSP-46213: bump to rails 8 and remove old tuts (#84)

* DOCSP-45356: i&h + code doc (#86)

* DOCSP-45356: i&h + code doc

* remove contributing

* vale fixes

* link fix

* vale fixes + RM comment

* DOCSP-42770: release notes/whats new (#87)

* DOCSP-42770: release notes/whats new

* fixes

* fixes

* DOCSP-42773: api links (#88)

* DOCSP-42773: api links

* fix

* link fixes

* DOCSP-42772: compatibility (#90)

* DOCSP-42772: compatibility

* small fix

* small fix

* SA PR fixes 1

* delete files for old build system

* column width adjustment

* DOCSP-45366 Encryption (#89)

* DOCSP-42741: config pages (#91)

* DOCSP-42741: config

* wip

* wip

* some vale fixes

* RM PR fixes 1

* small fix

* DOCSP-46555: rails integration (#92)

* wip

* DOCSP-46555: rails integration

* RM PR fixes 1

* DOCSP-45359 External Resources (#94)

* add additional resources page

* edits

* feedback

* DOCSP-42743 Collection config (#95)

* DOCSP-42730: landing page (#96)

* DOCSP-42730: landing page

* MW PR fixes 1

* small fix

* small fix

* small fix

* DOCSP-46121: cleanup (#97)

* cleanup

* copy compat action

* redirects

* MW PR fixes 1

* add index section

* change vs in redirects

---------

Co-authored-by: Nora Reidy <[email protected]>
Co-authored-by: Jordan Smith <[email protected]>
Co-authored-by: Dmitry Rybakov <[email protected]>
Co-authored-by: Maya Raman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants