Details
- Reviewers
ardumont - Maniphest Tasks
- T832: deposit metadata injection into origin_metadata
T837: add link to metadata documentation - Commits
- Restricted Diffusion Commit
rDDEPb827c935f461: Refactor tests: use actual prepare_metadata without overriding it for tests
rDDEP53f407f61338: Refactor origin_metadata injection with prepare_metadata from loader-core
rDDEPe2d1938b069c: Added tests for metadata injection
rDDEP922f8952915d: Change metadata provider and tool as additional config
rDDEP49f7beee3530: Added metadata injection and url to client
Diff Detail
- Repository
- rDDEP Push deposit
- Branch
- master
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 1107 Build 1451: arc lint + arc unit
Event Timeline
| swh/deposit/api/private/deposit_read.py | ||
|---|---|---|
| 234 | I would have made this a class configuration instead (using class variable ADDITIONAL_CONFIG). | |
| swh/deposit/migrations/0006_depositclient_url.py | ||
| 18 | I know we did this together but we must avoid hardcoding this. Well, at least, we don't want this to stay as is in the long run. At some point, this default should go away. | |
| swh/deposit/migrations/0006_depositclient_url.py | ||
|---|---|---|
| 18 | I'm not sure how to do it differently for now. | |
| swh/deposit/injection/loader.py | ||
|---|---|---|
| 163 | I think the metadata are important enough so we could consider this an error worth making the import fail altogether? | |
| swh/deposit/migrations/0006_depositclient_url.py | ||
| 18 | No worries there. We needed this because in our db we already had one deposit client prior to adding that field. | |
| swh/deposit/tests/api/test_deposit_read_metadata.py | ||
| 40–60 | Maybe worth adding a value different than production to make it show that this is not coming from the db (well, the runtime db at least)? | |
- Change metadata provider and tool as additional config
- Added tests for metadata injection
- Refactor origin_metadata injection with prepare_metadata from loader-core
Almost ok!
Just some minor test override detail or typo.
| docs/metadata.md | ||
|---|---|---|
| 11 ↗ | (On Diff #905) | persistent |
| docs/spec-injection.md | ||
| 208 ↗ | (On Diff #905) | I'm not so sure we want to integrate so much implementation detail. |
| swh/deposit/api/private/deposit_read.py | ||
| 174 | Note: We must make sure that the deposit.client.url holds a trailing '/'. Or, you can change this as: os.path.join(deposit.client.url.rstrip('/') , deposit.external_id) | |
| 174–175 | Yes, nice. Type deposit does exist in the db now. | |
| swh/deposit/injection/loader.py | ||
| 162 | Same as in D266, we should raise an error to notify that the loading somehow failed since we were not able to actually store the metadata. | |
| swh/deposit/tests/common.py | ||
| 163 | \m/ | |
| swh/deposit/tests/test_loader.py | ||
| 140 | Let the prepare_metadata do its job and do not override it here. Prefer overriding the send_tool and send_provider instead as they are in a way leaf methods. That one is more complicated as it orchestrates creation of tool and provider (this one has multiple actions including filtering...). Also, as you override it, we potentially let a bug crawl in as we do not pass in that runtime code in the test. It's more ok to do so for the send* function as they are really covered by the swh-storage layer already. | |
| 187 | return json.loads(r.content.decode('utf-8')) | |
| swh/deposit/injection/loader.py | ||
|---|---|---|
| 163 | Yes, i agree with myself, add a raise here in the except clause :) | |
- Refactor tests: use actual prepare_metadata without mocking it for tests
Well, you did not mock, you overrode :)