- Issue created by @vipin.mittal18
- First commit to issue fork.
- Merge request !61#3462318 – Fix asset versioning feature disabled results in a 403 error → (Merged) created by rajeshreeputra
- Status changed to Needs work
4 months ago 3:22pm 22 July 2024 - Status changed to Needs review
4 months ago 11:24am 23 July 2024 - Status changed to Postponed
4 months ago 1:18pm 29 July 2024 - 🇭🇺Hungary Balu Ertl Budapest 🇪🇺
By briefly running through the suggested changes, I foresee the potential of a code conflict. The self-explanatorily incorrect assumption of the inline comment
“// We can assume, that the latest version is the last item of the array.”
has been overridden once already in 🐛 Look for the "finalized" asset version not the latest modified Needs review .
Not counting this one, there are already 5–6 other open tickets already in Needs Review status. Most of them means a significant amount of code change. To avoid double-work I would suggest let's focus on getting them merged first, then get return here with a fresher
1.1.x
branch.Therefore I dare to mark this as Postponed for now but feel free to switch back if you feel it's urgent to work on.
- 🇭🇺Hungary Balu Ertl Budapest 🇪🇺
Now MR tested, I confirm that it solves the 403 error.
I also agree with renaming our DAM client's
getAssetVersionList()
method to a more genericgetAssetDetails()
which will better describe its functioning.Still I suggest to let's wait until 🐛 Look for the "finalized" asset version not the latest modified Needs review lands, then it will be easier to integrate these changes.
- 🇭🇺Hungary Balu Ertl Budapest 🇪🇺
Some thoughts on the Widen API v1/v2 usage
The Client\AcquiaDamClient::getAssetVersionList() in the current 1.0.x dev HEAD calls Widen API like this:
$response = $this->get("https://$domain/api/rest/asset/uuid/$id/assetversions");
(see API docs)Which is provided by the v1 API. This MR suggests replacing this method of our client with a different one, which would call API v2:
$response = $this->get("/v2/assets/$id");
(see API docs)Generally, I agree with that as it leans towards modernisation. However, now I checked our source code and at some point, we already rely on v2 API endpoints, for example
AcquiadamAuthService::sendAuthRequest()
(source).This mixed usage of Widen API causes some heavy feelings for me. I think we could – after some discussion – put some work into searching through the entire codebase and checking all external HTTP calls (heads up, not only the Client class!) to ensure that the next stable release of this module will rely on exclusively on Widen API v2.
- 🇭🇺Hungary Balu Ertl Budapest 🇪🇺
- MR updated to fresh 1.0.x target branch
- The Client class reworked to rely on API v2 for fetching asset versions (points of comment #8 🐛 Asset versioning feature disabled results in a 403 error Postponed are still valid)
- API response caching improved
- Test adapted to the code changes
Ready for review.
- First commit to issue fork.
-
japerry →
committed 53833aa3 on 1.0.x authored by
rajeshreeputra →
Issue #3462318: Asset versioning feature disabled results in a 403 error
-
japerry →
committed 53833aa3 on 1.0.x authored by
rajeshreeputra →
-
japerry →
committed 83001e52 on 1.1.x authored by
rajeshreeputra →
Issue #3462318: Asset versioning feature disabled results in a 403 error
-
japerry →
committed 83001e52 on 1.1.x authored by
rajeshreeputra →
-
japerry →
committed 83001e52 on acms-3567-asset-importer authored by
rajeshreeputra →
Issue #3462318: Asset versioning feature disabled results in a 403 error
-
japerry →
committed 83001e52 on acms-3567-asset-importer authored by
rajeshreeputra →
- Status changed to Fixed
about 2 months ago 7:06pm 5 October 2024 -
japerry →
committed 53833aa3 on acms-4080-alternate-preview authored by
rajeshreeputra →
Issue #3462318: Asset versioning feature disabled results in a 403 error
-
japerry →
committed 53833aa3 on acms-4080-alternate-preview authored by
rajeshreeputra →
Automatically closed - issue fixed for 2 weeks with no activity.