CGSP 727 scvcep in gpm#114
Conversation
as documented in the README, the production scheduler only runs at 0 and 10 minutes after the hour, so run at 6:05 would never happen.
| label="CIViC webpage hyperlink" | ||
| :vertical="true" | ||
| :required="true" | ||
| :max-length="10" |
There was a problem hiding this comment.
That's not going to be anywhere near long enough! Is this size-limited on the backend-side? If so, we should use that here, or like with the vcep case, just not limit.
Better yet, since we use the same model variable, can we just have one input-row and make an expression for the label?
There was a problem hiding this comment.
I think I addressed this with an additional commit by just making the label an expression.
| return Storage::disk('local')->download($path, $doc->filename); | ||
| } | ||
|
|
||
| public function downloadGroupFinalSpecification(Group $group, Document $document) |
There was a problem hiding this comment.
This method seems possibly unnecessary but also incomplete.
What is it intended to do that couldn't already be done with the existing show method? Is it just supposed to be a lookup against a specific document type for a group? If so, then the potential incompleteness about the method includes: i) There is no auth checking (like with the show method, but maybe on purpose since specs would be public), and ii) there's a $group argument that's never used. Seems like we'd at least want to validate that the group and document correspond to each other. It would make even more sense for the $group to be the only argument, and for the system to lookup the appropriate document based on the group.
There was a problem hiding this comment.
True, there are something have to be cleaned there.
- I have removed 1 out of 2 routes I set for this and update the plural to singular naming for
specification $groupwas always part of the plan. I must have forgotten to include it as part of the validation$groupcan be the only argument we need, but then again I've learned that final spec docs can be more than one, one of the reason I want to keep the doc UUID
There was a problem hiding this comment.
One thing I forgot to add is that, downloadGroupFinalSpecification initially created to be publicly accessible so when the url is displayed on the website, it's downloadable. But let me know if it's the right direction to what we intended to do here. I'm open for suggestion.
| @@ -0,0 +1,34 @@ | |||
| <?php | |||
There was a problem hiding this comment.
This needs to be done through the config/seeder approach (it looks like there's also something in config/next_actions.php that is relevant) rather than as a db migration.
There was a problem hiding this comment.
let me remove this, I must have added this before our last talk about db seeder using config
| if (this.applicationIsDirty() ) { | ||
| return this.$store.dispatch('groups/saveApplicationData', this.group) | ||
| .then(() => { | ||
| this.$emit('saved'); |
There was a problem hiding this comment.
"saved" event ends up getting emitted twice when save is called. The save() function calls saveUpdates(), which emits saved here, but then after completion of all promises in save(), emits saved again (line 83). I don't think we need the emit here on line 101, unless we can have a situation where saveUpdates is called outside of save.
There was a problem hiding this comment.
I'll remove this, as well as the one on VCEP and GCEP application file
bpow
force-pushed
the
CGSP-727-scvcep-in-gpm
branch
from
af646f8 to
37eacfb
Compare
| ->where('any', '^(?!(api|sanctum|impersonate|dev|documents|downloads|clockwork|profile-photos|storage)).*$'); | ||
|
|
||
| Route::get('/documents/{uuid?}', [DocumentController::class, 'show'])->middleware('auth:sanctum'); | ||
| Route::get('/downloads/groups/{group:uuid}/final-specifications', [DocumentController::class, 'downloadGroupFinalSpecifications'])->name('groups.final-specifications.download'); |
There was a problem hiding this comment.
There's at least one thing wrong here, maybe more... these two routes have the same name, so I think laravel will only register one. Furthermore, line 39 refers to downloadGroupFinalSpecifications (plural), whereas only the singular is defined (As in line 40). But also see my comment elsewhere for whether or not we need that as a separate endpoint, and if so what its functionality should be.
There was a problem hiding this comment.
Related to the previous comment about the final spec doc, I keep the 2nd route and remove the 1st one.
bpow
force-pushed
the
CGSP-727-scvcep-in-gpm
branch
from
81ae263 to
6530e26
Compare
- removed migration file, coverd by config - removed emit(saved) on all group types application file. CGSP-727
2 participants
making pull request for question/commenting purposes