Skip to content

DOCSP-35948 write operations - insert documents #2804

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

Conversation

ccho-mongodb
Copy link
Contributor

@ccho-mongodb ccho-mongodb commented Mar 29, 2024

JIRA: https://jira.mongodb.org/browse/DOCSP-35948
Staging:

Note for reviewer:

This PR is for one of a few tickets related to completing the "Write Operations" page.

Other "Write Operations" page tickets:

Checklist

  • Add tests and ensure they pass
  • Add an entry to the CHANGELOG.md file
  • Update documentation for new features

Comment on lines +30 to +31
- Modify Documents
- Delete Documents
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to reviewer:
These sections will be completed in separate tickets and merged together when ready for publishing.

@ccho-mongodb ccho-mongodb changed the title DOCSP-35948 write operations DOCSP-35948 write operations - insert documents Mar 29, 2024
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.

Left some suggestions, but overall LGTM!

.. tip::

The ``$fillable`` attribute lets you use Laravel mass assignment for insert
operations. To learn more about mass assignment, see :ref:`laravel-model-mass-assignment`.
Copy link
Contributor

Choose a reason for hiding this comment

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

S: I'd clarify where this links to

Suggested change
operations. To learn more about mass assignment, see :ref:`laravel-model-mass-assignment`.
operations. To learn more about mass assignment, see the :ref:`laravel-model-mass-assignment` section in the Eloquent Model Class guide.

data types. To learn more, see `Attribute Casting <https://laravel.com/docs/{+laravel-docs-version+}/eloquent-mutators#attribute-casting>`__
in the Laravel documentation.

To learn more about Eloquent models in {+odm-short+} see the :ref:`laravel-eloquent-models`
Copy link
Contributor

Choose a reason for hiding this comment

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

S: add a comma here

Suggested change
To learn more about Eloquent models in {+odm-short+} see the :ref:`laravel-eloquent-models`
To learn more about Eloquent models in {+odm-short+}, see the :ref:`laravel-eloquent-models`

These examples show how to use the ``save()`` Eloquent method to insert an
instance of a ``Concert`` model as a MongoDB document.

When the ``save()`` method succeeds, the instance on which you called 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: since you mention a Concert model instance in the previous paragraph, I think the use of "instance" here is a little confusing. It would be helpful to clarify that this refers to a MongoDB instance:

Suggested change
When the ``save()`` method succeeds, the instance on which you called the
When the ``save()`` method succeeds, the MongoDB instance on which you called the

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! This is the model instance, so I'll clarify that.

This example code performs the following actions:

- Creates a new instance of the ``Concert`` model
- Assigns string values to the ``performer`` and ``venue`` field
Copy link
Contributor

Choose a reason for hiding this comment

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

S: [nit] I think field should be plural

Suggested change
- Assigns string values to the ``performer`` and ``venue`` field
- Assigns string values to the ``performer`` and ``venue`` fields

:end-before: end model insert one mass assign

To learn more about the Carbon PHP API extension, see the
`Carbon <https://github.com/briannesbitt/Carbon>`__ GitHub repository.
Copy link
Contributor

Choose a reason for hiding this comment

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

S: you can use :github: here

Suggested change
`Carbon <https://github.com/briannesbitt/Carbon>`__ GitHub repository.
:github:`Carbon <briannesbitt/Carbon>` GitHub repository.

fails, it throws an exception.

The example code saves multiple models in a single call by passing them as
an array to ``insert()`` method:
Copy link
Contributor

Choose a reason for hiding this comment

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

S: add "the"

Suggested change
an array to ``insert()`` method:
an array to the ``insert()`` method:

unique index on the ``_id`` field.

For more information on creating indexes on MongoDB collections by using the
Laravel schema builder, see the :ref:`laravel-eloquent-indexes` guide.
Copy link
Contributor

Choose a reason for hiding this comment

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

S: since this links to a section of the "Schema Builder" guide and not a separate "Indexes" guide, I think something like this would be more accurate:

Suggested change
Laravel schema builder, see the :ref:`laravel-eloquent-indexes` guide.
Laravel schema builder, see the :ref:`laravel-eloquent-indexes` section of the Schema Builder guide.

Comment on lines 21 to 22
Learn how to perform the following tasks by using the {+odm-long+} in the
following pages:
Copy link
Contributor

Choose a reason for hiding this comment

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

S: this sentence is a little repetitive; you could reword to something like:

Suggested change
Learn how to perform the following tasks by using the {+odm-long+} in the
following pages:
Learn how to use the {+odm-long+} to perform the following tasks:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I like this reworded version!

@ccho-mongodb ccho-mongodb marked this pull request as ready for review April 1, 2024 13:50
@ccho-mongodb ccho-mongodb requested a review from a team as a code owner April 1, 2024 13:50
@ccho-mongodb ccho-mongodb requested a review from GromNaN April 1, 2024 13:50
@ccho-mongodb ccho-mongodb changed the base branch from 4.1 to 4.1-base-docs-write-operations April 1, 2024 18:58
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the test with assertions.

Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

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

Small remark, but LGTM.


When the ``save()`` method succeeds, you can access the model instance on
which you called the method. If the operation fails, the model instance is
assigned a null value.
Copy link
Member

Choose a reason for hiding this comment

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

Don't you format the value as code?

Suggested change
assigned a null value.
assigned a ``null`` value.

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 meant to use "null" as an adjective, but I can change it to make it clear that it's the value (e.g. "assigned null").

@ccho-mongodb ccho-mongodb merged commit 4459b55 into mongodb:4.1-base-docs-write-operations Apr 4, 2024
@ccho-mongodb ccho-mongodb deleted the DOCSP-35948-write-operations branch April 4, 2024 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants