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.

Sign up for GitHub

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

Jump to bottom

Feature/4007 remove the seal #2458

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Feature/4007 remove the seal #2458

wants to merge 7 commits into from
+145 −746

Conversation

RK206
Copy link
Contributor

@RK206 RK206 commented Mar 5, 2025 •
edited
Loading

Remove Links and metadata

Removed links and related metadata as requested. Run the migration script before testing

This PR...

  • has scripts to run
  • has migrations to run
  • adds new infrastructure
  • changes the CI pipeline
  • affects the public site
  • affects the editorial area
  • affects the publisher area
  • affects the monitoring

Developer Checklist

Developers should review and confirm each of these items before requesting review

  • Code meets acceptance criteria from issue
  • Unit tests are written and all pass
  • User Test Scripts (if required) are written and have been run through
  • Project's coding standards are met
    • No deprecated methods are used
    • No magic strings/numbers - all strings are in constants or messages files
    • ES queries are wrapped in a Query object rather than inlined in the code
    • Where possible our common library functions have been used (e.g. dates manipulated via dates)
    • Cleaned up commented out code, etc
    • Urls are constructed with url_for not hard-coded
  • Code documentation and related non-code documentation has all been updated
  • Migation has been created and tested
  • There is a recent merge from develop

Reviewer Checklist

Reviewers should review and confirm each of these items before approval
If there are multiple reviewers, this section should be duplicated for each reviewer

  • Code meets acceptance criteria from issue
  • Unit tests are written and all pass
  • User Test Scripts (if required) are written and have been run through
  • Project's coding standards are met
    • No deprecated methods are used
    • No magic strings/numbers - all strings are in constants or messages files
    • ES queries are wrapped in a Query object rather than inlined in the code
    • Where possible our common library functions have been used (e.g. dates manipulated via dates)
    • Cleaned up commented out code, etc
    • Urls are constructed with url_for not hard-coded
  • Code documentation and related non-code documentation has all been updated
  • Migation has been created and tested
  • There is a recent merge from develop

Testing

List user test scripts that need to be run

List any non-unit test scripts that need to be run by reviewers

Deployment

What deployment considerations are there? (delete any sections you don't need)

Configuration changes

What configuration changes are included in this PR, and do we need to set specific values for production

Scripts

What scripts need to be run from the PR (e.g. if this is a report generating feature), and when (once, regularly, etc).

Migrations

python portality/upgrade.py -u portality/migrate/4007_remove_the_seal/migrate.json

Monitoring

What additional monitoring is required of the application as a result of this feature

New Infrastructure

What new infrastructure does this PR require (e.g. new services that need to run on the back-end).

Continuous Integration

What CI changes are required for this

Sorry, something went wrong.

RK206 added 7 commits February 12, 2025 10:03
@RK206
Removed seal in some files
Loading
Loading status checks…
47817a0
@RK206
Merge branch 'develop' of https://github.com/DOAJ/doaj into feature/4…
864c53c 
…007_remove_the_seal
@RK206
Removed seal

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
Loading
Loading status checks…
871ba00
@RK206
Migration script to remove seal from indexed records
Loading
Loading status checks…
b5bcaa3
@RK206
Removed requested metadata
Loading
Loading status checks…
e9c1b8d
@RK206
Merge branch 'develop' of https://github.com/DOAJ/doaj into feature/4…
Loading
Loading status checks…
c38a198 
…007_remove_the_seal

# Conflicts:
#	doajtest/unit/application_processors/test_maned_journal_review.py
#	portality/forms/application_forms.py
#	portality/forms/application_processors.py
#	portality/static/js/doaj.fieldrender.edges.js
#	portality/templates-v2/public/layouts/toc.html
@RK206
merge changes from develop
Loading
Loading status checks…
925a90a
@RK206 RK206 requested a review from richard-jones March 5, 2025 15:11
richard-jones
richard-jones requested changes Mar 20, 2025
View reviewed changes
Copy link
Contributor

@richard-jones richard-jones left a comment

Choose a reason for hiding this comment

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

There is some confusion here over what is to be removed from the UI and what is to be removed from the data model. Some things have been removed from the data model when they should just be removed from the UI, and some things haven't been fully removed from the datamodel when they should be.

doajtest/example_files/incoming_application.json
@@ -11,7 +11,6 @@
},
"article": {
"license_display": ["Embed"],
"license_display_example_url": "http://licence.embedded",
Copy link
Contributor

Choose a reason for hiding this comment

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

These fields are not being removed from the data, only from the display, so they should still be allowed in the incoming application

doajtest/example_files/incoming_application.json
@@ -58,8 +57,7 @@
},
"pissn": "1234-5678",
"plagiarism": {
"detection": "True",
"url": "http://plagiarism.screening"
Copy link
Contributor

Choose a reason for hiding this comment

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

This field should still be accepted via the API, it is just being removed from display

doajtest/fixtures/v2/common.py
@@ -39,7 +35,6 @@
},
"article": {
"license_display": ["Embed"],
"license_display_example_url": "http://licence.embedded",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just being removed from display, not from the forms or the data

doajtest/fixtures/v2/common.py
@@ -89,8 +84,7 @@
},
"pissn": "1234-5678",
"plagiarism": {
"detection": True,
"url": "http://plagiarism.screening"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just being removed from display, not from the forms or the data

doajtest/fixtures/v2/common.py
@@ -155,11 +149,9 @@
"language": ["EN", "FR"],
"license_attributes" : ["BY", "NC"],
"license_display" : "y",
"license_display_example_url": "http://licence.embedded",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just being removed from display, not from the forms or the data

portality/scripts/sage_update.py
@@ -64,9 +63,6 @@
jbib.remove_urls("author_instructions")
jbib.add_url(author_instructions, "author_instructions")

if plagiarism_url is not None:
jbib.set_plagiarism_detection(plagiarism_url)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just being removed from display, not from the forms or the data

portality/static/js/suggestions_and_journals.js
@@ -145,7 +145,6 @@ jQuery(document).ready(function($) {

toggle_optional_field('waiver_policy', ['#waiver_policy_url']);
toggle_optional_field('download_statistics', ['#download_statistics_url']);
toggle_optional_field('plagiarism_screening', ['#plagiarism_screening_url']);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just being removed from display, not from the forms or the data

portality/models/v2/shared_structs.py
@@ -58,7 +58,6 @@
},
"article" : {
"fields" : {
"license_display_example_url" : {"coerce" : "url", "set__allow_coerce_failure" : True},
"orcid" : {"coerce" : "bool"},
"i4oc_open_citations" : {"coerce" : "bool"}
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be removed from the model

portality/crosswalks/journal_questions.py
Copy link
Contributor

Choose a reason for hiding this comment

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

The orcid and open_citations data also need to be removed from this crosswalk

portality/crosswalks/application_form.py
Copy link
Contributor

Choose a reason for hiding this comment

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

orcids and open citations need to be removed from this crosswalk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richard-jones richard-jones

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@RK206 @richard-jones