-
Notifications
You must be signed in to change notification settings - Fork 303
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.
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
Communication
: Include imageUrl for reactions
#9897
Communication
: Include imageUrl for reactions
#9897
Conversation
WalkthroughThe changes involve an update to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 pmd (7.7.0)src/main/java/de/tum/cit/aet/artemis/communication/domain/Reaction.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/communication/domain/Reaction.java
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/communication/domain/Reaction.java (1)
Pattern src/main/java/**/*.java
: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/communication/domain/Reaction.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on TS1, the response includes the imageUrl now 👍 Code changes LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a good change. We want to keep the information minimal here. Can you not change the Android way of handling this?
I actually did this as a workaround in the Android app for now. However, I think including the imageUrl here does not expose any private information (can be accessed anyway for a users post). Additionally this would then also be required in the future to improve the reaction display in the Android app (and maybe also iOS). I assume we want to implement the mobile reaction overview similar to Slack or Whatsapp, both of which show the profile picture in the BottomSheets next to the reactors name. Adding this is on the feature collection for Android (see here). EDIT: I updated the PR description to also reflect the benefits in future updates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a good approach, can you try to fix it in the Android app instead?
Yes, this PR already includes a workaround for this on Android. I am not sure whether I understand correctly, should we then just re-open this PR once we want to start working on the reaction overview improvements? |
It was decided that including the imageUrl in the reactions is not required, as it is not needed in the webapp and would be too much information the send for every reaction. For the mobile apps, if they want to show the ProfilePictures for reactions, they should rather fetch the users separately and get the imageUrl from there. |
Checklist
General
Motivation and Context
The Android app currently expects the reaction authors to also include a imageUrl (basically to have the same format as the author for a post entity). While they are not currently needed for displaying them in the UI, it messes with the database entries.
Additionally, this change is needed for planned future improvements of the reaction overview in Android, which will show the reaction authors profile picture in the BottomSheet.
Description
Included the imageUrl to be returned for reactions.
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Test Coverage
Summary by CodeRabbit
imageUrl
property alongsideid
andname
in the Reaction class.This update improves the data available for users without altering existing functionalities.