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

api: test existence of pid's #853

Merged
merged 1 commit into from
May 13, 2020
Merged

api: test existence of pid's #853

merged 1 commit into from
May 13, 2020

Conversation

rerowep
Copy link
Contributor

@rerowep rerowep commented Mar 17, 2020 •
edited
Loading

  • Adds functionality to test pids for iexistence.
  • Test during acq_accounts creation and update existence of pids.
  • Test during acq_invoices cration and update existence of pids.
  • Test during acq_order_lines creation and update existence of pids.
  • Test during acq_orders creation and update existence of pids.
  • Test during holdings creation and update existence of pids.
  • Test during items creation and update existence of pids.
  • Test during loans creation and update existence of pids.
  • Test during patron_types creation und update existence of pids.

Co-Authored-by: Peter Weber [email protected]

Why are you opening this PR?

How to test?

Code review check list

  • Commit message template compliance.
  • Commit message without typos.
  • File names.
  • Functions names.
  • Functions docstrings.
  • Unnecessary commited files?
  • Extracted translations?

@rerowep rerowep added the WIP label Mar 17, 2020
@rerowep rerowep self-assigned this Mar 17, 2020
@rerowep rerowep force-pushed the wep-#1251-records-links-to-organisation branch 16 times, most recently from ddfda05 to 7f89fe8 Compare March 22, 2020 15:53
@rerowep rerowep removed the WIP label Mar 23, 2020
@rerowep rerowep force-pushed the wep-#1251-records-links-to-organisation branch 3 times, most recently from 78a1c5d to a9e936a Compare March 30, 2020 06:42
BadrAly
BadrAly suggested changes Mar 31, 2020
View reviewed changes
Copy link

@BadrAly BadrAly left a comment

Choose a reason for hiding this comment

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

Missing still the patron_types, ebooks, invoices.

rero_ils/modules/acq_accounts/api.py Outdated Show resolved Hide resolved
rero_ils/modules/acq_order_lines/api.py Show resolved Hide resolved
rero_ils/modules/acq_orders/api.py Outdated Show resolved Hide resolved
rero_ils/modules/circ_policies/api.py Outdated Show resolved Hide resolved
rero_ils/modules/items/api.py Outdated Show resolved Hide resolved
rero_ils/modules/notifications/api.py Outdated Show resolved Hide resolved
rero_ils/modules/patron_transaction_events/api.py Outdated Show resolved Hide resolved
rero_ils/modules/patron_transaction_events/api.py Outdated Show resolved Hide resolved
rero_ils/modules/acq_accounts/api.py Outdated Show resolved Hide resolved
@rerowep rerowep force-pushed the wep-#1251-records-links-to-organisation branch 5 times, most recently from 316c05f to bd6c33c Compare April 3, 2020 09:44
@rerowep
Copy link
Contributor Author

rerowep commented Apr 3, 2020 •
edited
Loading

Missing still the patron_types, ebooks, invoices.

  • ebooks: are covert by documents.
  • patron_types: Added pattern to JSON schema and configured required org
  • acq_invoices: Added config:
{
            'required': {
                'lib': 'library',
                'vndr': 'vendor'
            },
            'not_required': {
                'org': 'organisation'
            },
            'raise_on_erro': True
    }

@rerowep rerowep force-pushed the wep-#1251-records-links-to-organisation branch from bd6c33c to 31c776d Compare April 3, 2020 12:13
@rerowep rerowep force-pushed the wep-#1251-records-links-to-organisation branch 2 times, most recently from aae3e6f to fe59975 Compare May 1, 2020 20:53
@jma jma removed their request for review May 3, 2020 12:00
@rerowep rerowep force-pushed the wep-#1251-records-links-to-organisation branch from fe59975 to 9e443df Compare May 3, 2020 15:05
iGormilhit
iGormilhit suggested changes May 4, 2020
View reviewed changes
Copy link

@iGormilhit iGormilhit left a comment

Choose a reason for hiding this comment

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

Commit message proposition:

api: test if a PID is already existing

A method is needed to verify if a PID is already existing as the
system creates or updates a resource, in order to avoid ending
up with two records sharing the same PID.

* Adds functionality to test pids for existence.
* Test during acq_accounts creation and update the existence of pids.
* Test during acq_invoices cration and update the existence of pids.
* Test during acq_order_lines creation and update the existence of pids.
* Test during acq_orders creation and update the existence of pids.
* Test during holdings creation and update the existence of pids.
* Test during items creation and update the existence of pids.
* Test during loans creation and update the existence of pids.
* Test during patron_types creation and update the existence of pids.
* Closes #850.

@rerowep rerowep force-pushed the wep-#1251-records-links-to-organisation branch from 9e443df to 738b0d8 Compare May 4, 2020 17:35
@rerowep rerowep requested a review from iGormilhit May 4, 2020 17:35
@rerowep rerowep requested a review from jma May 5, 2020 07:35
@rerowep rerowep force-pushed the wep-#1251-records-links-to-organisation branch from 738b0d8 to b79f23c Compare May 5, 2020 07:36
@rerowep rerowep force-pushed the wep-#1251-records-links-to-organisation branch 10 times, most recently from b9ee0a1 to d25813b Compare May 11, 2020 14:20
jma
jma approved these changes May 12, 2020
View reviewed changes
rero_ils/modules/api.py Outdated
if not id_:
id_ = uuid4()
cls.minter(id_, data)
if cls.pids_exist_check:
from .utils import pids_exists_in_data
pids_exists_in_data(
Copy link
Contributor

Choose a reason for hiding this comment

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

From my point of view this code should be in the validate method and should raise a validation error

@rerowep rerowep force-pushed the wep-#1251-records-links-to-organisation branch 4 times, most recently from df351e7 to 0720964 Compare May 13, 2020 07:00
@rerowep
api: test if a PID is existing
392c4a8 
Method are needed to verify if a PID is already existing as the
system creates or updates a resource, in order to avoid ending
up with two records sharing the same PID and whether a reference
in the record to a different record exists.

* Adds functionality to test pids for existence.
* Test during acq_accounts creation and update existence of pids.
* Test during acq_invoices cration and update existence of pids.
* Test during acq_order_lines creation and update existence of pids.
* Test during acq_orders creation and update existence of pids.
* Test during holdings creation and update existence of pids.
* Test during items creation and update existence of pids.
* Test during loans creation and update existence of pids.
* Test during patron_types creation und update existence of pids.
* closes rero#850

Co-Authored-by: Peter Weber <[email protected]>
@rerowep rerowep force-pushed the wep-#1251-records-links-to-organisation branch from 0720964 to 392c4a8 Compare May 13, 2020 08:41
@rerowep rerowep merged commit 9f436dc into rero:dev May 13, 2020
@rerowep rerowep deleted the wep-#1251-records-links-to-organisation branch May 13, 2020 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jma jma jma approved these changes

@iGormilhit iGormilhit iGormilhit approved these changes

@BadrAly BadrAly BadrAly approved these changes

@lauren-d lauren-d Awaiting requested review from lauren-d

Assignees

@rerowep rerowep

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rerowep @jma @iGormilhit @BadrAly