🇮🇳India @chandu7929

chandu7929 → created an issue.

chandu7929 → made their first commit to this issue’s fork.

chandu7929 → created an issue.

Hello @becw,

I have verified with "Metadata: Selection list" with field "Usage right" and don't see any issue even if I have asset tagged with label that contains ampersands, please the attach screenshot.

Also, could you please let us know how you are creating filter "Image Classification" and which filed being used for this filter?

This changes cause regression with following error:

TypeError: Drupal\acquia_dam\EmbedCodeFactory::getImageDimensions(): Argument #1 ($image_properties) must be of type array, null given, called in /home/ide/project/docroot/modules/contrib/acquia_dam/src/EmbedCodeFactory.php on line 179 in Drupal\acquia_dam\EmbedCodeFactory::getImageDimensions() (line 220 of modules/contrib/acquia_dam/src/EmbedCodeFactory.php).

Hell @becw,

With respect to your issue:

I have also found that filter values containing an ampersand are not handled correctly. Specifically, one of our DAM select lists contains values like "Places | Classroom & Education". When this is selected as a filter, I get an error on the filter select list, no results, and the error message "The submitted value Places | Classroom in the image_classification element is not allowed."

This issue can't be addressed in the module because the issue exists at API end.

chandu7929 → changed the visibility of the branch 3416582-fix-clear-filter to hidden.

Validated changed of MR197 and can confirm following items have been fixed:

1. Fixed: "Search", and multiple "Metadata: Selection list" filters now work in combination with each other
2. Fixed: The "Clear Filter" button now clears the "Search" filter and the "Metadata: Selection list" filters
3. Fixed: Selecting "Image" from one of our "Metadata: Selection list" filters still matches assets with the metadata value "Image" OR the metadata value "Image, purchased"
4. Fixed: Sorting does not work
5. Fixed: The "Clear Filter" button does not reset the exposed sort form elements -- and highlights them in red with the messages "The submitted value All in the Sort by element is not allowed." and "The submitted value All in the Order element is not allowed."

Hence requesting review.

chandu7929 → changed the visibility of the branch 3416582-multiple-values-for to hidden.

Thanks @serg.linkin, I have included the changes for jsonapi, also adding latest patch here.

chandu7929 → created an issue.

Created MR!36 with required changes to work with 5.0.6

Need to re-rolls patch as per 5.0.6 latest release.

Point #1 completed

The testing challenge I originally identified in #17, which @rajeshreeputra partially responded to in #19 and I then responded to in #21 still needs to be addressed. Just set up an additional GitLab CI job. Something like:

composer-canvas-core-version:
  _TARGET_CORE: 11.2.2

phpunit-canvas-core-version:
  needs:
    - composer-canvas-core-version
  variables:
    _TARGET_CORE: 11.2.2
  extends: .phpunit-base

#2 completed

Per my #14: the new test coverage this MR introduces is failing randomly (!) due to a bug in \Drupal\acquia_dam\Plugin\Field\FieldType\AssetItem: it should override the parent ::generateSampleValue(). (For similar reasons that \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::generateSampleValue() exists.)

chandu7929 → created an issue.

Eventually we will be removing common module dependency from all Acquia CMS ecosystem modules, hence this change is not needed.

chandu7929 → created an issue.

chandu7929 → created an issue.

Commenting this code allow site install to continue without failure. Need to find why its failing with 11.2.2

chandu7929 → created an issue.

chandu7929 → created an issue.

chandu7929 → created an issue.

chandu7929 → created an issue.

chandu7929 → created an issue.

chandu7929 → created an issue.

chandu7929 → created an issue.

chandu7929 → changed the visibility of the branch fix-alt-text-value to hidden.

chandu7929 → made their first commit to this issue’s fork.

I attempted to replicate the issue but was unable to do so. Could you please share the database log generated when you visit the media library page and encounter the error? While this error can occur if a user is not authenticated with Widen, I don't believe that is the case here.

chandu7929 → created an issue.

chandu7929 → created an issue.

chandu7929 → created an issue.

Fixed all failing tests, hence requesting review.

chandu7929 → changed the visibility of the branch 3460115-contribpath to active.

chandu7929 → changed the visibility of the branch 3460115-composerrewrite to hidden.

Hence RTBC.

Reviwed and verify MR 165 and it looks good to me.

@capysara

I'm not actually sure what I expect to happen if it's linked but doesn't download. How should the 2 format options be different

The only difference with download option is that you will get option in you document viewer to download it.

Without download

chandu7929 → made their first commit to this issue’s fork.

chandu7929 → made their first commit to this issue’s fork.

chandu7929 → changed the visibility of the branch 3519247-acquia-dam-all-media to hidden.

chandu7929 → made their first commit to this issue’s fork.

I have fixed the issue, hence moving in review.

chandu7929 → made their first commit to this issue’s fork.

chandu7929 → made their first commit to this issue’s fork.

There's separate issue for comment #3 →

