Edit album title and description #823
Conversation
|
Warning Rate limit exceeded@janosrusiczki has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 38 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThe changes involve the removal of the Changes
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/javascript/mixins/photo-title.js(0 hunks)app/javascript/mixins/title-helper.js(1 hunks)app/javascript/photos/photo-management.vue(1 hunks)app/javascript/photos/photo-title-editable.vue(2 hunks)app/javascript/photos/show.vue(3 hunks)
💤 Files with no reviewable changes (1)
- app/javascript/mixins/photo-title.js
✅ Files skipped from review due to trivial changes (1)
- app/javascript/photos/photo-management.vue
🔇 Additional comments (4)
app/javascript/photos/photo-title-editable.vue (1)
46-46: LGTM: Title computation updated correctly
The change from photoTitle to titleHelper is implemented correctly.
app/javascript/photos/show.vue (3)
51-51: LGTM: Component renamed consistently
The PhotoAdministration component has been renamed to PhotoManagement.
390-390: LGTM: Title helper implementation with loading state
The title computation correctly uses titleHelper with both photo and loading parameters.
271-271: Verify component rename consistency
Let's ensure the PhotoAdministration to PhotoManagement rename is consistent across the codebase.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #823 +/- ##
==========================================
- Coverage 93.42% 93.41% -0.01%
==========================================
Files 164 169 +5
Lines 2919 3038 +119
==========================================
+ Hits 2727 2838 +111
- Misses 192 200 +8 ☔ View full report in Codecov by Sentry. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/javascript/mixins/title-helper.js (2)
1-3: Consider adding i18n support for the "no title" textThe
noTitleconstant could be internationalized to support multiple languages.import { ref } from "vue"; +import { useI18n } from "vue-i18n"; -const noTitle = "(no title)"; +const { t } = useI18n(); +const noTitle = t("common.no_title", "(no title)");
15-15: Consider using named exportsUsing named exports instead of default exports can improve tree-shaking and make imports more explicit.
-export default titleHelper; +export { titleHelper };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/javascript/mixins/title-helper.js(1 hunks)
🔇 Additional comments (3)
app/javascript/mixins/title-helper.js (3)
5-10: Documentation looks great!
The JSDoc documentation is clear, complete, and includes proper type definitions.
11-13:
Add parameter validation for safer runtime behavior
The function still lacks validation for the input parameters which could lead to runtime errors if:
objectis undefined or not a refobject.valueis undefined
const defaultLoading = ref(false);
-const titleHelper = (object, loading = defaultLoading) =>
- loading.value ? "Loading..." : object.value.title || noTitle;
+const titleHelper = (object, loading = defaultLoading) => {
+ if (!object?.value) {
+ console.warn('titleHelper: Invalid object parameter');
+ return noTitle;
+ }
+ return loading.value ? "Loading..." : object.value.title || noTitle;
+};1-15: Verify complete migration from photoTitle mixin
Let's ensure all usages of the old photoTitle mixin have been updated to use this new helper.
✅ Verification successful
Migration from photoTitle mixin is complete
The verification shows that the new titleHelper is properly integrated and used in both relevant components:
app/javascript/photos/show.vue: Used for page title with loading stateapp/javascript/photos/photo-title-editable.vue: Used for editable title display
No remaining instances of the old photoTitle mixin were found in the codebase, indicating a successful migration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of photoTitle mixin
echo "Checking for remaining photoTitle mixin usage..."
rg -l "photoTitle"
# Search for new titleHelper usage to confirm migration
echo "Checking titleHelper usage..."
rg -l "titleHelper"
Length of output: 330
Script:
#!/bin/bash
# Let's check the actual usage context in the files
echo "Checking titleHelper usage in show.vue..."
rg -C 5 "titleHelper" "app/javascript/photos/show.vue"
echo "\nChecking titleHelper usage in photo-title-editable.vue..."
rg -C 5 "titleHelper" "app/javascript/photos/photo-title-editable.vue"
# Let's also check for any historical references to photoTitle in git history
echo "\nChecking git history for photoTitle..."
git log --oneline --grep="photoTitle"
Length of output: 1887
janosrusiczki
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (7)
app/graphql/mutations/update_album_title.rb (1)
8-9: Correct the argument description for clarityThe description for the
idargument incorrectly mentions 'Photo Id' instead of 'Album Id'. This could cause confusion for developers.Apply this diff to fix the description:
argument :id, String, - 'Photo Id', + 'Album Id', required: truespec/requests/graphql/mutations/update_album_title_spec.rb (1)
42-52: Reduce test example length to comply with RuboCop recommendationsThe
it 'updates the album title' doblock exceeds the recommended maximum of 5 lines for an example. This can be improved by extracting parts of the test into helper methods for better readability and maintainability.Apply this refactoring to adhere to the style guidelines:
it 'updates the album title' do post_mutation data = response_data expect(data).to include( 'id' => album.slug, 'title' => new_title, 'description' => description ) end private def response_data json = response.parsed_body json['data']['updateAlbumTitle'] endAlternatively, consider disabling the RuboCop rule for this example if refactoring is not appropriate.
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 42-52: Example has too many lines. [8/5]
(RSpec/ExampleLength)
app/javascript/albums/album-title-editable.vue (1)
8-9: Consider improving the title display formatThe current format "Album: {title}" is hardcoded in the template. Consider making this more flexible by:
- Moving the "Album:" prefix to a translatable string
- Making the prefix optional through props
- Album: - {{ title }} + <span v-if="showPrefix">{{ prefix }}</span> + <span>{{ title }}</span>app/graphql/types/photo_type.rb (1)
15-18: Consider extracting EXIF fields into a separate typeThe EXIF-related fields could be better organized by moving them into a dedicated EXIF type. This would improve code organization and make the schema more maintainable.
Example implementation:
module Types class ExifType < Types::BaseObject description 'EXIF data for a photo' field :exists, Boolean, 'Whether EXIF data exists', null: true field :camera_friendly_name, String, 'Camera friendly name', null: true field :exposure_time, String, 'Exposure time', null: true field :f_number, Float, 'F number', null: true field :focal_length, Float, 'Focal length', null: true field :iso, Integer, 'ISO', null: true end class PhotoType < Types::BaseObject # ... other fields ... field :exif, ExifType, 'EXIF data', null: true end end🧰 Tools
🪛 rubocop (1.69.1)
[convention] 18-18: Consider moving exif_camera_friendly_name, exif_exists, exif_exposure_time, exif_f_number, exif_focal_length, exif_iso to a new type and adding the
exiffield instead(GraphQL/ExtractType)
app/graphql/types/mutation_type.rb (3)
8-10: Add missing field descriptions for better API documentation.GraphQL best practices recommend adding descriptions for all fields to improve API documentation and discoverability.
Add descriptions like this:
- field :update_album_title, mutation: Mutations::UpdateAlbumTitle - field :update_photo_description, mutation: Mutations::UpdatePhotoDescription - field :update_photo_title, mutation: Mutations::UpdatePhotoTitle + field :update_album_title, mutation: Mutations::UpdateAlbumTitle, + description: 'Update an album\'s title' + field :update_photo_description, mutation: Mutations::UpdatePhotoDescription, + description: 'Update a photo\'s description' + field :update_photo_title, mutation: Mutations::UpdatePhotoTitle, + description: 'Update a photo\'s title'🧰 Tools
🪛 rubocop (1.69.1)
[convention] 8-8: Missing field description
(GraphQL/FieldDescription)
[convention] 9-9: Missing field description
(GraphQL/FieldDescription)
[convention] 10-10: Consider moving update_photo_description, update_photo_title to a new type and adding the
update_photofield instead(GraphQL/ExtractType)
[convention] 10-10: Missing field description
(GraphQL/FieldDescription)
9-10: Consider extracting photo-related mutations into a separate type.For better organization and maintainability, consider grouping photo-related mutations under a new
UpdatePhototype.Example refactor:
# types/mutations/update_photo_type.rb module Types module Mutations class UpdatePhotoType < GraphQL::Schema::Object field :description, mutation: Mutations::UpdatePhotoDescription field :title, mutation: Mutations::UpdatePhotoTitle end end end # In MutationType field :update_photo, mutation: Types::Mutations::UpdatePhotoType🧰 Tools
🪛 rubocop (1.69.1)
[convention] 9-9: Missing field description
(GraphQL/FieldDescription)
[convention] 10-10: Consider moving update_photo_description, update_photo_title to a new type and adding the
update_photofield instead(GraphQL/ExtractType)
[convention] 10-10: Missing field description
(GraphQL/FieldDescription)
Line range hint
217-229: Optimize user settings update method.Several improvements can be made to this method:
- Remove the unused
- Use a single update call for better performance
- Improve documentation about email updates
Suggested refactor:
- def update_user_settings(email:, first_name:, last_name:, display_name:, timezone:) + def update_user_settings(first_name:, last_name:, display_name:, timezone:) user = context[:current_user] raise Pundit::NotAuthorizedError, 'User not signed in' unless user context[:authorize].call(user, :update?) - # for now we don't allow users to change their email - # as that should trigger Devise's confirmation email - # user.update(email: email) - user.update(first_name: first_name) - user.update(last_name: last_name) - user.update(display_name: display_name) - user.update(timezone: timezone) + # Email updates are handled separately through Devise's confirmation flow + user.update( + first_name: first_name, + last_name: last_name, + display_name: display_name, + timezone: timezone + ) user end🧰 Tools
🪛 rubocop (1.69.1)
[convention] 184-215: Assignment Branch Condition size for continue_with_facebook is too high. [<5, 28, 6> 29.07/17]
(Metrics/AbcSize)
[convention] 184-215: Method has too many lines. [24/10]
(Metrics/MethodLength)
[warning] 217-217: Unused method argument -
(Lint/UnusedMethodArgument)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/graphql/graphql_query_collection.rb(1 hunks)app/graphql/mutations/update_album_title.rb(1 hunks)app/graphql/types/album_type.rb(2 hunks)app/graphql/types/mutation_type.rb(4 hunks)app/graphql/types/photo_type.rb(1 hunks)app/javascript/albums/album-title-editable.vue(1 hunks)app/javascript/albums/show.vue(3 hunks)spec/requests/graphql/mutations/update_album_title_spec.rb(1 hunks)
🧰 Additional context used
🪛 rubocop (1.69.1)
spec/requests/graphql/mutations/update_album_title_spec.rb
[convention] 42-52: Example has too many lines. [8/5]
(RSpec/ExampleLength)
app/graphql/types/photo_type.rb
[convention] 18-18: Consider moving exif_camera_friendly_name, exif_exists, exif_exposure_time, exif_f_number, exif_focal_length, exif_iso to a new type and adding the exif field instead
(GraphQL/ExtractType)
app/graphql/types/mutation_type.rb
[convention] 8-8: Missing field description
(GraphQL/FieldDescription)
[convention] 9-9: Missing field description
(GraphQL/FieldDescription)
[convention] 10-10: Consider moving update_photo_description, update_photo_title to a new type and adding the update_photo field instead
(GraphQL/ExtractType)
[convention] 10-10: Missing field description
(GraphQL/FieldDescription)
[convention] 184-215: Assignment Branch Condition size for continue_with_facebook is too high. [<5, 28, 6> 29.07/17]
(Metrics/AbcSize)
[convention] 184-215: Method has too many lines. [24/10]
(Metrics/MethodLength)
[warning] 217-217: Unused method argument - email.
(Lint/UnusedMethodArgument)
🔇 Additional comments (2)
app/javascript/albums/album-title-editable.vue (1)
88-101: 🛠️ Refactor suggestion
Add validation for maximum title length
The updateTitle method validates for empty titles but doesn't check for maximum length. Consider adding a maximum length validation to prevent excessively long titles.
const updateTitle = () => {
if (savedTitle != localTitle.value) {
- if (
- localTitle.value.trim() === ""
- ) {
+ const trimmedTitle = localTitle.value.trim();
+ if (trimmedTitle === "") {
toaster("The album title is required", "is-warning");
focusInput();
return;
}
+ if (trimmedTitle.length > 255) {
+ toaster("The album title cannot exceed 255 characters", "is-warning");
+ focusInput();
+ return;
+ }
emit("updateTitle", { id: album.value.id, title: localTitle.value });
}
editing.value = false;
applicationStore.stopEditing();
};app/graphql/graphql_query_collection.rb (1)
66-66: LGTM! Consistent permission handling
The addition of canEdit field to the album query is consistent with the implementation in other queries and aligns with the PR objective of enabling album editing.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/javascript/albums/show.vue (1)
4-13: Consider adding ARIA attributes for better accessibilityThe conditional rendering logic looks good, but we could improve accessibility for screen readers.
Consider adding appropriate ARIA attributes:
- <div class="title mt-5 mb-0"> + <div class="title mt-5 mb-0" role="heading" aria-level="1">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/graphql/types/album_type.rb(2 hunks)app/javascript/albums/show.vue(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/graphql/types/album_type.rb
🔇 Additional comments (6)
app/javascript/albums/show.vue (6)
42-42: LGTM! Imports are well-organized
The new imports are properly structured and necessary for the added functionality.
Also applies to: 44-46
61-62: LGTM! Store initialization is correct
The userStore is properly initialized and used for authentication-based conditional rendering.
66-71: LGTM! Query setup follows best practices
The query setup properly handles loading state and follows Vue Apollo best practices.
78-93: LGTM! Mutation setup follows best practices
The GraphQL mutation is well-structured and the success handler provides appropriate user feedback.
95-100:
Avoid exposing internal error details to users
Displaying the raw error message to users could reveal sensitive information.
Consider using a user-friendly message:
onUpdateTitleError((error) => {
toaster(
- "An error occurred while updating the title: " + error.message,
+ "An error occurred while updating the title. Please try again later.",
"is-danger",
);
});75-76: Verify titleHelper implementation
The title computation looks good, but let's verify the titleHelper implementation.
✅ Verification successful
The previous pattern might have been too specific. Let's search more broadly for the titleHelper implementation and any files that might contain it.
We found the titleHelper implementation file. Let's examine its complete contents to verify the implementation.
titleHelper implementation is correct and well-documented
The titleHelper function is properly implemented with:
- Correct handling of loading state showing "Loading..." when loading
- Fallback to "(no title)" for empty titles
- Well-documented parameters and return types
- Consistent usage across photos and albums components
The title computation in show.vue is using it correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check titleHelper implementation
ast-grep --pattern 'export default function titleHelper($_, $_) {
$$$
}'
Length of output: 76
Script:
#!/bin/bash
# Search for titleHelper function with broader patterns and context
rg "titleHelper" -A 5
# Also try finding the file containing titleHelper
fd "titleHelper"
Length of output: 3745
Script:
#!/bin/bash
# Get the full implementation of title-helper.js
cat app/javascript/mixins/title-helper.js
Length of output: 602
Refactor ContinueWithFacebookService Rewrite specs Rename social -> provider
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (7)
app/mailers/admin_mailer.rb (1)
Line range hint
1-13: Consider adding frozen string literal commentAs suggested by Rubocop, consider adding the frozen string literal comment to potentially improve memory usage.
+# frozen_string_literal: true class AdminMailer < ApplicationMailerspec/mailers/admin_mailer_spec.rb (1)
12-18: Use described_class instead of explicit class nameFollowing RSpec best practices, replace
AdminMailerwithdescribed_class.let(:mail) do - AdminMailer.with( + described_class.with( provider:, first_name:, last_name:, email:, admin_emails: ).new_provider_user end🧰 Tools
🪛 rubocop (1.69.1)
[convention] 12-12: Use
described_classinstead ofAdminMailer.(RSpec/DescribedClass)
app/services/continue_with_facebook_service.rb (1)
46-48: Handle potentialnilvalues invalidate_user_info!When comparing user IDs, ensure that both
decoded_payload['user_id']anduser_info['id']are present to avoidNoMethodErrorexceptions.Modify the condition to safely handle
nilvalues:def validate_user_info!(user_info) - raise InvalidUserInfoError unless decoded_payload['user_id'] == user_info['id'] + if decoded_payload['user_id'].nil? || user_info['id'].nil? || decoded_payload['user_id'] != user_info['id'] + raise InvalidUserInfoError, 'User ID mismatch or missing' + end endspec/services/continue_with_facebook_service_spec.rb (2)
18-18: Use verifying doubles instead of normal doublesPrefer using verifying doubles like
instance_doubleto ensure that stubbed methods exist on the actual object, preventing tests from passing with incorrect or outdated stubs.Apply this diff to use an
instance_doublefor the response:-let(:response) { double('response', body: facebook_user_info.to_json) } +let(:response) { instance_double(Net::HTTPResponse, body: facebook_user_info.to_json) }Ensure that
Net::HTTPResponseis the correct class for the response object.🧰 Tools
🪛 rubocop (1.69.1)
[convention] 18-18: Prefer using verifying doubles over normal doubles.
(RSpec/VerifiedDoubles)
21-22: Usereceive_messagesto stub multiple methods conciselyWhen stubbing multiple methods on the same object, using
receive_messagescan make the test code cleaner and more maintainable.Apply this change:
-allow(instance).to receive(:valid_signature?).and_return(true) -allow(instance).to receive(:decoded_payload).and_return(decoded_payload) +allow(instance).to receive_messages( + valid_signature?: true, + decoded_payload: decoded_payload +)🧰 Tools
🪛 rubocop (1.69.1)
[convention] 21-21: Use
receive_messagesinstead of multiple stubs on lines [22].(RSpec/ReceiveMessages)
[convention] 21-21: Do not stub methods of the object under test.
(RSpec/SubjectStub)
[convention] 22-22: Use
receive_messagesinstead of multiple stubs on lines [21].(RSpec/ReceiveMessages)
[convention] 22-22: Do not stub methods of the object under test.
(RSpec/SubjectStub)
spec/requests/graphql/mutations/continue_with_facebook_spec.rb (2)
55-61: Consider using real records instead of stubbing ActiveRecord relationsStubbing
User.adminsand its methods may lead to brittle tests that don't reflect actual behavior. Creating real admin users in the test database can improve test reliability.Replace the stubs with actual data setup:
before do # Create an admin user for the mailer create(:user, email: '[email protected]', admin: true) endThis approach avoids stubbing and relies on the real behavior of
User.admins.
70-70: Useperform_enqueued_jobsto test email deliveryInstead of relying on
Sidekiq::Testing.inline!, useperform_enqueued_jobsto execute jobs within the test, ensuring that the test environment is correctly isolated.Apply this change:
- Sidekiq::Testing.inline! do + perform_enqueued_jobs do expect { post_mutation }.to change(ActionMailer::Base.deliveries, :count).by(1) endEnsure you have the necessary configuration for
perform_enqueued_jobs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/graphql/types/mutation_type.rb(5 hunks)app/mailers/admin_mailer.rb(1 hunks)app/models/user.rb(1 hunks)app/services/continue_with_facebook_service.rb(1 hunks)spec/mailers/admin_mailer_spec.rb(1 hunks)spec/mailers/previews/admin_mailer_preview.rb(1 hunks)spec/models/user_spec.rb(2 hunks)spec/requests/graphql/mutations/continue_with_facebook_spec.rb(3 hunks)spec/services/continue_with_facebook_service_spec.rb(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- spec/mailers/previews/admin_mailer_preview.rb
🧰 Additional context used
🪛 rubocop (1.69.1)
spec/models/user_spec.rb
[convention] 71-71: Example has too many expectations [7/2].
(RSpec/MultipleExpectations)
spec/services/continue_with_facebook_service_spec.rb
[convention] 18-18: Prefer using verifying doubles over normal doubles.
(RSpec/VerifiedDoubles)
[convention] 21-21: Use receive_messages instead of multiple stubs on lines [22].
(RSpec/ReceiveMessages)
[convention] 21-21: Do not stub methods of the object under test.
(RSpec/SubjectStub)
[convention] 22-22: Use receive_messages instead of multiple stubs on lines [21].
(RSpec/ReceiveMessages)
[convention] 22-22: Do not stub methods of the object under test.
(RSpec/SubjectStub)
[convention] 40-40: Do not stub methods of the object under test.
(RSpec/SubjectStub)
app/graphql/types/mutation_type.rb
[convention] 8-8: Missing field description
(GraphQL/FieldDescription)
[convention] 9-9: Missing field description
(GraphQL/FieldDescription)
[convention] 10-10: Consider moving update_photo_description, update_photo_title to a new type and adding the update_photo field instead
(GraphQL/ExtractType)
[convention] 10-10: Missing field description
(GraphQL/FieldDescription)
[warning] 188-188: Unused method argument - email.
(Lint/UnusedMethodArgument)
spec/mailers/admin_mailer_spec.rb
[convention] 1-1: Missing frozen string literal comment.
(Style/FrozenStringLiteralComment)
[convention] 12-12: Use described_class instead of AdminMailer.
(RSpec/DescribedClass)
[convention] 26-26: Example has too many expectations [4/2].
(RSpec/MultipleExpectations)
🔇 Additional comments (10)
app/mailers/admin_mailer.rb (1)
Line range hint 5-13: Verify all callers of the renamed mailer method
The method has been renamed from new_social_user to new_provider_user. While the implementation looks correct, we should ensure all callers are updated.
✅ Verification successful
All references to the mailer method have been updated correctly
The verification shows that:
- No occurrences of the old method name
new_social_userwere found in the Ruby files - All references to
new_provider_userare properly updated in:- The mailer class (
app/mailers/admin_mailer.rb) - The mailer specs (
spec/mailers/admin_mailer_spec.rb) - The mailer preview (
spec/mailers/previews/admin_mailer_preview.rb) - The GraphQL mutation type (
app/graphql/types/mutation_type.rb)
- The mailer class (
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old method name
rg "new_social_user" --type ruby
# Search for calls to the new method name for verification
rg "new_provider_user" --type ruby
Length of output: 677
spec/mailers/admin_mailer_spec.rb (1)
21-32: LGTM! Test coverage looks comprehensive
The test coverage is thorough, checking both headers and body content. While there are multiple expectations in the body test, they are logically related and testing different aspects of the same rendered content.
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 26-26: Example has too many expectations [4/2].
(RSpec/MultipleExpectations)
spec/models/user_spec.rb (2)
Line range hint 55-86: LGTM! Tests properly updated to match implementation.
The test changes correctly reflect the method rename while maintaining thorough test coverage. All scenarios are well-tested.
Note: While RuboCop flags multiple expectations in one example, this is acceptable here as all expectations are testing different attributes of the same user object. Breaking this into separate examples would reduce test readability.
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 71-71: Example has too many expectations [7/2].
(RSpec/MultipleExpectations)
Line range hint 1-1:
The PR is titled "Edit album title and description" and references issue #685, but the actual changes are about renaming an OAuth authentication method from find_or_create_from_social to find_or_create_from_provider. While these changes are well-implemented, they appear unrelated to the stated objectives.
Please clarify:
- Are these changes intended for a different PR?
- Are there missing changes related to album title editing?
app/graphql/types/mutation_type.rb (2)
215-223: Good job extracting admin notification logic into a separate method
Extracting the admin notification logic into notify_admins_of_new_user improves code reusability and reduces duplication across authentication methods.
225-233: Refactored continue_with_facebook by extracting user creation logic
Extracting the user creation logic into find_or_create_facebook_user reduces complexity in the continue_with_facebook method, enhancing readability and maintainability.
app/services/continue_with_facebook_service.rb (1)
16-21: Well-structured facebook_user_info method improves clarity
The facebook_user_info method effectively encapsulates the steps of verifying the signature, fetching user info, and validating it, enhancing code readability and maintainability.
spec/services/continue_with_facebook_service_spec.rb (2)
40-40: Avoid stubbing methods on the object under test (instance)
As mentioned earlier, stubbing methods on the subject can mask potential issues. Refactor the test to avoid stubbing valid_signature? on instance.
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 40-40: Do not stub methods of the object under test.
(RSpec/SubjectStub)
15-28: Effective test coverage for facebook_user_info method
The tests comprehensively cover the successful retrieval of Facebook user info and the handling of mismatched user IDs, ensuring robust behavior.
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 18-18: Prefer using verifying doubles over normal doubles.
(RSpec/VerifiedDoubles)
[convention] 21-21: Use receive_messages instead of multiple stubs on lines [22].
(RSpec/ReceiveMessages)
[convention] 21-21: Do not stub methods of the object under test.
(RSpec/SubjectStub)
[convention] 22-22: Use receive_messages instead of multiple stubs on lines [21].
(RSpec/ReceiveMessages)
[convention] 22-22: Do not stub methods of the object under test.
(RSpec/SubjectStub)
spec/requests/graphql/mutations/continue_with_facebook_spec.rb (1)
Line range hint 14-46: Good use of instance doubles to isolate external dependencies
Using instance_double(ContinueWithFacebookService) effectively isolates the tests from external dependencies, making them faster and more reliable.
| def decoded_payload | ||
| decoded_payload = Base64.urlsafe_decode64(@encoded_payload) | ||
| JSON.parse(decoded_payload) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle exceptions during payload decoding and parsing
The decoded_payload method may raise exceptions if the payload is not valid Base64 or if the JSON is malformed. Adding error handling will make the method more robust.
Apply this diff to handle potential exceptions:
def decoded_payload
- decoded_payload = Base64.urlsafe_decode64(@encoded_payload)
- JSON.parse(decoded_payload)
+ begin
+ decoded_payload = Base64.urlsafe_decode64(@encoded_payload)
+ JSON.parse(decoded_payload)
+ rescue ArgumentError, JSON::ParserError => e
+ raise InvalidSignatureError, "Invalid payload: #{e.message}"
+ end
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def decoded_payload | |
| decoded_payload = Base64.urlsafe_decode64(@encoded_payload) | |
| JSON.parse(decoded_payload) | |
| def decoded_payload | |
| begin | |
| decoded_payload = Base64.urlsafe_decode64(@encoded_payload) | |
| JSON.parse(decoded_payload) | |
| rescue ArgumentError, JSON::ParserError => e | |
| raise InvalidSignatureError, "Invalid payload: #{e.message}" | |
| end | |
| end |
| response = @http_client.get_response(uri) | ||
| JSON.parse(response.body) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for HTTP requests and JSON parsing
In fetch_facebook_user_info, handling HTTP errors and JSON parsing exceptions will improve reliability when external requests fail or return unexpected data.
Apply this diff to enhance error handling:
def fetch_facebook_user_info
uri = URI.parse("https://graph.facebook.com/v8.0/me?fields=email,first_name,last_name,name&access_token=#{@access_token}")
response = @http_client.get_response(uri)
+ unless response.is_a?(Net::HTTPSuccess)
+ raise "Failed to fetch user info from Facebook: #{response.code} #{response.message}"
+ end
+ begin
JSON.parse(response.body)
+ rescue JSON::ParserError => e
+ raise "Invalid JSON response from Facebook: #{e.message}"
+ end
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| response = @http_client.get_response(uri) | |
| JSON.parse(response.body) | |
| end | |
| response = @http_client.get_response(uri) | |
| unless response.is_a?(Net::HTTPSuccess) | |
| raise "Failed to fetch user info from Facebook: #{response.code} #{response.message}" | |
| end | |
| begin | |
| JSON.parse(response.body) | |
| rescue JSON::ParserError => e | |
| raise "Invalid JSON response from Facebook: #{e.message}" | |
| end | |
| end |
| allow(instance).to receive(:valid_signature?).and_return(true) | ||
| allow(instance).to receive(:decoded_payload).and_return(decoded_payload) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid stubbing methods on the object under test (instance)
Stubbing methods on the subject under test can conceal issues and reduce test reliability. Consider refactoring to avoid stubbing private methods or inject dependencies that can be controlled in tests.
One approach is to inject collaborators or use dependency injection to pass in objects that can be mocked.
Alternatively, focus the tests on public behavior without stubbing private methods.
🧰 Tools
🪛 rubocop (1.69.1)
[convention] 21-21: Use receive_messages instead of multiple stubs on lines [22].
(RSpec/ReceiveMessages)
[convention] 21-21: Do not stub methods of the object under test.
(RSpec/SubjectStub)
[convention] 22-22: Use receive_messages instead of multiple stubs on lines [21].
(RSpec/ReceiveMessages)
[convention] 22-22: Do not stub methods of the object under test.
(RSpec/SubjectStub)
…tinueWithFacebookService
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
app/javascript/albums/show.vue (1)
119-135:⚠️ Potential issueAvoid exposing internal error details to users
The same issue exists in both error handlers.
Apply this fix to both error handlers:
- "An error occurred while updating the title: " + error.message, + "An error occurred while updating the title. Please try again later.",- "An error occurred while updating the description: " + error.message, + "An error occurred while updating the description. Please try again later.",
🧹 Nitpick comments (9)
app/services/continue_with_facebook_service.rb (1)
8-15: Consider simplifying the app_secret validationThe current implementation has two layers of validation:
ENV.fetchwill raise KeyError if the environment variable is missingblank?check raises ArgumentError if the value is blankConsider consolidating these checks to avoid redundancy.
-def initialize(access_token, signed_request, app_secret: ENV.fetch('PHOTONIA_FACEBOOK_APP_SECRET'), http_client: Net::HTTP) - @access_token = access_token - @signed_request = signed_request - @app_secret = app_secret - @http_client = http_client - - raise ArgumentError, 'app_secret is required' if @app_secret.blank? +def initialize(access_token, signed_request, app_secret: nil, http_client: Net::HTTP) + @access_token = access_token + @signed_request = signed_request + @app_secret = app_secret.presence || ENV.fetch('PHOTONIA_FACEBOOK_APP_SECRET') + @http_client = http_clientapp/javascript/mixins/description-helper.js (1)
3-3: Consider making the default text configurableThe hardcoded default text could be made configurable to support internationalization (i18n) or different use cases.
-const noDescription = "(no description)"; +const noDescription = import.meta.env.VITE_DEFAULT_NO_DESCRIPTION || "(no description)";app/graphql/mutations/base_mutation.rb (1)
14-19: Consider improving error message clarityThe error messages could be more specific and user-friendly. Also, the error handling could be more structured.
def handle_photo_update_errors(photo) if photo.errors[:title].include?("can't be blank") && photo.errors[:description].include?("can't be blank") - raise GraphQL::ExecutionError, 'Either title or description is required' + raise GraphQL::ExecutionError, 'Please provide either a title or a description for the photo' end - raise GraphQL::ExecutionError, photo.errors.full_messages.join(', ') + error_messages = photo.errors.full_messages + raise GraphQL::ExecutionError, "Validation failed: #{error_messages.join('. ')}" endapp/javascript/albums/show.vue (1)
4-26: Consider adding a loading indicator for better UXWhile the loading state is properly handled to prevent premature rendering, users might benefit from seeing a loading indicator when the content is being fetched.
<div class="title mt-5 mb-0"> + <div v-if="loading" class="has-text-centered"> + <span class="icon is-large"> + <i class="fas fa-spinner fa-pulse"></i> + </span> + </div> <AlbumTitleEditable v-if="!loading && userStore.signedIn && album.canEdit" :album="album" @update-title="updateAlbumTitle" /> <h1 v-else> {{ title }} </h1> </div>app/javascript/albums/album-description-editable.vue (3)
50-55: Remove commented codeRemove the commented-out markdown indicator code block if it's not needed.
1-74: Add ARIA labels for better accessibilityThe buttons and interactive elements should have ARIA labels for better accessibility.
<button class="button" @click="showPreview = false" :class="{ 'is-dark': !showPreview }" + aria-label="Switch to editor mode" ><button class="button" @click="showPreview = true" :class="{ 'is-dark': showPreview }" + aria-label="Switch to preview mode" >
136-153: Consider using trimmed values for comparisonThe description comparison should use trimmed values to prevent updates with only whitespace changes.
const updateDescription = () => { - if (savedDescription != localDescription.value) { + if (savedDescription.trim() !== localDescription.value.trim()) {app/graphql/types/mutation_type.rb (2)
8-11: Consider grouping related mutations into separate types.To improve schema organization and maintainability, consider extracting these mutations into separate types:
- Create an
AlbumMutationTypefor album-related mutations- Create a
PhotoMutationTypefor photo-related mutationsThis would make the schema more organized and easier to maintain as it grows.
Example refactor:
module Types class AlbumMutationType < GraphQL::Schema::Object field :update_description, mutation: Mutations::UpdateAlbumDescription field :update_title, mutation: Mutations::UpdateAlbumTitle end class PhotoMutationType < GraphQL::Schema::Object field :update_description, mutation: Mutations::UpdatePhotoDescription field :update_title, mutation: Mutations::UpdatePhotoTitle end class MutationType < GraphQL::Schema::Object field :album, AlbumMutationType, null: false field :photo, PhotoMutationType, null: false end end🧰 Tools
🪛 rubocop (1.69.1)
[convention] 9-9: Consider moving update_album_description, update_album_title to a new type and adding the
update_albumfield instead(GraphQL/ExtractType)
[convention] 11-11: Consider moving update_photo_description, update_photo_title to a new type and adding the
update_photofield instead(GraphQL/ExtractType)
Line range hint
189-198: Document email update limitation in the GraphQL schema.While there's a code comment explaining why email updates are not supported, this limitation should be documented in the GraphQL schema to inform API consumers.
field :update_user_settings, UserType, null: false do - description 'Update user settings' + description 'Update user settings. Note: Email updates are not supported as they require confirmation.' argument :email, String, 'User email', required: true🧰 Tools
🪛 rubocop (1.69.1)
[warning] 189-189: Unused method argument -
(Lint/UnusedMethodArgument)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/graphql/graphql_query_collection.rb(2 hunks)app/graphql/mutations/base_mutation.rb(1 hunks)app/graphql/mutations/update_album_description.rb(1 hunks)app/graphql/mutations/update_album_title.rb(1 hunks)app/graphql/types/mutation_type.rb(5 hunks)app/javascript/albums/album-description-editable.vue(1 hunks)app/javascript/albums/album-title-editable.vue(1 hunks)app/javascript/albums/show.vue(3 hunks)app/javascript/mixins/description-helper.js(1 hunks)app/javascript/photos/photo-description-editable.vue(2 hunks)app/javascript/photos/show.vue(3 hunks)app/services/continue_with_facebook_service.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/javascript/photos/show.vue
- app/javascript/albums/album-title-editable.vue
- app/graphql/graphql_query_collection.rb
- app/graphql/mutations/update_album_title.rb
🧰 Additional context used
🪛 rubocop (1.69.1)
app/graphql/types/mutation_type.rb
[convention] 9-9: Consider moving update_album_description, update_album_title to a new type and adding the update_album field instead
(GraphQL/ExtractType)
[convention] 11-11: Consider moving update_photo_description, update_photo_title to a new type and adding the update_photo field instead
(GraphQL/ExtractType)
[warning] 189-189: Unused method argument - email.
(Lint/UnusedMethodArgument)
🔇 Additional comments (11)
app/services/continue_with_facebook_service.rb (4)
5-6: LGTM! Well-defined custom error classes.
The custom error classes provide clear error distinction and improve error handling capabilities.
19-24: LGTM! Well-structured method orchestration.
The method provides a clear flow of operations with appropriate error handling delegation.
28-36: LGTM! Secure implementation of signature verification.
The implementation follows security best practices:
- Uses constant-time comparison
- Properly implements HMAC verification
- Clear error handling
38-51:
Add robust error handling for external operations
The methods handling external operations (Base64 decoding, HTTP requests, JSON parsing) should include comprehensive error handling.
Apply these improvements:
def decoded_payload
- decoded_payload = Base64.urlsafe_decode64(@encoded_payload)
- JSON.parse(decoded_payload)
+ begin
+ decoded_payload = Base64.urlsafe_decode64(@encoded_payload)
+ JSON.parse(decoded_payload)
+ rescue ArgumentError
+ raise InvalidSignatureError, "Invalid Base64 encoding"
+ rescue JSON::ParserError
+ raise InvalidSignatureError, "Invalid JSON payload"
+ end
end
def fetch_facebook_user_info
uri = URI.parse("https://graph.facebook.com/v8.0/me?fields=email,first_name,last_name,name&access_token=#{@access_token}")
- response = @http_client.get_response(uri)
- JSON.parse(response.body)
+ response = @http_client.get_response(uri)
+
+ case response
+ when Net::HTTPSuccess
+ JSON.parse(response.body)
+ else
+ raise InvalidUserInfoError, "Facebook API error: #{response.code} - #{response.message}"
+ end
+rescue JSON::ParserError
+ raise InvalidUserInfoError, "Invalid response from Facebook"
+rescue StandardError => e
+ raise InvalidUserInfoError, "Failed to fetch user info: #{e.message}"
endLikely invalid or redundant comment.
app/javascript/albums/show.vue (2)
Line range hint 49-71: LGTM! Clean setup following Vue 3 composition API best practices
The imports are well-organized and the store setup is properly implemented.
84-87: LGTM! Good use of helper functions for consistent formatting
The title and description handling is well-implemented using helper functions, promoting code reuse and consistency.
app/javascript/photos/photo-description-editable.vue (1)
82-82: LGTM! Clean migration to new helper function
The changes correctly implement the new description helper pattern, maintaining consistency across the application.
Also applies to: 93-93
app/javascript/albums/album-description-editable.vue (1)
76-105: LGTM! Well-structured Vue 3 composition API usage
The component setup follows Vue 3 best practices with proper prop definitions and reactive state management.
app/graphql/types/mutation_type.rb (3)
28-28: LGTM! Well-documented argument additions.
The new arguments are properly defined with clear descriptions and appropriate required flags.
Also applies to: 34-34, 65-65, 74-77
216-224: Verify the mailer template name.
The mailer is using new_provider_user template, but based on the context, it might have been renamed from new_social_user (mentioned in past review comments).
✅ Verification successful
The mailer template name new_provider_user is correctly implemented
The verification shows that both the mailer method and templates are consistently using new_provider_user:
- The mailer templates exist at
app/views/admin_mailer/new_provider_user.html.erbandnew_provider_user.text.erb - The mailer class in
app/mailers/admin_mailer.rbdefines the correspondingnew_provider_usermethod - The i18n lookup path is also set to
en.admin_mailer.new_provider_user.subject
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for mailer template definitions
fd -e erb -e haml -e slim . app/views/admin_mailer/
Length of output: 153
Script:
#!/bin/bash
# Let's check the content of the mailer class to verify the method name
rg -g "admin_mailer.rb" "class AdminMailer" -A 10
Length of output: 708
Script:
#!/bin/bash
# Let's check git history to see if there was a rename from new_social_user
git log -p --all --full-history -S "new_social_user"
Length of output: 0
163-174: Verify Google authentication payload thoroughly.
The current implementation only checks email_verified, but it's important to verify other critical fields in the payload:
iss(issuer)exp(expiration time)iat(issued at time)
| const descriptionHelper = (object, loading = ref(false)) => | ||
| loading.value ? "Loading..." : object.value.description || noDescription; | ||
|
|
||
| const descriptionHtmlHelper = (object, loading = ref(false)) => | ||
| loading.value ? "Loading..." : object.value.descriptionHtml || noDescription; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding input validation
The functions assume object.value exists but don't handle cases where object is null/undefined or doesn't have a value property.
const descriptionHelper = (object, loading = ref(false)) =>
- loading.value ? "Loading..." : object.value.description || noDescription;
+ loading.value ? "Loading..." : (object?.value?.description ?? noDescription);
const descriptionHtmlHelper = (object, loading = ref(false)) =>
- loading.value ? "Loading..." : object.value.descriptionHtml || noDescription;
+ loading.value ? "Loading..." : (object?.value?.descriptionHtml ?? noDescription);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const descriptionHelper = (object, loading = ref(false)) => | |
| loading.value ? "Loading..." : object.value.description || noDescription; | |
| const descriptionHtmlHelper = (object, loading = ref(false)) => | |
| loading.value ? "Loading..." : object.value.descriptionHtml || noDescription; | |
| const descriptionHelper = (object, loading = ref(false)) => | |
| loading.value ? "Loading..." : (object?.value?.description ?? noDescription); | |
| const descriptionHtmlHelper = (object, loading = ref(false)) => | |
| loading.value ? "Loading..." : (object?.value?.descriptionHtml ?? noDescription); |
| def authorize(record, action) | ||
| context[:authorize].call(record, action) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling to authorization method
The authorize method should handle cases where the context callable is missing or raises exceptions.
def authorize(record, action)
+ raise GraphQL::ExecutionError, 'Authorization not configured' unless context[:authorize].respond_to?(:call)
context[:authorize].call(record, action)
+rescue StandardError => e
+ raise GraphQL::ExecutionError, "Authorization failed: #{e.message}"
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def authorize(record, action) | |
| context[:authorize].call(record, action) | |
| end | |
| def authorize(record, action) | |
| raise GraphQL::ExecutionError, 'Authorization not configured' unless context[:authorize].respond_to?(:call) | |
| context[:authorize].call(record, action) | |
| rescue StandardError => e | |
| raise GraphQL::ExecutionError, "Authorization failed: #{e.message}" | |
| end |
| description 'Update album description' | ||
|
|
||
| argument :description, String, 'New album description', required: true | ||
| argument :id, String, 'Photo Id', required: true |
There was a problem hiding this comment.
Fix incorrect argument documentation
The documentation for the id argument refers to "Photo Id" but this mutation is for albums.
- argument :id, String, 'Photo Id', required: true
+ argument :id, String, 'Album Id', required: true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| argument :id, String, 'Photo Id', required: true | |
| argument :id, String, 'Album Id', required: true |
| def resolve(id:, description:) | ||
| begin | ||
| album = Album.friendly.find(id) | ||
| rescue ActiveRecord::RecordNotFound | ||
| raise GraphQL::ExecutionError, 'Album not found' | ||
| end | ||
| authorize(album, :update?) | ||
| raise GraphQL::ExecutionError, album.errors.full_messages.join(', ') unless album.update(description: description) | ||
|
|
||
| album | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and transaction management
The resolve method could benefit from better error handling structure and transaction management.
def resolve(id:, description:)
- begin
- album = Album.friendly.find(id)
- rescue ActiveRecord::RecordNotFound
- raise GraphQL::ExecutionError, 'Album not found'
- end
- authorize(album, :update?)
- raise GraphQL::ExecutionError, album.errors.full_messages.join(', ') unless album.update(description: description)
+ Album.transaction do
+ album = Album.friendly.find(id)
+ authorize(album, :update?)
+
+ if album.update(description: description)
+ album
+ else
+ raise GraphQL::ExecutionError, "Failed to update album: #{album.errors.full_messages.join('. ')}"
+ end
+ end
+rescue ActiveRecord::RecordNotFound
+ raise GraphQL::ExecutionError, 'Album not found'
+rescue StandardError => e
+ raise GraphQL::ExecutionError, "Unexpected error: #{e.message}"
+endThis refactor:
- Wraps the operation in a transaction
- Improves error message clarity
- Handles unexpected errors
- Makes the success path more explicit
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def resolve(id:, description:) | |
| begin | |
| album = Album.friendly.find(id) | |
| rescue ActiveRecord::RecordNotFound | |
| raise GraphQL::ExecutionError, 'Album not found' | |
| end | |
| authorize(album, :update?) | |
| raise GraphQL::ExecutionError, album.errors.full_messages.join(', ') unless album.update(description: description) | |
| album | |
| end | |
| def resolve(id:, description:) | |
| Album.transaction do | |
| album = Album.friendly.find(id) | |
| authorize(album, :update?) | |
| if album.update(description: description) | |
| album | |
| else | |
| raise GraphQL::ExecutionError, "Failed to update album: #{album.errors.full_messages.join('. ')}" | |
| end | |
| end | |
| rescue ActiveRecord::RecordNotFound | |
| raise GraphQL::ExecutionError, 'Album not found' | |
| rescue StandardError => e | |
| raise GraphQL::ExecutionError, "Unexpected error: #{e.message}" | |
| end |
Todo:
Relates to #685
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores