search

serlo / serlo.org-legacy

Legacy implementation of https://serlo.org/
https://serlo.org/
Apache License 2.0
18 stars 3 forks source link

server: Add /api/add-comment #572

Closed kulla closed 3 years ago

kulla commented 3 years ago

@inyono Ready for first feedback (since I have questions where I need you help :smile: ) My questions are:

~FormInterface is used as arguments~

The function https://github.com/serlo/serlo.org/blob/cf9c2df36a81a7ff23bb68d7f2e4d52a401b8854/packages/public/server/src/module/Discussion/src/DiscussionManager.php#L74-L99 uses FormInterface as an argument. I want to discuss what an appropriate type shall be for API mutations. In the current PR I have created a new comment with new Comment(), stored all necessary arguments and saved it with $this->getObjectManager()->persist() + ->flush(). Is this a good way to create a new uuid?

~User is not logged in~

Since the api does not forward the session to the legacy serlo.org the current user is not logged in but given via the userId parameter. However the function https://github.com/serlo/serlo.org/blob/cf9c2df36a81a7ff23bb68d7f2e4d52a401b8854/packages/public/server/src/module/Discussion/src/DiscussionManager.php#L88 checks against the currently logged in user. How shall we implement it.

What is missing:

inyono commented 3 years ago

Since the api does not forward the session to the legacy serlo.org the current user is not logged in but given via the userId parameter. However the function

https://github.com/serlo/serlo.org/blob/cf9c2df36a81a7ff23bb68d7f2e4d52a401b8854/packages/public/server/src/module/Discussion/src/DiscussionManager.php#L88

checks against the currently logged in user. How shall we implement it.

I'd create a new helper for that (you might want some different behavior there anyways since the current view helper also triggers a redirect to the login page when not authorized).

In the current PR I have created a new comment with new Comment(), stored all necessary arguments and saved it with $this->getObjectManager()->persist() + ->flush(). Is this a good way to create a new uuid?

IMHO we should still use the form since this already checks if everything is valid. So do something similar to addRevision, i.e. initialize the form, set the data and then check if everything is valid.

kulla commented 3 years ago

I'd create a new helper for that (you might want some different behavior there anyways since the current view helper also triggers a redirect to the login page when not authorized).

Can you point me to the helper where this is currently implemented?

inyono commented 3 years ago

Can you point me to the helper where this is currently implemented?

kulla commented 3 years ago

Looks okay, I'd check if the normal comment stuff still works (especially regarding the possible runtime error). Otherwise, this is probably okay for merge.

@inyono I just checked: Locally I could start and comment threads