Skip to content

DOCSP-35947: write operations modify documents #2808

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 Apr 1, 2024

JIRA: https://jira.mongodb.org/browse/DOCSP-35947

Note for the reviewer:

This contains some updates to the insert documents section (added new fields to the sample doc).
This is one of three tickets to document the update operations. The other two are:

Staging:
https://preview-mongodbcchomongodb.gatsbyjs.io/laravel/DOCSP-35947-write-operations-modify-documents/fundamentals/write-operations/#modify-documents

Checklist

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

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 good, requesting changes since I left a couple questions!

Comment on lines 190 to 191
When the ``save()`` method succeeds, the model instance on which you called the
method.
Copy link
Contributor

Choose a reason for hiding this comment

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

I: I think this sentence is incomplete. You could add something like this:

Suggested change
When the ``save()`` method succeeds, the model instance on which you called the
method.
When the ``save()`` method succeeds, the model instance on which you called the
method contains the updated values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, good catch! Will update.

it by calling the ``update()`` method or other helper methods.

The following example shows how to update a document by modifying an instance
of the model and calling its ``save()`` 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: not necessary, but I like the detailed description of the code example in lines 86-94. Could be helpful to introduce this code example with more specific info

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there specific info that you think might be helpful to add? I think the insert example adds type information which sets up the rest of the examples on the page, but could be repetitive and draw away from the focus of the rest of the examples.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking of specifying which fields / values you're updating (venue, ticketsSold), but agreed that this could be repetitive and I think it's fine as is!

Comment on lines 161 to 162
Eloquent lets you specify criteria to retrieve documents, perform updates on
the returned object collections, and save them.
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: this description is a little confusing - what is returning the "object collections"? Are you using "documents" and "object collections" interchangeably in this sentence?

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 replace the sentence with what I think might be less confusing.

I think I meant collection object. I can clarify further by using either the class name or adjective "Eloquent".

Comment on lines 195 to 196
The following example shows how to update a document by using fluent syntax to
retrieve the first match and apply updates to the matching document:
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 think you can condense this description a little; also, the other mentions of "fluent syntax" use "the", so I'd do the same here

Suggested change
The following example shows how to update a document by using fluent syntax to
retrieve the first match and apply updates to the matching document:
The following example shows how to use the fluent syntax to
retrieve and update the first matching document:

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 suggestion, thanks!


To perform an update on one or more documents, call the ``update()``
method on a collection of model objects. You can use the fluent syntax to
retrieve a collection of model objects that match your criteria and then
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 collection usually refers to a MongoDB collection, using that word here (as a synonym of "group", from my understanding) could be misleading. You could change to:

Suggested change
retrieve a collection of model objects that match your criteria and then
retrieve multiple model objects that match your criteria and then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will clarify the connection between MongoDB documents and a Laravel collection object.

retrieve a collection of model objects that match your criteria and then
pass your updates to the ``update()`` method.

The following example shows how to chain calls to retrieve a collection of
Copy link
Contributor

Choose a reason for hiding this comment

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

S: same comment regarding "collection" as above (unless you are referring to a whole MongoDB collection?)

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 for pointing this out. I'll make sure to establish the connection between "documents" and "Laravel collection objects" when necessary, and otherwise use only one of those terms in a paragraph.


In this section, you can learn how to modify documents in your MongoDB
collection from your Laravel application. Use update operations to modify
existing documents or to insert a document if no documents match the search
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] to avoid repeating "documents":

Suggested change
existing documents or to insert a document if no documents match the search
existing documents or to insert a document if none match the search

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!


If the operation fails, {+odm-short+} assigns the model instance a null value.

The following example shows how to update a document to retrieve and update
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 example demonstrates how to update documents by chaining methods together:

Suggested change
The following example shows how to update a document to retrieve and update
The following example shows how to chain methods to retrieve and update

it by calling the ``update()`` method or other helper methods.

The following example shows how to update a document by modifying an instance
of the model and calling its ``save()`` method:
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking of specifying which fields / values you're updating (venue, ticketsSold), but agreed that this could be repetitive and I think it's fine as is!

@ccho-mongodb ccho-mongodb marked this pull request as ready for review April 2, 2024 22:11
@ccho-mongodb ccho-mongodb requested a review from a team as a code owner April 2, 2024 22:11
@ccho-mongodb ccho-mongodb requested review from GromNaN and removed request for a team April 2, 2024 22:11
@ccho-mongodb ccho-mongodb merged commit 014c7f4 into mongodb:4.1-base-docs-write-operations Apr 8, 2024
@ccho-mongodb ccho-mongodb deleted the DOCSP-35947-write-operations-modify-documents branch April 8, 2024 13:44
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