fix: update photo asset id property to use correct record#216
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR fixes photo album operations (notably PhotoAlbum.add_photo) by aligning PhotoAsset.id with the correct iCloud Photos identifier (the asset record’s recordName) rather than the master record’s recordName, addressing issue #207 where adding a photo to an album created an unusable/invisible entry.
Changes:
- Update
PhotoAsset.idto return the asset recordrecordName. - Update photos service tests to include
CPLAsset.recordNamein mocked responses and assertPhotoAsset.idmatches the asset id.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
pyicloud/services/photos.py |
Switch PhotoAsset.id to use the asset record identifier so album relation writes target the correct record. |
tests/services/test_photos.py |
Adjust mocks/assertions so tests reflect asset-vs-master id distinctions and validate the new id behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @property | ||
| def id(self) -> str: | ||
| """Gets the photo id.""" | ||
| return self._master_record["recordName"] | ||
| return self._asset_record["recordName"] |
There was a problem hiding this comment.
The id docstring is now ambiguous: this property returns the asset recordName (used by album relation operations), not the master recordName. Please update the docstring (and ideally mention/expose the master record identifier separately) so API consumers can distinguish the two identifiers.
| @property | ||
| def id(self) -> str: | ||
| """Gets the photo id.""" | ||
| return self._master_record["recordName"] | ||
| return self._asset_record["recordName"] |
There was a problem hiding this comment.
This change is intended to fix PhotoAlbum.add_photo() by switching PhotoAsset.id to the asset recordName, but there is currently no direct unit test asserting the add_photo() request payload uses the asset id in both itemId and recordName. Adding a focused test would prevent regressions and ensure the original issue (#207) stays fixed.
| mock_photo = MagicMock(spec=PhotoAsset) | ||
| mock_photo.id = "target_photo" | ||
|
|
||
| mock_photo_library.service.session.post.return_value.json.return_value = { | ||
| "records": [ |
There was a problem hiding this comment.
mock_photo is declared but never used in this test, which makes the test harder to follow. Consider removing it (or using it) since the behavior under test is driven entirely by the mocked session.post().json() response.
| mock_photo = MagicMock(spec=PhotoAsset) | ||
| mock_photo.id = "different_photo" | ||
|
|
||
| mock_photo_library.service.session.post.return_value.json.return_value = { | ||
| "records": [ |
There was a problem hiding this comment.
mock_photo is declared but never used in this test, which makes the test harder to follow. Consider removing it (or using it) since the behavior under test is driven entirely by the mocked session.post().json() response.
Proposed change
Type of change
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: