Feature/UI retrieve reserve identifiers #63
Conversation
…er might be deprecated (codecheckers#48)
… Identifier Selects (codecheckers/issues/48)
…tes like users or submission codecheckers#48)
|
I now merged main into the UI branch @nuest |
nuest
left a comment
nuest
left a comment
There was a problem hiding this comment.
Re file structure and naming: the folder "classes/RetrieveReserveIdentifiers/" is not consistent with the code in it, there is also code about venue types and names; suggest to rename, maybe to "classes/Register" ?
See this file for examples on how to use localised strings in a .vue file: https://github.com/codecheckers/ojs-codecheck/blob/main/resources/js/Components/CodecheckMetadataForm.vue
api/v1/CodecheckApiHandler.php
Outdated
| $this->endpoints = [ | ||
| 'GET' => [ | ||
| [ | ||
| 'route' => 'getVenueData', |
There was a problem hiding this comment.
The route should not contain a verb - that is what the GET in the HTTP operation is for. Also, the "Data" is redundant, we always want to retrieve data via an API, or if we want something else (more specific), we can add sub-resources (e.g., venue/location or venue/image).
So please rename this route to venue.
The function name can stay the same, there the verb and the detail about the value of the returned result make sense.
| * | ||
| * @return void | ||
| */ | ||
| public function downloadFile(): void |
There was a problem hiding this comment.
Please clarify what this upload and download is used for. The manifest is primarily metadata and IIRC we are currently not implementing the case that a manifest file is actually uploaded to OJS. I understand this may be useful in the future where an author may be able to privately share manifest files this way, but first we handle the case of a check based on an external code/data repository.
I'm not saying we need to take these functions out, but they are probably dormant for now.
| 'roles' => $this->roles, | ||
| ], | ||
| [ | ||
| 'route' => 'yaml', |
| class CodecheckApiHandler | ||
| { | ||
| private JsonResponse $response; | ||
| private array $roles; |
There was a problem hiding this comment.
For my understanding: where do we assign the required roles? Or is this handled by OJS core?
api/v1/CodecheckApiHandler.php
Outdated
| ], | ||
| 'POST' => [ | ||
| [ | ||
| 'route' => 'reserveIdentifier', |
There was a problem hiding this comment.
Similarly as above, we avoid verbs in routes, so this can simply be POST identifier and then it is not a concern of the caller of the API what kind of "creation" this means, so in this case an external system is used to "reserve something", but it could also be a local random string generator. From the API perspective, we just say "call this endpoint when you want to create a new identifier".
| } | ||
|
|
||
| let codecheckApiUrl = pkp.context.apiBaseUrl.replace(/\/api\/v1\/?$/, ''); | ||
| codecheckApiUrl += '/codecheck_api'; |
There was a problem hiding this comment.
The same code is in lines 89 and 90 > define the API endpoint only once.
Also, in the regex for matching requests in the API handler https://github.com/codecheckers/ojs-codecheck/pull/63/changes#diff-b807d8c7cd429d2b42ac67336f8803632abdc50a1675f4109979b3850760e819 the API URL is only /codecheck/.
| codecheckApiUrl += '/codecheck_api'; | ||
|
|
||
| try { | ||
| const response = await fetch(`${codecheckApiUrl}/reserveIdentifier`, { |
There was a problem hiding this comment.
Rename endpoint as described in handler class.
| if (data.success) { | ||
| identifier.value = data.identifier; | ||
| issueUrl.value = data.issueUrl; | ||
| alert(`Added new issue with identifier: ${data.identifier}`); |
There was a problem hiding this comment.
This is fine for now, but please create an issue to add a nicer user notification than a plain JS alert.
| console.error('Error:', data.error); | ||
| } | ||
| } catch (error) { | ||
| console.error('Request failed:', error); |
There was a problem hiding this comment.
Same here: fine for now, but create issues to add proper user notifications.
| <textarea | ||
| :value="dataSoftwareAvail" | ||
| @input="onInput" | ||
| placeholder="Describe how your data and code are available..." |
Then I would call it |
I am aware on how to use localised strings, have I forgotten to do that soemwhere in the code? If so, then I am sorry and I will fix it |
2 participants
Closes: #47, closes: #48, closes: #64;
IMPORTANT:
This needs to be merged after #58, as it utilized the Vue refactor form the branch that #58 is based on. So any changes made through #58 should also be in this PR afterwards.
Added The features to retrieve and reserve identifiers via the UI. At this point in time, this is only in the submission form, but once the other PRs are merged, I will also implement this in our editorial workflow pages.
Click
Reserve Identifier:Get Alert once Identifier has been reserved and new issue was created inside: https://github.com/codecheckers/testing-dev-register/issues
When Identifier is reserved, the UI now looks like this:
When the identifier gets deleted, it now looks like this:
It it correctly displayed in the created YAML file:
