feat: add project grants with dataset-level linkage#1619
feat: add project grants with dataset-level linkage#1619
Conversation
Introduce ProjectGrant identity + ProjectGrantVersion snapshots, extract from dataset version metadata, and link grants to datasets via project_grant_has_dataset. Add API endpoints/resources and wire extraction from DatasetVersionObserver.
| $table->string('projectGrantName'); | ||
| $table->string('leadResearcher')->nullable(); | ||
| $table->string('leadResearchInstitute')->nullable(); | ||
|
|
||
| $table->json('grantNumbers')->nullable(); | ||
|
|
||
| $table->date('projectGrantStartDate')->nullable(); | ||
| $table->date('projectGrantEndDate')->nullable(); | ||
| $table->text('projectGrantScope')->nullable(); |
There was a problem hiding this comment.
need to be like
project_grant_name
There was a problem hiding this comment.
@cocoon02 Per Dan's comment, all DB fields should maintain snake_case conventions.
|
|
||
| protected $table = 'project_grant_versions'; | ||
|
|
||
| protected $fillable = [ |
| 'path' => '/project_grants', | ||
| 'methodController' => 'ProjectGrantController@index', | ||
| 'namespaceController' => 'App\Http\Controllers\Api\V1', | ||
| 'middleware' => [], |
There was a problem hiding this comment.
Are these endpoints completely public? No sensitive information that needs guarding?
| use Illuminate\Http\Resources\Json\JsonResource; | ||
| use App\Models\ProjectGrant; | ||
|
|
||
| class ProjectGrantIndexResource extends JsonResource |
There was a problem hiding this comment.
In my opinion, I will add another folder v1 ... vx in Resources and add all the resources related to Controller/Vx. The same can be done with Services, except in the case where we will have general services.
| use App\Observers\ProjectGrantObserver; | ||
|
|
||
| #[ObservedBy([ProjectGrantObserver::class])] | ||
| class ProjectGrant extends Model |
There was a problem hiding this comment.
In my opinion, we need a command who need to fill with current data.
|
@cocoon02 One other thing. We do mandate that every PR comes with functional/unit tests as standard. For some of this, you may need some light demo data, which is acceptable until we action Dan's comment about seeding demo data in another PR. |
spco
left a comment
spco
left a comment
There was a problem hiding this comment.
A few comments to consider please
| use Illuminate\Database\Eloquent\Factories\HasFactory; | ||
| use Illuminate\Database\Eloquent\Relations\BelongsTo; | ||
|
|
||
| class ProjectGrantHasDataset extends Model |
There was a problem hiding this comment.
Here (and in ProjectGrantHasPublication/Tool) it would be more explicit and consistent with our treatment of DatasetVersion to rename to ProjectGrantVersionHasDataset etc please.
| // | ||
| } | ||
|
|
||
| private function reindexLinkedDatasets(int $projectGrantId): void |
There was a problem hiding this comment.
At least until ProjectGrant is a searchable/indexed field within Datasets, do we need these two Observers? Perhaps I don't see where this reindexing would be necessary if a ProjectGrant or ProjectGrantVersion were to change. Or is this here in expectation that we'll also make ProjectGrant a searchable/indexed field in Elastic and search-service?
4 participants
Introduce ProjectGrant identity + ProjectGrantVersion snapshots, extract from dataset version metadata, and link grants to datasets via project_grant_has_dataset. Add API endpoints/resources and wire extraction from DatasetVersionObserver.
Screenshots (if relevant)
Describe your changes
Issue ticket link
Environment / Configuration changes (if applicable)
Requires migrations being run?
If not using the pre-push hook. Confirm tests pass:
Checklist before requesting a review