Check PTO From Sheets File In The Organization's Drive #135
Conversation
…s to run correct files from the implementation made
WalkthroughThe changes migrate PTO data integration from Notion to Google Sheets. This includes adding new Google Sheets API dependencies, updating environment variable examples, introducing new Ruby classes for fetching and formatting PTO from Sheets, updating use case execution scripts, modifying schedule paths, and replacing or removing Notion-based tests and logic with Google Sheets equivalents. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant UseCaseScript
participant GoogleSheetsFetcher
participant GoogleSheetsAPI
Scheduler->>UseCaseScript: Trigger fetch PTO script (Drive)
UseCaseScript->>GoogleSheetsFetcher: Initialize with creds & spreadsheet ID
GoogleSheetsFetcher->>GoogleSheetsAPI: Authenticate & fetch PTO rows
GoogleSheetsAPI-->>GoogleSheetsFetcher: Return PTO data
GoogleSheetsFetcher->>UseCaseScript: Return formatted PTO descriptions
UseCaseScript->>Scheduler: Complete PTO update
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
juanhiginio
requested review from
FelipeGuzmanSierra and
L-Zuluaga
and removed request for
L-Zuluaga
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (1)
.env.example (1)
69-70: Consider documenting the relationship with existing Notion PTO variables.The new Google Sheets environment variables are correctly added. However, notice that Notion-related PTO variables (lines 17-22) are still present. Consider adding comments to clarify which variables are for the new Google Sheets implementation vs. legacy Notion setup, or remove unused Notion variables if they're no longer needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.env.example(1 hunks)Gemfile(1 hunks)spec/implementations/pto/fetch_pto_from_drive_spec.rb(1 hunks)spec/implementations/pto/fetch_pto_from_notion_spec.rb(0 hunks)spec/implementations/pto_next_week/fetch_next_week_pto_from_drive_spec.rb(1 hunks)spec/implementations/pto_next_week/fetch_next_week_pto_from_notion_spec.rb(0 hunks)src/implementations/fetch_next_week_pto_from_drive.rb(1 hunks)src/implementations/fetch_pto_from_drive.rb(1 hunks)src/use_cases_execution/pto/fetch_pto_from_drive.rb(1 hunks)src/use_cases_execution/pto/humanize_pto_workspace.rb(1 hunks)src/use_cases_execution/pto_next_week/fetch_next_week_pto_from_drive_for_workspace.rb(1 hunks)src/use_cases_execution/pto_next_week/humanize_next_week_pto_workspace.rb(1 hunks)src/use_cases_execution/schedules.rb(1 hunks)
💤 Files with no reviewable changes (2)
- spec/implementations/pto/fetch_pto_from_notion_spec.rb
- spec/implementations/pto_next_week/fetch_next_week_pto_from_notion_spec.rb
🧰 Additional context used
📓 Path-based instructions (3)
`spec/implementations/**/*`: Tests should mirror the structure of src/implementations/ and be comprehensive for each use case.
spec/implementations/**/*: Tests should mirror the structure of src/implementations/ and be comprehensive for each use case.
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
spec/implementations/pto/fetch_pto_from_drive_spec.rbspec/implementations/pto_next_week/fetch_next_week_pto_from_drive_spec.rb
`src/use_cases_execution/schedules.rb`: Centralize all scheduling configuration for use cases in this file.
src/use_cases_execution/schedules.rb: Centralize all scheduling configuration for use cases in this file.
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
src/use_cases_execution/schedules.rb
`src/implementations/**/*.rb`: Reusable bot implementations should inherit from Bas::Bot::Base.
src/implementations/**/*.rb: Reusable bot implementations should inherit from Bas::Bot::Base.
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
src/implementations/fetch_pto_from_drive.rbsrc/implementations/fetch_next_week_pto_from_drive.rb
🪛 RuboCop (1.75.5)
spec/implementations/pto/fetch_pto_from_drive_spec.rb
[convention] 15-15: Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.
(Layout/FirstHashElementIndentation)
[convention] 27-27: Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
(Layout/FirstArrayElementIndentation)
[convention] 28-28: Indent the right bracket the same as the first position after the preceding left parenthesis.
(Layout/FirstArrayElementIndentation)
[convention] 35-35: Indent the first argument one step more than the start of the previous line.
(Layout/FirstArgumentIndentation)
[convention] 36-36: Indent ) to column 6 (not 4)
(Layout/ClosingParenthesisIndentation)
[convention] 39-39: Indent the first argument one step more than the start of the previous line.
(Layout/FirstArgumentIndentation)
[convention] 40-40: Indent ) to column 6 (not 4)
(Layout/ClosingParenthesisIndentation)
[convention] 48-48: Line is too long. [125/120]
(Layout/LineLength)
spec/implementations/pto_next_week/fetch_next_week_pto_from_drive_spec.rb
[convention] 27-27: Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.
(Layout/FirstArrayElementIndentation)
[convention] 28-28: Indent the right bracket the same as the first position after the preceding left parenthesis.
(Layout/FirstArrayElementIndentation)
[convention] 49-49: Line is too long. [133/120]
(Layout/LineLength)
[convention] 54-54: Line is too long. [191/120]
(Layout/LineLength)
🪛 GitHub Actions: CI
spec/implementations/pto/fetch_pto_from_drive_spec.rb
[error] 15-15: RuboCop Layout/FirstHashElementIndentation: Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is. [Correctable]
[error] 27-27: RuboCop Layout/FirstArrayElementIndentation: Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis. [Correctable]
[error] 28-28: RuboCop Layout/FirstArrayElementIndentation: Indent the right bracket the same as the first position after the preceding left parenthesis. [Correctable]
[error] 35-35: RuboCop Layout/FirstArgumentIndentation: Indent the first argument one step more than the start of the previous line. [Correctable]
[error] 36-36: RuboCop Layout/ClosingParenthesisIndentation: Indent ) to column 6 (not 4). [Correctable]
[error] 39-39: RuboCop Layout/FirstArgumentIndentation: Indent the first argument one step more than the start of the previous line. [Correctable]
[error] 40-40: RuboCop Layout/ClosingParenthesisIndentation: Indent ) to column 6 (not 4). [Correctable]
[error] 48-48: RuboCop Layout/LineLength: Line is too long. [125/120] [Correctable]
spec/implementations/pto_next_week/fetch_next_week_pto_from_drive_spec.rb
[error] 27-27: RuboCop Layout/FirstArrayElementIndentation: Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis. [Correctable]
[error] 28-28: RuboCop Layout/FirstArrayElementIndentation: Indent the right bracket the same as the first position after the preceding left parenthesis. [Correctable]
[error] 49-49: RuboCop Layout/LineLength: Line is too long. [133/120] [Correctable]
[error] 54-54: RuboCop Layout/LineLength: Line is too long. [191/120]
🔇 Additional comments (15)
Gemfile (1)
50-50: LGTM! Google Sheets API gem added correctly.The
google-apis-sheets_v4gem addition supports the PTO migration from Notion to Google Sheets. The version constraint~> 0.45.0is appropriate and the positioning among other Google API gems is logical.src/use_cases_execution/pto/humanize_pto_workspace.rb (1)
16-16: To confirm that the implementation class is defined and correctly tagged, let’s inspect its source:#!/bin/bash # Display the implementation file to verify class name and tag usage sed -n '1,200p' src/implementations/fetch_pto_from_drive.rbsrc/use_cases_execution/pto_next_week/humanize_next_week_pto_workspace.rb (1)
16-16: ```shell
#!/bin/bashCheck if the Google Sheets implementation class defines the correct tag
rg -n "tag:" src/implementations/fetch_next_week_pto_from_drive.rb
</details> <details> <summary>src/use_cases_execution/schedules.rb (2)</summary> `38-38`: **Verify the new PTO script file exists.** The schedule path update from notion to drive correctly reflects the migration to Google Sheets. Ensure the new PTO script file exists at the updated path: ```shell #!/bin/bash # Description: Verify the new PTO drive script exists # Check if the new script file exists fd "fetch_pto_from_drive.rb" src/use_cases_execution/pto/
45-46: Verify the new next week PTO script file exists.The schedule path update from notion to drive is consistent with the migration pattern.
Ensure the new next week PTO script file exists at the updated path:
#!/bin/bash # Description: Verify the new next week PTO drive script exists # Check if the new script file exists fd "fetch_next_week_pto_from_drive_for_workspace.rb" src/use_cases_execution/pto_next_week/src/use_cases_execution/pto/fetch_pto_from_drive.rb (1)
7-27: LGTM! Clean migration from Notion to Google SheetsThe migration is well-executed with consistent updates to the require path (line 7), environment variables (lines 18-19), database tag (line 14), and class instantiation (line 27). The error handling and shared storage patterns remain appropriately unchanged.
spec/implementations/pto/fetch_pto_from_drive_spec.rb (1)
9-62: Well-structured test with comprehensive mockingThe test properly follows the required directory structure mirroring
src/implementations/and includes comprehensive mocking of Google Sheets API, authentication, and shared storage dependencies. The test logic correctly verifies the execute method functionality.src/use_cases_execution/pto_next_week/fetch_next_week_pto_from_drive_for_workspace.rb (1)
7-26: LGTM! Consistent migration patternThe migration follows the same clean pattern as the main PTO fetching script, with appropriate updates to the require path (line 7), environment variables (lines 18-19), database tag (line 14), and class instantiation (line 26). Good consistency across the codebase.
spec/implementations/pto_next_week/fetch_next_week_pto_from_drive_spec.rb (1)
9-61: Well-designed test with proper mocking strategyThe test correctly follows the required directory structure and includes comprehensive mocking with a good testing strategy that stubs both the
processmethod and verifiesexecutefunctionality. The inclusion of emoji categories in test data is a nice touch for real-world scenario testing.src/implementations/fetch_next_week_pto_from_drive.rb (2)
8-146: Excellent implementation following coding guidelinesThis implementation properly inherits from
Bas::Bot::Baseas required and demonstrates excellent software engineering practices:
- Comprehensive error handling with safe date parsing
- Well-structured private methods with clear responsibilities
- Robust validation and filtering logic
- Human-readable message formatting with proper date handling
- Support for various PTO types including half-day periods
- Proper Google Sheets API integration with service account authentication
The code is well-documented and maintainable.
130-133: Verify next week calculation logicThe next week range calculation may not work as expected. If today is Monday,
((1 - today.wday) % 7)equals 0, giving you this week's Monday instead of next week's.Let me verify this calculation logic:
#!/bin/bash # Verify the next week calculation logic cat > verify_date_calc.rb << 'EOF' require 'date' def next_week_range(today) monday = today + ((1 - today.wday) % 7) monday..(monday + 6) end # Test various days test_dates = [ Date.new(2025, 6, 2), # Monday Date.new(2025, 6, 3), # Tuesday Date.new(2025, 6, 4), # Wednesday Date.new(2025, 6, 7), # Saturday Date.new(2025, 6, 8) # Sunday ] test_dates.each do |date| range = next_week_range(date) puts "#{date.strftime('%A %B %d, %Y')} -> Next week: #{range.begin.strftime('%A %B %d')} to #{range.end.strftime('%A %B %d')}" end EOF ruby verify_date_calc.rbConsider this corrected version for truly getting next week:
def next_week_range - monday = today + ((1 - today.wday) % 7) + monday = today + (7 - today.wday + 1) monday..(monday + 6) endsrc/implementations/fetch_pto_from_drive.rb (4)
35-35: LGTM! Proper inheritance from Bas::Bot::Base.The class correctly inherits from
Bas::Bot::Baseas required by the coding guidelines for reusable bot implementations.
103-107: LGTM! Good error handling for date parsing.The
safe_parse_datemethod properly handles date parsing errors and returnsnilfor invalid dates, which is appropriate for the filter logic.
109-115: LGTM! Clean and readable description building logic.The description building method creates human-readable PTO descriptions with proper formatting and handles different scenarios well.
117-124: LGTM! Solid weekend handling logic.The return date computation correctly handles weekends by rolling forward to the next weekday, which is appropriate for business context.
…op recommendations
…ions and making service account credentials readable from environment variable
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/implementations/fetch_pto_from_drive.rb (1)
122-126: Consider making date format configurable.The method only supports one date format (
%m/%d/%Y), which might be limiting if Google Sheets contain dates in different formats.def safe_parse_date(str) - Date.strptime(str.strip, '%m/%d/%Y') + format = process_options[:date_format] || '%m/%d/%Y' + Date.strptime(str.strip, format) rescue ArgumentError nil end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.env.example(1 hunks)spec/implementations/pto/fetch_pto_from_drive_spec.rb(1 hunks)spec/implementations/pto_next_week/fetch_next_week_pto_from_drive_spec.rb(1 hunks)src/implementations/fetch_next_week_pto_from_drive.rb(1 hunks)src/implementations/fetch_pto_from_drive.rb(1 hunks)src/use_cases_execution/pto/fetch_pto_from_drive.rb(1 hunks)src/use_cases_execution/pto_next_week/fetch_next_week_pto_from_drive_for_workspace.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- .env.example
- src/use_cases_execution/pto_next_week/fetch_next_week_pto_from_drive_for_workspace.rb
- spec/implementations/pto/fetch_pto_from_drive_spec.rb
- spec/implementations/pto_next_week/fetch_next_week_pto_from_drive_spec.rb
- src/use_cases_execution/pto/fetch_pto_from_drive.rb
- src/implementations/fetch_next_week_pto_from_drive.rb
🧰 Additional context used
📓 Path-based instructions (1)
`src/implementations/**/*.rb`: Reusable bot implementations should inherit from Bas::Bot::Base.
src/implementations/**/*.rb: Reusable bot implementations should inherit from Bas::Bot::Base.
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
src/implementations/fetch_pto_from_drive.rb
🧠 Learnings (1)
src/implementations/fetch_pto_from_drive.rb (2)
Learnt from: CR
PR: kommitters/bas_use_cases#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T18:39:30.369Z
Learning: Most use cases in this Ruby automation system follow a standardized pipeline: Fetch, Format, Notify, Garbage Collect. Each step reads from and writes to a PostgreSQL-backed shared storage using tags to track processing stages.
Learnt from: juanpabloxk
PR: kommitters/bas_use_cases#104
File: src/use_cases_execution/schedules.rb:119-127
Timestamp: 2025-06-06T20:08:44.949Z
Learning: In the bas_use_cases repository, the user juanpabloxk prefers to keep schedule constants consolidated in a single file (src/use_cases_execution/schedules.rb) rather than extracting them to separate files, even when it exceeds RuboCop's 100-line module limit.
🔇 Additional comments (5)
src/implementations/fetch_pto_from_drive.rb (5)
1-48: LGTM! Well-documented class following coding guidelines.The class properly inherits from
Bas::Bot::Baseas required and includes comprehensive documentation with usage examples. The process method follows the expected pattern of returning a hash with success/failure status.
64-84: Excellent error handling and configuration improvements.The implementation addresses previous review comments by:
- Adding proper error handling for Google Sheets API calls
- Removing hardcoded subject email from authentication
- Making credentials configurable through process_options
- Providing clear error messages for missing configuration
86-108: Clean and well-structured row processing logic.The implementation uses appropriate Ruby patterns:
filter_mapfor efficient processing of potentially invalid rows- Proper validation of required fields before processing
- Clear separation of concerns between validation and processing
128-164: Excellent business logic implementation.The description building methods demonstrate thoughtful handling of various PTO scenarios:
- Proper weekend handling for return dates
- Support for half-day PTOs with AM/PM distinctions
- Fallback handling for empty categories
- Human-readable date formatting
166-169: Simple and clean accessor method.Follows the established pattern for configuration accessors in the class.
Description
This is a new feature that covers the use cases of pto and pto_next_week so that they are no longer queried in a Notion DB but directly in the entity's own Google Sheets file. This implementation was motivated by the desire to contribute to the migration of Notion to Google Drive and Google Workspace, both in terms of users and data, as well as the implementation and querying of this data.
New dependencies required:
gem ‘google-apis-sheets_v4’, '~> 0.45.0'
Fixes # (issue)
Adjust PTO Notification Use Case
Type of change
Feature
Please delete options that are not relevant.
Checklist
Summary by CodeRabbit
New Features
Chores
Tests