-
Notifications
You must be signed in to change notification settings - Fork 451
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
Add a "country" field to journal setup #6099
Comments
where should the new country field appear:
should it be available in the api summary? is this also relevant for the OMP? |
@asmecher, do you maybe know more about this requirement -- where/how is this information used, an use case this is for, is this a required field,... ? |
Hi @bozana! Actually, I think this is no longer a requirement -- I had forgotten we have an entry here. It's possible to identify the country using the ISSN with a high degree of reliability. See: https://github.com/asmecher/beacon/blob/master/updateBeacon.php#L155 So I'll close the issue for now... |
Because ISSNs aren't mandatory, we will still need to have a mandatory Country field. I'd suggest including it in the Journal Identity section of the Masthead. |
Thanks, @jonasraoni! Back in #4946 we added a field in the Citation Style Language plugin for publisher place; I wonder if it would be possible to move that setting alongside this one, call it City, and use both the city and country fields to inform the CSL plugin. I would feel a little better about this field, especially making it mandatory, if it were more than single-use. @stranack and @NateWr, I'm wondering what your opinions are for making the field required. As it stands, existing users who upgrade to a release with this change in place will be required to choose a country before they can save the form. I think that's the only way to get good data coverage, but it'll be annoying. |
@asmecher Ok, I can take a look at the citation plugin. A migration/cleanup script will be needed there 😁 About the required field, another option to decrease the annoyance (considering there might be users with several contexts to update), is to offer the user to just confirm a default country (we can take it from the ISSN/domain/IP) using a "post-upgrade" interface + notification. |
I like this idea, but is there any risk of muddying the data? My understanding is that the data we're adding now is meant to be the journal's location. But I think the CSL property is meant to be the city of the publisher. There's a good chance these won't necessarily be the same... |
Hmm, yes, let's leave this change for now, @jonasraoni -- I think there's room to gather these somehow but not the way I proposed earlier.
Hmm, that's possible... What about adding the Country field to the settings wizard, and making it required there, so that all new journals will require an entry -- but making the field optional in the regular settings form so it doesn't force a choice for existing journals? |
It sounds like we are crossing into some of the territory described in #5980. That issue suggests an overall UI that brings together status tracking for lots of services. But maybe in the near-term, since we are focusing on the Beacon, we can think of what a "Beacon Status" UI might look like. I'm thinking something like the examples described in #6042 -- though much more simplified. |
@NateWr, it looks like @jonasraoni has updated the PRs; would you mind taking another look (especially WRT your comments above about #5980 / #6042)? @jonasraoni, don't forget to drop a note on the issue when you're ready for another round of review -- otherwise we're not sure when you're ready. |
@asmecher @stranack @jonasraoni I thought that we had settled on making this field not required? There are a couple of things we need to address about usability, not code:
|
From @stranack:
|
Also, @stranack's opinion is that the field should be required. |
So correct me if I'm wrong, but I think the remaining changes are:
|
… and required on the masthead form.
…masthead data into pkp-lib.
…xt-country-field #6099 Added a required country field to the context form.
Thanks @jonasraoni, all merged! In the future, when moving a locale key from OJS to pkp-lib, go ahead and move it in all languages. I went ahead and did this in a8989ad and pkp/ojs@dd8afd3. Hopefully when #6328 is solved we won't need to do this anymore! 😂 |
Heads-up that pkp/ops#130 and pkp/omp#941 didn't get merged, which broke the tests when submodules updated in OMP and OPS -- I've cherry-picked in the relevant changes. |
The OPS tests were breaking on a check on the country field being required -- I've committed a change that removes the check to get tests working again, but can you have a look, @jonasraoni? pkp/ops@490bb83 |
Add a country field to journal setup.
The text was updated successfully, but these errors were encountered: