Include xml in frontend attribution options #4499

madewithkode · 2024-06-17T17:35:47Z

Fixes

Description

This PR seeks to add an extra Dublin-Core-Conforming XML attribution option on the frontend, allowing users to be able to credit creators in XML.

Testing Instructions

This change can be observed by visiting the detail page of any media item on Openverse, under the "Credit the creator" section, you should see an extra XML option.

Checklist

My pull request has a descriptive title (not a vague title likeUpdate index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.
I ran the DAG documentation generator (just catalog/generate-docs for catalog
PRs) or the media properties generator (just catalog/generate-docs media-props
for the catalog or just api/generate-docs for the API) where applicable.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

frontend/src/components/VMediaInfo/VCopyLicense.vue

frontend/src/utils/attribution-html.ts

madewithkode · 2024-06-18T12:50:35Z

Hi @sarayourfriend your suggestions have been resolved. However, I see that playwright visual regression tests continue to fail, I'm not exactly sure how to go about updating them, I've looked at the docs referenced by the bot on playwright but it's not exactly clear how to come up with the new comparison screenshots e.t.c. I'd appreciate if I could get a step by step instruction on how to tackle this.

dhruvkb · 2024-06-18T14:31:17Z

@madewithkode here's a hack I've used in the past which can avoid the need to run the VR tests locally.

Go to the workflow summary page https://github.com/WordPress/openverse/actions/runs/9564951682 and look for the artifacts named *_test_results. Download them. Then extract the files in the artifact ZIP that you downloaded and look at the previous and new versions of the file and the diff. If the new version of the file looks correct, replace the file in the repo with the files from the artifact ZIP.

sarayourfriend · 2024-06-18T23:01:32Z

@madewithkode here are instructions for updating snapshots in tests: https://docs.openverse.org/frontend/guides/test.html#updating-snapshots. Run ./ov just frontend/run test:playwright visual-regression -u (leave off ./ov if you haven't set that up yet).

We should probably combine that document with the one the bot links in its comment. I've long struggled to know what the precise difference was between reference and guides in our docs, and it's often muddied things up a bit like it has here. Sorry for the confusion there.

Here's an issue to fix that problem with the docs: #4514

madewithkode · 2024-06-19T08:34:15Z

Thanks @dhruvkb for the quick hack, going to try it out and hope it works, as running playwright locally appears to be a bit resource-intensive for me. @sarayourfriend Thanks for pointing to the actual documentation too and I totally agree that the information shared by the bot can really be improved to include the actual steps needed for updating snapshots. I might as well take up #4514 to fix this after this one is done.

zackkrida

This LGTM, thank you @madewithkode!

I do want to flag a concern for our designer @fcoveram: this change makes some of the tab labels occupy two lines on mobile instead of one. Here is an example:

https://github.com/WordPress/openverse/blob/4430_include_xml_in_frontend_attribution_options/frontend/test/playwright/visual-regression/pages/pages-single-result.spec.ts-snapshots/audio-ltr-from-search-results-with-additional-search-views-xs-linux.png

Do you think that looks okay, or should we change the design a bit, perhaps reducing the font size slightly on mobile or some other change? It doesn't have to happen in this PR in my opinion, but is something we could fix later.

dhruvkb

I have a bit of a small suggestion which will, unfortunately, necessitate new snapshots for Arabic. Mostly everything else looks fine and I would've approved the PR except for this change.

frontend/test/locales/ar.json

dhruvkb

There are a number of snapshots in the PR that are the same before and after, like the audio search and error pages, because they do not have the attribution box. The snapshot tests will likely still pass if these files are dropped from the PR.

I have a minor nit that seems okay to ignore.

dhruvkb · 2024-06-19T22:22:44Z

frontend/src/utils/attribution-html.ts

+  if (isXml) {
+    isPlaintext = true
+  }


This might just be me, but this conflation of XML with plain text causes a bit of mental gymnastics in the logic below. I feel like using isXml and isPlaintext separately would've been clearer.

Can we bypass the plaintext handling if we just use mediaItem.attribution, too, by the way?

That could work, and it would cut down on the code the frontend duplicates from the API, but I'm not sure about localisation though. The frontend is capable of localising the attribution text in a way the API is not.

Ah true 😕 In that case, should we undo the change from this comment before merging? #4499 (comment)

I don't a simple revert will be adequate. I'm personally not well versed with the i18n situation of the attribution box and feel like looking into what currently happens (and I expect should happen) will need a bit of time. Part of that's because I'm not entirely sure if XML, which is more geared towards machines than people, should even be translated.

I was only referring to the rich and plain text versions, which should be translated imo and using mediaItem.attribution is definitely not enough for the latter of those two types of attribution.

@zackkrida suggested using just the license URL, and the DC standard recommendation matches that too:

Typically, rights information includes a statement about various property rights associated with the resource, including intellectual property rights. Recommended practice is to refer to a rights statement with a URI. If this is not possible or feasible, a literal value (name, label, or short text) may be provided.

Then we don't need to worry about i18n at all for this (ideal!).

@madewithkode would you be able to update this to use the license URL instead of the attribution text?

@dhruvkb this would mean just switching <dc:rights>${mediaItem.attribution}</dc:rights> in the XML attribution snippet to reference the CC license URL as in <dc:rights>${mediaItem.license_url}</dc:rights> right?

This has been updated.

dhruvkb

After reversing the snapshots that had not been changed, I feel like this PR is good to go.

Thanks for the contribution @madewithkode! This is a very valuable feature and your implementation has been very clear and thorough.

fcoveram · 2024-06-20T15:23:09Z

I do want to flag a concern for our designer @fcoveram: this change makes some of the tab labels occupy two lines on mobile instead of one. Here is an example:

https://github.com/WordPress/openverse/blob/4430_include_xml_in_frontend_attribution_options/frontend/test/playwright/visual-regression/pages/pages-single-result.spec.ts-snapshots/audio-ltr-from-search-results-with-additional-search-views-xs-linux.png

Do you think that looks okay, or should we change the design a bit, perhaps reducing the font size slightly on mobile or some other change? It doesn't have to happen in this PR in my opinion, but is something we could fix later.

I would follow the common practice of replacing with a dropdown element. Tailwind addresses this pretty well.

zackkrida · 2024-06-20T15:31:27Z

That's a great (and simple) idea, @fcoveram. I'll make a follow up issue for that!

sarayourfriend · 2024-06-25T05:21:56Z

@madewithkode I pushed a change to revert some unneeded snapshot updates as well as to fix a bug I realised with unescaped elements in the title and other work meta data. Some works would have had broken XML due to unescaped XML (or XML-like) text. Take this work, for example, where the creator's included their email in their username: https://openverse.org/image/7be0e5cb-6308-4c11-b397-38baf2d0a4e7

The XML generated without escaping it (which is caused by setting isPlaintext = true), is as follows:

<attribution xmlns:dc="http://purl.org/dc/elements/1.1/">
	<dc:creator>Photographer: Richard Ling <wikipedia@rling.com></dc:creator>
	<dc:title>Grey Nurse Shark at Fish Rock Cave, NSW</dc:title>
	<dc:rights>https://creativecommons.org/licenses/by-sa/3.0/</dc:rights>
	<dc:identifier>https://commons.wikimedia.org/w/index.php?curid=130380</dc:identifier>
	<dc:type>StillImage</dc:type>
</attribution>

That won't pass XML validation. Fix was easy though, just removed the lines setting isPlaintext = true to enable the escaping (though really we should separate these concerns into more explicit variables, I think!).

I'm also rebasing and re-running the snapshot tests to fix the conflicts for you.

sarayourfriend

LGTM!

madewithkode · 2024-06-25T09:03:10Z

@madewithkode I pushed a change to revert some unneeded snapshot updates as well as to fix a bug I realised with unescaped elements in the title and other work meta data. Some works would have had broken XML due to unescaped XML (or XML-like) text. Take this work, for example, where the creator's included their email in their username: https://openverse.org/image/7be0e5cb-6308-4c11-b397-38baf2d0a4e7

The XML generated without escaping it (which is caused by setting isPlaintext = true), is as follows:
<attribution xmlns:dc="http://purl.org/dc/elements/1.1/">
	<dc:creator>Photographer: Richard Ling <wikipedia@rling.com></dc:creator>
	<dc:title>Grey Nurse Shark at Fish Rock Cave, NSW</dc:title>
	<dc:rights>https://creativecommons.org/licenses/by-sa/3.0/</dc:rights>
	<dc:identifier>https://commons.wikimedia.org/w/index.php?curid=130380</dc:identifier>
	<dc:type>StillImage</dc:type>
</attribution>
That won't pass XML validation. Fix was easy though, just removed the lines setting isPlaintext = true to enable the escaping (though really we should separate these concerns into more explicit variables, I think!).

I'm also rebasing and re-running the snapshot tests to fix the conflicts for you.

Thank you for the changes and for spotting this edge case @sarayourfriend

sarayourfriend · 2024-06-26T04:15:05Z

Thanks for your hard work, @madewithkode 🙏

madewithkode requested a review from a team as a code owner June 17, 2024 17:35

madewithkode requested review from zackkrida and obulat June 17, 2024 17:35

sarayourfriend reviewed Jun 17, 2024

View reviewed changes

frontend/src/components/VMediaInfo/VCopyLicense.vue Outdated Show resolved Hide resolved

frontend/src/utils/attribution-html.ts Outdated Show resolved Hide resolved

madewithkode force-pushed the 4430_include_xml_in_frontend_attribution_options branch from b446fab to a9c2b40 Compare June 18, 2024 12:25

zackkrida mentioned this pull request Jun 18, 2024

Attribution: XML/RDF/Turtle please. #4430

Closed

sarayourfriend mentioned this pull request Jun 18, 2024

Combine frontend testing documentation pages #4514

Closed

madewithkode force-pushed the 4430_include_xml_in_frontend_attribution_options branch 4 times, most recently from 7306fda to fbfa4c3 Compare June 19, 2024 13:46

zackkrida approved these changes Jun 19, 2024

View reviewed changes

dhruvkb requested changes Jun 19, 2024

View reviewed changes

frontend/test/locales/ar.json Outdated Show resolved Hide resolved

madewithkode force-pushed the 4430_include_xml_in_frontend_attribution_options branch 3 times, most recently from 0238d00 to d2c080e Compare June 19, 2024 17:40

dhruvkb reviewed Jun 19, 2024

View reviewed changes

dhruvkb approved these changes Jun 20, 2024

View reviewed changes

sarayourfriend force-pushed the 4430_include_xml_in_frontend_attribution_options branch from 8d1062c to 2c5e141 Compare June 25, 2024 05:14

madewithkode and others added 5 commits June 25, 2024 15:22

include xml in frontend attribution options

d746f92

Revert redundant snapshot changes

a24bb78

Use license url for dc:rights

25ad120

Ensure elements embedded in XML text are escaped

d7c2082

Update snapshots

ab61eb1

sarayourfriend force-pushed the 4430_include_xml_in_frontend_attribution_options branch from 2c5e141 to ab61eb1 Compare June 25, 2024 05:27

sarayourfriend approved these changes Jun 25, 2024

View reviewed changes

sarayourfriend merged commit 049e7f8 into main Jun 26, 2024
45 checks passed

sarayourfriend deleted the 4430_include_xml_in_frontend_attribution_options branch June 26, 2024 04:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include xml in frontend attribution options #4499

Include xml in frontend attribution options #4499

madewithkode commented Jun 17, 2024

madewithkode commented Jun 18, 2024

dhruvkb commented Jun 18, 2024

sarayourfriend commented Jun 18, 2024 •

edited

Loading

madewithkode commented Jun 19, 2024 •

edited

Loading

zackkrida left a comment

dhruvkb left a comment

dhruvkb left a comment

dhruvkb Jun 19, 2024

sarayourfriend Jun 19, 2024

dhruvkb Jun 20, 2024

sarayourfriend Jun 20, 2024 •

edited by madewithkode

Loading

dhruvkb Jun 20, 2024 •

edited

Loading

sarayourfriend Jun 20, 2024

dhruvkb Jun 21, 2024

madewithkode Jun 21, 2024

madewithkode Jun 21, 2024

dhruvkb left a comment

fcoveram commented Jun 20, 2024

zackkrida commented Jun 20, 2024

sarayourfriend commented Jun 25, 2024

sarayourfriend left a comment

madewithkode commented Jun 25, 2024

sarayourfriend commented Jun 26, 2024

Include xml in frontend attribution options #4499

Include xml in frontend attribution options #4499

Conversation

madewithkode commented Jun 17, 2024

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

madewithkode commented Jun 18, 2024

dhruvkb commented Jun 18, 2024

sarayourfriend commented Jun 18, 2024 • edited Loading

madewithkode commented Jun 19, 2024 • edited Loading

zackkrida left a comment

Choose a reason for hiding this comment

dhruvkb left a comment

Choose a reason for hiding this comment

dhruvkb left a comment

Choose a reason for hiding this comment

dhruvkb Jun 19, 2024

Choose a reason for hiding this comment

sarayourfriend Jun 19, 2024

Choose a reason for hiding this comment

dhruvkb Jun 20, 2024

Choose a reason for hiding this comment

sarayourfriend Jun 20, 2024 • edited by madewithkode Loading

Choose a reason for hiding this comment

dhruvkb Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

sarayourfriend Jun 20, 2024

Choose a reason for hiding this comment

dhruvkb Jun 21, 2024

Choose a reason for hiding this comment

madewithkode Jun 21, 2024

Choose a reason for hiding this comment

madewithkode Jun 21, 2024

Choose a reason for hiding this comment

dhruvkb left a comment

Choose a reason for hiding this comment

fcoveram commented Jun 20, 2024

zackkrida commented Jun 20, 2024

sarayourfriend commented Jun 25, 2024

sarayourfriend left a comment

Choose a reason for hiding this comment

madewithkode commented Jun 25, 2024

sarayourfriend commented Jun 26, 2024

sarayourfriend commented Jun 18, 2024 •

edited

Loading

madewithkode commented Jun 19, 2024 •

edited

Loading

sarayourfriend Jun 20, 2024 •

edited by madewithkode

Loading

dhruvkb Jun 20, 2024 •

edited

Loading