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

Fix funders show country #343

Merged
merged 3 commits into from
Jun 18, 2024

Conversation

ptamarit
Copy link
Member

@ptamarit ptamarit commented Jun 4, 2024

❤️ Thank you for your contribution!

Description

🚶 Review path

  • The 1st commit includes the country_name in the API response for funders which have a country_name (recently updated ones)
  • The pull request vocabularies: pass on country_name in deserializeFunder invenio-app-rdm#2704 modifies deserializeFunder to pass include country_name in the deserialized form of funder items.
  • The 2nd commit simplifies the logic of deserializeFunderToDropdown by using optional chaining
  • The 3rd commit modifies deserializeFunderToDropdown to shows the country code for existing funders which don't have yet a country_name
  • Note that the modified deserializeFunder function is used lower down in the render function of CustomAwardForm.

🤷 Open questions

Places which might need to be modified to also include country_name:

Checklist

Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:

Frontend

Reminder

By using GitHub, you have already agreed to the GitHub’s Terms of Service including that:

  1. You license your contribution under the same terms as the current repository’s license.
  2. You agree that you have the right to license your contribution under the current repository’s license.

Comment on lines 31 to 35
if (funderItem.country_name) {
funderCountry = funderItem.country_name;
} else if (funderItem.country) {
funderCountry = funderItem.country;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be a one liner:

const funderCountry =  funderItem ?. country_name  ?? funderItem ?. country

You can check running this code (e.g. in a js fiddle):

let funderItem = {}
let funderCountry =  funderItem ?. country_name  ?? funderItem ?. country
console.log(funderCountry)
// Expected: undefined 

funderItem['country_name'] = 'Portugal'
funderCountry =  funderItem ?. country_name  ?? funderItem ?. country
console.log(funderCountry)
// Expected: 'Portugal'

delete funderItem['country_name']
funderItem['country'] = 'PT'
funderCountry =  funderItem ?. country_name  ?? funderItem ?. country
console.log(funderCountry)
// Expected: 'PT'

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, I also simplified the other fields. Optional chaining returns undefined while before we were defaulting to null, but it does not make a difference since the rest of the function is relying on truthy/falsy and both undefined and null behave the same way in this regard.

@ptamarit ptamarit force-pushed the fix-funders-show-country branch 2 times, most recently from 6410034 to da8ed2f Compare June 17, 2024 13:11
@slint slint merged commit 575ec16 into inveniosoftware:master Jun 18, 2024
16 checks passed
@ptamarit ptamarit deleted the fix-funders-show-country branch June 18, 2024 09:41
carlinmack added a commit to carlinmack/zenodo-rdm that referenced this pull request Jul 17, 2024
📁 invenio-administration (2.4.0 -> 2.4.1 ��)

    📦 release: v2.4.1
    global: remove dependency on invenio-vocabularies

    * Removes the dependency from invenio-vocabularies.
    * Delegates administration UI schema type definition to the mashmallow
      class.

📁 invenio-app-rdm (13.0.0b0.dev6 -> 13.0.0b0.dev8 )

    📦 release: v13.0.0b0.dev8
    previewer: gracefuly handle default preference
    📦 release: v13.0.0b0.dev7
    datastreams: add affiliations and update funders

📁 invenio-banners (3.0.1 -> 3.0.2 🐛)

    release: v3.0.2
    errors: fix validation errors not propagated to resource
    administration: set default category to info

    * Fixes inveniosoftware/invenio-banners#26 by setting default category value,
      which is a required field.

📁 invenio-rdm-records (11.3.1 -> 11.4.0 🌈)

    📦 release: v11.4.0
    affiliations: update defaults to ror v2

📁 invenio-rest (1.3.0 -> 1.3.1 🐛)

    release: v1.3.1
    tests; fix csrf tests
    csrf: improve validation

    * inveniosoftware/invenio-rest#132

📁 invenio-vocabularies (4.0.0 -> 4.1.1 🌈)

    📦 release: v4.1.1
    installation: use invenio-oaipmh-scythe from PyPI
    📦 release: v4.1.0
    readers: make OAI-PMH an optional extra
    schema: add administration UI attributes
    ror: fix duplicate acronymns and aliases
    affiliations: fix title search
    datastreams: have yaml writer output utf8
    datastreams: add configs for funders and affiliations
    affiliations: add datastreams
    datastreams: move ror transformer to common
    affiliations: add new fields
    vocabulary-types: services, resources, and administration UI (inveniosoftware/invenio-vocabularies#310)

    config: add OpenAIRE mapping for "Latvian Council of Science"
    tasks: fixed exception logging
    funding: update award label
    funders: fix country name display (inveniosoftware/invenio-vocabularies#343)
    first implementation of OAIPMHReader (inveniosoftware/invenio-vocabularies#329)

    * first implementation of OAIPMHReader

    * introduce a simple way to map OAI records to a dict without expecting a special metadata format.

    * fix installation requirements

    * fix tests

    * small fixes to make the tests run

    * add error handling

    * renamed oaipmh_scythe package

    * handle remarks/questions from review.

    * replaced access to a real OAI server with a mocking implementation.

    * Update invenio_vocabularies/datastreams/readers.py

    * Update tests/datastreams/test_datastreams.py

    * Moved reader tests to testreaders.py

    * add missing copyright in header

    ---------

    global: add "tags" field to all vocabularies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Funders ROR v2 not showing country in suggestion
3 participants