Passing a blank string serves as a workaround to bypass the error. Ideally, we should update the method signature to accept null as an optional parameter. However, it's unclear what the implications of passing null might be, although all tests are currently passing with this change.

Hello @Japerry,

The optional parameter should be set to null rather than an empty string—unless there's a specific reason to use an empty string in the method. Using an empty string can cause an error when the function is called without the second parameter, as it expects a value of type string. This results in the following error:

"Argument #2 ($version_id) must be of type string",

which is inaccurate in this context.

Please refer to the error trace from the getAsset() call in media_acquiadam where version_id is not passed.

 [error]  Client error: `GET https://api.widencollective.com/v2/assets/0c772875-c010-47af-bf7e-2a2080587d50/versions?expand=asset_properties,embeds,file_properties,metadata,metadata_info,metadata_vocabulary,security,thumbnails` resulted in a `404 Not Found` response:
> {
>   "error": true,
>   "response_code": 404,
>   "error_message": "Asset with UUID 0c772875-c010-47af-bf7e-2a2080587d50 not  (truncated...)
>  [error]  TypeError: Drupal\acquia_dam\Client\AcquiaDamClient::getAsset(): Argument #2 ($version_id) must be of type string, null given, called in /var/www/html/web/modules/contrib/acquia_dam/src/Plugin/media/Source/Asset.php on line 234 in Drupal\acquia_dam\Client\AcquiaDamClient->getAsset() (line 170 of /var/www/html/web/modules/contrib/acquia_dam/src/Client/AcquiaDamClient.php) #0 /var/www/html/web/modules/contrib/acquia_dam/src/Plugin/media/Source/Asset.php(234): Drupal\acquia_dam\Client\AcquiaDamClient->getAsset()

Overall, the changes in MR!47 look good to me. I validated the behavior using both private://myfile.key and public://myfile.key paths, and was able to retrieve the key value when using this patch. However, without the patch, the value returns null. Therefore, marking this as RTBC.

chandu7929 → created an issue.

chandu7929 → created an issue.

Updated variable name as requested, hence moving in NR.

chandu7929 → made their first commit to this issue’s fork.

Incorporated requested changes, hence requesting review.

@japerry - Thank you for highlighting first point; we will take it into consideration for inclusion. However, regarding your second point,

Also, the media type labels are greyed out. The ones that come with Acquia DAM should be editable alongside the old Media: Acquia DAM types so you can get a full picture of the all the media types and their names.

What is the reasoning behind permitting updates to the media labels of the new media type provided by the Acquia DAM module, despite the apparent logic in offering label modification options for the older media types?

Since all requested changes are done, hence requesting review.

Updated MR with required changes, hence requesting review.

chandu7929 → made their first commit to this issue’s fork.

chandu7929 → made their first commit to this issue’s fork.

chandu7929 → changed the visibility of the branch 1.0.x to hidden.

chandu7929 → created an issue.

chandu7929 → made their first commit to this issue’s fork.

chandu7929 → changed the visibility of the branch 3501895-bulk-import to hidden.

chandu7929 → made their first commit to this issue’s fork.

Thanks for the feedback, @vishal! I've refactored the code and improved the logic based on your suggestions.

The requested changes have been incorporated. Hence NR

Updated MR with required changes hence requesting review.

chandu7929 → changed the visibility of the branch 3500484-media-view-display to hidden.

chandu7929 → changed the visibility of the branch 3500484-media-view-display to active.

Pulling back in NW since its not working as expected.

Verified this chagnes with Migration issue 🐛 Media view display handling not honored during migration Active , this completly make sence to update external_id in this queue working this is the most appropriate place, rather creating new process and make a whole seprate API call again to get assest and update it.

RTBC.

Verified the migration with latest changes, and I can confirm that its working as expected. Also I like the idead of using existing queue worker to update asset with external_id becuase we already making api call to get asset details.

Hence RTBC

"Warning: Attempt to read property "asset_id" on null in acquia_dam_media_delete() (line 696 of /var/www/html/upstreams/acquia_dam/acquia_dam.module).

acquia_dam_media_delete(Object)"

Not able to replicate this issue.

chandu7929 → made their first commit to this issue’s fork.

Created MR with required changes, hence requesting review.

chandu7929 → created an issue.

rajeshreeputra → credited chandu7929 → .

We are not going to take these block in recipe:

1. block.block.views_block__article_cards_recent_articles_block.yml
2. block.block.views_block__event_cards_past_events_block.yml
3. block.block.views_block__event_cards_upcoming_events_block.yml

Hence closing MR!44

Requesting review for MR 47 →

chandu7929 → made their first commit to this issue’s fork.

chandu7929 → made their first commit to this issue’s fork.

ankitv18 → credited chandu7929 → .

chandu7929 → made their first commit to this issue’s fork.

rajeshreeputra → credited chandu7929 → .

ankitv18 → credited chandu7929 → .

ankitv18 → credited chandu7929 → .

chandu7929 → made their first commit to this issue’s fork.

rajeshreeputra → credited chandu7929 → .