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

[draft!] New endpoints for entity translations and using lumen #3964

Merged
merged 48 commits into from
Mar 24, 2021

Conversation

rowasc
Copy link
Contributor

@rowasc rowasc commented May 6, 2020

Started this to comment with @tuxpiper as I go

@rowasc
Copy link
Contributor Author

rowasc commented May 6, 2020

@tuxpiper just pushing here to give you a chance to start looking into it and reviewing stuff. It's filled with @note and @todo stuff and things to figure out how to do better still but it's a start :) planning to start regression testing in a few days for the survey endpoints to see how it goes

@@ -39,7 +39,7 @@ public function isPrivate()
* Check if user can access deployment
* @return boolean
*/
public function canAccessDeployment(User $user)
public function canAccessDeployment($user)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed type hint here because of an issue with the user I'm sending now but making a note to reconsider since it'd be better just to send the correct user entity even if I have to cast it for the moment

-
id: 2
form_id: 1
label: "2nd step"
show_when_published: 1
task_is_internal_only: 0
type: "task"
priority: 2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok so this is actually important/good: we are being explicit about priorities to be able to test things more safely, especially the order of how we return things.

@@ -1,4 +1,4 @@
<?php
<?php /** @noinspection ALL */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

mmm this may be a phpstorm thing, TODO check

@@ -0,0 +1,165 @@
@acl
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is similar to our acl.feature with minimal changes and some improvement on what we check and of course hydrated entities

*/
public function translations()
{
return $this->morphMany('v4\Models\Translation', 'translatable');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Polymorphic relations are really useful here, because we can easily have translations for all entities

*
* @var array
*/
protected $appends = ['can_create'];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this makes it so that calculated properties like can_create make it cleanly into the json responses we send

v4/Models/Survey.php Outdated Show resolved Hide resolved
v4/Models/Survey.php Outdated Show resolved Hide resolved
* We check for relationship permissions here, to avoid hydrating anything that should not be hydrated.
* @return \Illuminate\Database\Eloquent\Relations\HasMany
*/
public function stages()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is actually kind of nice? Not loving the usage of v3 authorizers but not much to do there. The good part is that we just do it in the hydration and it works seamlessly with the ->stages relation.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 20.308% when pulling 48edfb6 on v4laravel into 2b432c0 on develop.

@tuxpiper tuxpiper merged commit f54d3a2 into develop Mar 24, 2021
@tuxpiper tuxpiper deleted the v4laravel branch March 24, 2021 20:35
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.

None yet

3 participants