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

DOCSP-35970: Find one usage example #2768

Merged
merged 32 commits into from
Apr 4, 2024

Conversation

norareidy
Copy link
Contributor

@norareidy norareidy commented Mar 12, 2024

JIRA - https://jira.mongodb.org/browse/DOCSP-35970
Staging - https://preview-mongodbnorareidy.gatsbyjs.io/laravel/DOCSP-35970-staging/usage-examples/findOne/

Checklist

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

Copy link
Contributor

@rustagir rustagir left a comment

Choose a reason for hiding this comment

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

a few small things, let me know if you have any questions about my comments!

Comment on lines 13 to 14
:term:`natural order` in the database or according to the sort order specified by the
``orderBy()`` method.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
:term:`natural order` in the database or according to the sort order specified by the
``orderBy()`` method.
:term:`natural order` in the database or according to the sort order that you can specify by using the
``orderBy()`` method.

docs/usage-examples/findOne.txt Show resolved Hide resolved
docs/usage-examples/findOne.txt Outdated Show resolved Hide resolved
rustagir
rustagir previously approved these changes Mar 15, 2024
Copy link
Contributor

@rustagir rustagir 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 non-blocking comments, Can leave another review if you prefer though!

You can retrieve a single document from a collection by chaining the ``where()`` and
``first()`` methods on an Eloquent model or a query builder.

Pass a query filter to the ``where()`` method and call the ``first()`` method to return
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Pass a query filter to the ``where()`` method and call the ``first()`` method to return
Pass a query filter to the ``where()`` method and then call the ``first()`` method to return

Comment on lines 27 to 28
- Defines a query filter that matches documents in which the value of the ``directors`` field
is ``"Rob Reiner"``
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 the directors field is an array-valued field

Suggested change
- Defines a query filter that matches documents in which the value of the ``directors`` field
is ``"Rob Reiner"``
- Defines a query filter that matches documents in which the value of the ``directors`` field
includes ``"Rob Reiner"``

Comment on lines 46 to 58
class MovieController
{
public function show()
{
$movie = Movie::where('directors', 'Rob Reiner')
->orderBy('_id')
->first();

return view('browse_movies', [
'movie' => $movie
]);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

S: move into files and include (applies to all code examples)

docs/usage-examples/findOne.txt Outdated Show resolved Hide resolved
@GromNaN GromNaN added the docs label Mar 25, 2024
@GromNaN GromNaN changed the base branch from 4.1 to docs-rewrite March 25, 2024 16:26
@GromNaN GromNaN changed the base branch from docs-rewrite to 4.1 March 25, 2024 17:00
@norareidy norareidy marked this pull request as ready for review March 26, 2024 17:06
@norareidy norareidy requested a review from a team as a code owner March 26, 2024 17:06
@norareidy norareidy requested a review from GromNaN March 26, 2024 17:06
docs/includes/usage-examples/FindOneTest.php Show resolved Hide resolved
->orderBy('_id')
->first();

print_r($movie->toJson());
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want to display the results with print_r? That is not very compatible with PHPUnit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you prefer to display the results? Originally I used a view, but I changed it to print_r for simplicity

Copy link
Member

Choose a reason for hiding this comment

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

Since it's a simple string, I modified to use echo.

@caitlindavey caitlindavey requested a review from GromNaN April 1, 2024 18:47
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. With a simplification of the model. Maybe sharing the same model class for all the find examples would be a good idea.

docs/includes/usage-examples/Movie.php Outdated Show resolved Hide resolved
->orderBy('_id')
->first();

print_r($movie->toJson());
Copy link
Member

Choose a reason for hiding this comment

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

Since it's a simple string, I modified to use echo.


$this->assertInstanceOf(Movie::class, $movie);

$this->expectOutputRegex('^{"_id":"[a-z0-9]{24}","title":"The Shawshank Redemption","directors":["Frank Darabont","Rob Reiner"]}$');
Copy link
Member

Choose a reason for hiding this comment

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

Adding assertions on output.

@GromNaN GromNaN force-pushed the DOCSP-35970-find-one-usage branch from aad93f7 to bf98e98 Compare April 2, 2024 21:56
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

@norareidy norareidy merged commit e77a474 into mongodb:4.1 Apr 4, 2024
15 checks passed
@norareidy norareidy deleted the DOCSP-35970-find-one-usage branch April 4, 2024 14:17
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