Show/hide action list item paperclip icon when attachment is manually added/removed by users #1372
Conversation
WalkthroughThis pull request introduces changes to the file attachment handling process across multiple JavaScript files. The modifications focus on improving the management of file relationships for actions, specifically in how attachments are added and removed. The changes include updating the Changes
Sequence DiagramsequenceDiagram
participant User
participant UI
participant FileUploader
participant ActionModel
User->>UI: Upload File
UI->>FileUploader: Upload File
FileUploader-->>UI: Upload Success
UI->>ActionModel: Add File Relationship
ActionModel-->>UI: Update Action
UI->>User: Show Attachment Icon
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
RoundingWell Care Ops Frontend
|
||||||||||||||||||||||||||||
| Project |
RoundingWell Care Ops Frontend
|
| Branch Review |
attachment-icons
|
| Run status |
|
| Run duration | 03m 04s |
| Commit |
|
| Committer | Nick Major |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
313
|
| View all changes introduced in this branch ↗︎ | |
| @@ -236,6 +236,7 @@ export default App.extend(extend({ | |||
| }); | |||
| attachment.upload(file); | |||
|
|
|||
| this.action.addAttachment(attachment); | |||
There was a problem hiding this comment.
When passing the attachment to the addAttachment function here, attachment.id is undefined. Which will cause the test I created in this PR to fail.
There was a problem hiding this comment.
I think we need to wait until the upload is finished.
We could do something like this:
this.listenTo(attachment, 'upload:success', uploadedAttachment => {
this.action.addAttachment(uploadedAttachment);
Radio.request('ws', 'add', uploadedAttachment);
});Or:
attachment.upload(file).then(uploadedFile => {
this.action.addAttachment(uploadedAttachment);
Radio.request('ws', 'add', uploadedAttachment);
});There was a problem hiding this comment.
And if an upload fails ('upload:failed' triggered), this would also stop the paperclip icon and websocket from being added when they shouldn't be.
There was a problem hiding this comment.
Decided to create a new test. I couldn't figure out how to weave the testing of this feature into an existing one.
Since this test needs to have an action with this sequence:
- Start with zero attachments (icon hidden)
- Add an attachment (icon switched to shown)
- Remove that attachment (icon switch back to hidden)
| @@ -250,6 +249,11 @@ const _Model = BaseModel.extend({ | |||
|
|
|||
| return !!size(programAction.get('allowed_uploads')); | |||
| }, | |||
| addAttachment(resource) { | |||
There was a problem hiding this comment.
Are the names of these clear? Could cause confusion in the future (for me at least).
Maybe they should be addAttachmentRelationship()/removeAttachmentRelationshop() or addFileRelationship()/removeFileRelationship() 🤔
nmajor25
force-pushed
the
attachment-icons
branch
from
d5b8065 to
8050d47
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/js/entities-service/entities/actions.js (1)
252-256: Efficient creation of new files array via spread operator.Using the spread operator to produce a new
_filesrelationship array is clear and concise. However, consider verifying the file does not already exist in_filesif duplicates aren’t desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/js/apps/patients/sidebar/action-sidebar_app.js(2 hunks)src/js/entities-service/entities/actions.js(2 hunks)src/js/entities-service/entities/files.js(2 hunks)test/integration/patients/sidebar/action-sidebar.js(1 hunks)
🔇 Additional comments (7)
src/js/entities-service/entities/files.js (2)
31-31: Adoption of removeFileRelationship is appropriate.Replacing the old
removeAttachmentcall withremoveFileRelationshipaligns well with the centralized file-relationship handling introduced elsewhere in this PR.
69-71: Good event-based approach for successful uploads.Emitting the
upload:successevent allows higher-level components to respond to upload completion in a decoupled manner. This pattern is clean and maintainable.src/js/apps/patients/sidebar/action-sidebar_app.js (2)
239-242: Properly adding the file relationship after successful upload.Listening for
upload:successand then invokingaction.addFileRelationship(uploadedAttachment)ensures the UI and data model remain in sync, preventing race conditions and incomplete references to attachments.
251-252: Consistency in removing file relationship alongside model destruction.Destroying the attachment model and removing its file relationship on the action object maintains data integrity by ensuring there are no stale references.
src/js/entities-service/entities/actions.js (2)
67-67: Refined approach to adding attachments.Using
addFileRelationship(attachmentModel)in theAttachmentAddedmessage centralizes attachment handling for better consistency and maintainability.
257-257: Robust removal of file relationship.Filtering out the relevant file by its
idensures correctness when removing the relationship object from_files.test/integration/patients/sidebar/action-sidebar.js (1)
971-1124: Comprehensive test coverage for attachment behavior in the action list.The newly added test thoroughly verifies the presence/absence of the paperclip icon upon file add/remove, ensuring accurate reflection of attachment status in the UI. This end-to-end approach helps safeguard against regressions.
Pull Request Test Coverage Report for Build a888a9b5-d682-4839-832a-9fa460d05fe5Details
💛 - Coveralls |
nmajor25
force-pushed
the
attachment-icons
branch
from
8050d47 to
be2d4df
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/js/entities-service/entities/actions.js (1)
Line range hint
257-263: Consider consistent implementation styleWhile the implementation is correct, consider making it consistent with addFileRelationship's style:
removeFileRelationship(resource) { - const files = this.get('_files'); - const newFilesRelationship = files.filter(file => { - return file.id !== resource.id; - }); + const newFilesRelationship = this.get('_files').filter(file => file.id !== resource.id); this.set({ _files: newFilesRelationship }); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/js/apps/patients/sidebar/action-sidebar_app.js(2 hunks)src/js/entities-service/entities/actions.js(2 hunks)src/js/entities-service/entities/files.js(2 hunks)test/integration/patients/sidebar/action-sidebar.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/js/entities-service/entities/files.js
- test/integration/patients/sidebar/action-sidebar.js
🔇 Additional comments (4)
src/js/apps/patients/sidebar/action-sidebar_app.js (2)
239-242: Great improvement in upload handling!The code now correctly waits for the upload to complete before adding the file relationship and websocket subscription. This fixes the previous issue with undefined attachment.id.
251-252: Clean implementation of attachment removal!The code properly maintains the file relationship state after attachment removal.
src/js/entities-service/entities/actions.js (2)
67-67: Good architectural choice!Using the dedicated addFileRelationship method centralizes the file relationship management logic.
252-256: Well-implemented relationship management!The method properly maintains immutability while adding new file relationships.
| @@ -236,7 +236,10 @@ export default App.extend(extend({ | |||
| }); | |||
| attachment.upload(file); | |||
|
|
|||
| Radio.request('ws', 'add', attachment); | |||
| this.listenTo(attachment, 'upload:success', uploadedAttachment => { | |||
There was a problem hiding this comment.
we should combine this with 244 this.listenTo(attachment, {}
There was a problem hiding this comment.
Yep good call, updated to that 👍
| @@ -250,7 +249,12 @@ const _Model = BaseModel.extend({ | |||
|
|
|||
| return !!size(programAction.get('allowed_uploads')); | |||
| }, | |||
| removeAttachment(resource) { | |||
| addFileRelationship(resource) { | |||
There was a problem hiding this comment.
I think we can remove the "Relationship" from the name The other instances like this don't have it.. addTag removeTag etc
There was a problem hiding this comment.
Makes sense, updated to that 👍
… added/removed by a user
nmajor25
force-pushed
the
attachment-icons
branch
from
be2d4df to
d0e03a0
Compare
Shortcut Story ID: [sc-58034]
Summary by CodeRabbit
New Features
Bug Fixes
Tests