Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#9940 - fixed various issues with generated urls of authors for signposting #9941

Conversation

luddaniel
Copy link
Contributor

@luddaniel luddaniel commented Sep 20, 2023

What this PR does / why we need it:

  • Level 2 is no longer limited in authors number
  • Level 1 show better authors links, using type and id or using id as url if valid

Which issue(s) this PR closes:

Suggestions on how to test this:

I tested a lot of cases, I suggest the same approach :

  • check both level 1 and level 2 in every case scenario
  • add different kind of data on authors (empty identifier, good and bad url/id as identifier, with and without type)
  • cross the limit of 5 good possible links

Is there a release notes update needed for this change?:

I would say yes

Copy link
Member

@qqmyers qqmyers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - thanks for the fixes.

if (authorURL != null && !authorURL.isBlank()) {
// return empty if number of visible author more than max allowed
// >= since we're comparing before incrementing visibleAuthorCounter
if (visibleAuthorCounter >= maxAuthors) {
if (limit && visibleAuthorCounter >= maxAuthors) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch - we had a limit param and didn't use it!

@pdurbin pdurbin added the Size: 3 A percentage of a sprint. 2.1 hours. label Sep 20, 2023
@pdurbin pdurbin mentioned this pull request Oct 7, 2023
@pdurbin pdurbin added the Feature: Signposting Typed links in headers, etc. https://signposting.org label Oct 7, 2023
@cmbz
Copy link

cmbz commented Dec 13, 2023

2923/12/13
Added to 6.2 Proposals due to linkage with #9940

@cmbz cmbz modified the milestone: 6.2 Dec 13, 2023
@cmbz
Copy link

cmbz commented Jan 8, 2024

2024/01/08: Moved into sprint read during prioritization meeting.

@stevenwinship stevenwinship self-assigned this Jan 22, 2024
@stevenwinship stevenwinship merged commit 8dee2ea into IQSS:develop Jan 22, 2024
6 of 7 checks passed
@pdurbin pdurbin added this to the 6.2 milestone Jan 22, 2024
@luddaniel luddaniel deleted the 9940_fix_signposting_authors_links branch September 19, 2024 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature: Signposting Typed links in headers, etc. https://signposting.org Size: 3 A percentage of a sprint. 2.1 hours.
Projects
Status: 🚀 Done (Recherche Data Gouv)
Development

Successfully merging this pull request may close these issues.

Signposting Authors links issues
5 participants