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

#10189 Convert Queries to an Eloquent Model #10384

Merged
merged 6 commits into from
Sep 23, 2024
Merged

Conversation

kaitlinnewson
Copy link
Member

@kaitlinnewson kaitlinnewson commented Sep 10, 2024

No description provided.

@kaitlinnewson kaitlinnewson force-pushed the 10189-main branch 11 times, most recently from 45a02ff to 7ae8d92 Compare September 12, 2024 18:48
@kaitlinnewson kaitlinnewson force-pushed the 10189-main branch 6 times, most recently from 27a8cc4 to 411193d Compare September 13, 2024 17:10
@kaitlinnewson kaitlinnewson marked this pull request as ready for review September 13, 2024 17:48
Copy link
Contributor

@taslangraham taslangraham left a comment

Choose a reason for hiding this comment

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

Mostly looks good @kaitlinnewson. I just had a few questions and suggestions

classes/note/Repository.php Outdated Show resolved Hide resolved
classes/query/Query.php Outdated Show resolved Hide resolved
classes/query/QueryParticipant.php Outdated Show resolved Hide resolved
classes/query/QueryParticipant.php Outdated Show resolved Hide resolved
classes/query/Repository.php Outdated Show resolved Hide resolved
controllers/grid/queries/QueriesGridHandler.php Outdated Show resolved Hide resolved
controllers/grid/queries/form/QueryForm.php Outdated Show resolved Hide resolved
controllers/grid/queries/form/QueryForm.php Outdated Show resolved Hide resolved
templates/controllers/grid/queries/form/queryForm.tpl Outdated Show resolved Hide resolved
classes/query/Repository.php Outdated Show resolved Hide resolved
classes/query/Repository.php Outdated Show resolved Hide resolved
classes/query/Repository.php Outdated Show resolved Hide resolved
classes/query/QueryParticipant.php Show resolved Hide resolved
classes/query/QueryParticipant.php Show resolved Hide resolved
classes/query/QueryParticipant.php Show resolved Hide resolved
classes/query/Query.php Show resolved Hide resolved
classes/query/Query.php Show resolved Hide resolved
classes/query/Query.php Outdated Show resolved Hide resolved
@Hafsa-Naeem
Copy link
Contributor

@kaitlinnewson the PR looks good to me. 👍 I've added a few comments and suggestions.

Copy link
Member

@touhidurabir touhidurabir left a comment

Choose a reason for hiding this comment

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

@kaitlinnewson I have added few comments, mostly suggestions/questions . Take a look and feel free to address those if you think it make sense . Otherwise this look good to me .

classes/controllers/grid/GridHandler.php Outdated Show resolved Hide resolved
classes/query/Query.php Outdated Show resolved Hide resolved
classes/query/Repository.php Outdated Show resolved Hide resolved
@@ -58,14 +54,14 @@ public function getNotificationMessage(PKPRequest $request, Notification $notifi
'noteTitle' => Str::limit($headNote->title, 200),
]);
case Notification::NOTIFICATION_TYPE_QUERY_ACTIVITY:
$latestNote = Note::withAssoc(PKPApplication::ASSOC_TYPE_QUERY, $query->getAssocId())
Copy link
Member

Choose a reason for hiding this comment

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

Good catch here.

@asmecher asmecher merged commit b8bd6fd into pkp:main Sep 23, 2024
1 check passed
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.

5 participants