-
Notifications
You must be signed in to change notification settings - Fork 21
feat: enhance RavenColonial integration and add Project Creation #383
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
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -148,3 +148,4 @@ MEMORY_BANK.md | |
| .DS_Store | ||
| scripts/ | ||
| data/tmp.py | ||
| bounce.ps1 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,147 @@ | ||
| # BGS-Tally Improvements | ||
|
|
||
| ## Changes Summary | ||
|
|
||
| This update includes two improvements: | ||
| 1. **Auto-Sync RavenColonial** - Automatically enable RCSync when assigned to a project | ||
| 2. **UI Worker Fix** - Prevent startup AttributeError when no system is loaded | ||
|
|
||
| --- | ||
|
|
||
| ## 1. Auto-Sync RavenColonial Feature | ||
|
|
||
| ### Overview | ||
| This change automatically creates/tracks systems and enables RavenColonial sync (`RCSync`) when a commander docks at a construction ship or receives colonization data, **if**: | ||
| 1. A project exists on RavenColonial for that system/market | ||
| 2. The current commander is assigned to that project | ||
|
|
||
| **Key Improvement**: Systems are now **automatically added to BGS-Tally tracking** when you dock at a construction ship where you're assigned to a RavenColonial project - no manual system creation required! | ||
|
|
||
| ### Changes Made | ||
|
|
||
| #### A. `bgstally/ravencolonial.py` | ||
| Added new method `check_auto_sync(system_address, market_id)`: | ||
| - Checks if a project exists via `/api/system/{systemAddress}/{marketId}` | ||
| - Verifies commander assignment via `/api/cmdr/{cmdr}` | ||
| - Handles both string buildIds and project objects from API | ||
| - Returns `True` if both conditions are met | ||
|
|
||
| **Location**: Lines 179-238 | ||
|
|
||
| #### B. `bgstally/colonisation.py` | ||
| Added auto-sync checks in three locations: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is a question for Aussi and the BGS-Tally users –– should we just assume RC integration for anyone who has a key setup or only use the integration if they enable it?
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's always a good design principle to give maximum control to the users, and I've always tried to do this wherever possible in BGS-Tally. Currently you can enable / disable synching with RC system-by-system. However, what is the actual benefit to a user of disabling RC sync for a system? If they have gone to the trouble of setting up RC with a key etc, would a user ever want to select a system to not be synched... I suspect the answer is no, in which case I don't see a problem with auto-synching. Happy to hear an opposing argument though, if there is a valid reason.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My 2 cents: I think if a user has gone through the trouble to figure out where the RC API Key is in the first place, it's likely a safe assumption they will want that integration in BGS Tally. That being said, I'm curious why the API key is needed? As far as I can tell all of the API calls being made don't require the API key to be entered, but I may be missing something. |
||
|
|
||
| **1. ColonisationConstructionDepot Event (Lines 181-199)** | ||
| - **Auto-creates system** if it doesn't exist and RavenColonial project + assignment confirmed | ||
| - Enables RCSync for existing systems when conditions are met | ||
| - Prevents "Invalid ColonisationConstructionDepot event (no system)" warnings | ||
|
|
||
| **2. Docked Event - New System Creation (Lines 216-222)** | ||
| - Auto-enables RCSync when creating a new system during construction ship dock | ||
| - Checks project assignment before enabling sync | ||
|
|
||
| **3. Docked Event - Existing Systems (Lines 234-238)** | ||
| - Enables RCSync for already-tracked systems if not yet enabled | ||
|
|
||
| ## API Endpoints Used | ||
|
|
||
| 1. **Check Project**: `GET /api/system/{systemAddress}/{marketId}` | ||
| - Returns project data if it exists | ||
| - Returns 404 if no project found | ||
|
|
||
| 2. **Check Commander Projects**: `GET /api/cmdr/{cmdr}` | ||
| - Returns list of projects the commander is assigned to | ||
|
|
||
| ## Testing Steps | ||
|
|
||
| ### Prerequisites | ||
| 1. Have RavenColonial API key configured in BGS-Tally settings | ||
| 2. Be assigned to a project on RavenColonial.com | ||
|
|
||
| ### Test Case 1: Auto-create System (New System) | ||
| 1. **Remove the system from BGS-Tally** if it exists (or use a fresh colonization system) | ||
| 2. Dock at a construction ship where you're assigned to a RavenColonial project | ||
| 3. **Expected**: System automatically added to BGS-Tally with RCSync enabled | ||
| 4. **Expected Logs**: | ||
| - "Auto-creating system [system] with RCSync enabled" | ||
| - "Commander [cmdr] is assigned to project [buildId], enabling auto-sync" | ||
| 5. **Verify**: System appears in BGS-Tally Colonisation window with sync icon (🔄 button visible) | ||
|
|
||
| ### Test Case 2: Auto-enable for Existing System | ||
| 1. Have a system already tracked in BGS-Tally with RCSync **disabled** | ||
| 2. Dock at the construction ship or receive depot data | ||
| 3. **Expected**: RCSync automatically enables | ||
| 4. **Expected Log**: "Auto-enabling RCSync for [system]" | ||
| 5. **Verify**: Contributions now sync to RavenColonial | ||
|
|
||
| ### Test Case 3: No Auto-create (Not Assigned) | ||
| 1. Dock at a construction ship where you're **NOT** assigned to the project | ||
| 2. **Expected**: System is NOT created, warning logged | ||
| 3. **Expected Log**: "Commander [name] is not assigned to project [id]" | ||
| 4. **Expected**: "Invalid ColonisationConstructionDepot event (no system)" warning (normal behavior) | ||
|
|
||
| ### Test Case 4: No Auto-create (No Project) | ||
| 1. Dock at a construction ship with **no RavenColonial project** | ||
| 2. **Expected**: System is NOT created | ||
| 3. **Expected Logs**: | ||
| - "No project found for system [id], market [id]" | ||
| - "Invalid ColonisationConstructionDepot event (no system)" warning (normal behavior) | ||
|
|
||
| ## Debug Logging | ||
|
|
||
| Watch for these log messages: | ||
|
|
||
| **Success Messages:** | ||
| - `"Auto-creating system {system} with RCSync enabled"` - New system auto-created | ||
| - `"Auto-enabling RCSync for newly created system {system}"` - RCSync enabled on dock | ||
| - `"Auto-enabling RCSync for {system}"` - RCSync enabled for existing system | ||
| - `"Commander {cmdr} is assigned to project {buildId}, enabling auto-sync"` - Assignment confirmed | ||
|
|
||
| **Info/Warning Messages:** | ||
| - `"No commander set, cannot check auto-sync"` - Commander name not loaded yet | ||
| - `"No project found for system {systemAddress}, market {marketId}"` - No RC project exists | ||
| - `"Commander {cmdr} is not assigned to project {buildId}"` - Not assigned to this project | ||
| - `"Error checking project: {status_code}"` - API error when checking project | ||
| - `"Error in check_auto_sync: {error}"` - Exception during auto-sync check | ||
|
|
||
| ## Potential Issues | ||
|
|
||
| 1. **API Key Required**: If no API key is configured, requests may fail | ||
| 2. **Network Timeouts**: 5-second timeout on API calls may be too short in some cases | ||
| 3. **Duplicate Checks**: Both dock and depot events may trigger - this is intentional for coverage | ||
| 4. **Rate Limiting**: Multiple API calls per dock event - consider caching if this becomes an issue | ||
|
|
||
| --- | ||
|
|
||
| ## 2. UI Worker Startup Fix | ||
|
|
||
| ### Overview | ||
| Fixed an AttributeError that occurred during EDMC startup when the UI worker tried to access system data before it was loaded. | ||
|
|
||
| ### Changes Made | ||
|
|
||
| #### `bgstally/ui.py` (Line 491-493) | ||
| Added null check for `current_system` before accessing its attributes: | ||
| ```python | ||
| # Fix: Check if current_system is not None before accessing its attributes | ||
| # This prevents AttributeError during startup when no system is loaded yet | ||
| if current_system is not None: | ||
| system_tick: str = current_system.get('TickTime') | ||
| ``` | ||
|
|
||
| ### Error Fixed | ||
| ``` | ||
| AttributeError: 'NoneType' object has no attribute 'get' | ||
| at ui.py line 490: system_tick: str = current_system.get('TickTime') | ||
| ``` | ||
|
|
||
| **Impact**: Prevents harmless but annoying error message during EDMC startup. | ||
|
|
||
| --- | ||
|
|
||
| ## Future Improvements | ||
|
|
||
| 1. Cache project/commander assignment to reduce API calls | ||
| 2. Add UI notification when auto-sync is enabled | ||
| 3. Allow users to opt-out of auto-sync via settings | ||
| 4. Consider async API calls to avoid blocking | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -148,16 +148,27 @@ def journal_entry(self, cmdr, is_beta, sys, station, entry, state) -> None: | |
| return | ||
|
|
||
| system = self.find_system({'StarSystem' : self.current_system, 'SystemAddress': self.system_id}) | ||
| Debug.logger.debug(f"ColonisationContribution - system found: {system != None}, Hidden: {system.get('Hidden', True) if system else 'N/A'}, RCSync: {system.get('RCSync', False) if system else 'N/A'}") | ||
|
|
||
| if system != None and system.get('Hidden', True) == False and system.get('RCSync', False) == True: | ||
| found_progress = False | ||
| for progress in self.progress: | ||
| Debug.logger.debug(f"Checking progress: MarketID {progress.get('MarketID')} vs {self.market_id}, ProjectID: {progress.get('ProjectID')}") | ||
| if progress.get('MarketID', None) == self.market_id and progress.get('ProjectID', None) != None: | ||
| Debug.logger.info(f"Calling record_contribution for ProjectID {progress.get('ProjectID')}") | ||
| rc.record_contribution(progress.get('ProjectID', 0), entry.get('Contributions', [])) | ||
| found_progress = True | ||
|
|
||
| # Just in case we don't have the ProjectID on the build, add it now. | ||
| b:dict|None = self.find_build(system, {'MarketID': self.market_id}) | ||
| if b != None and b.get('ProjectID', None) == None: | ||
| self.modify_build(system, b.get('BuildID', ''), {'ProjectID': progress.get('ProjectID', None)}) | ||
| break | ||
|
|
||
| if not found_progress: | ||
| Debug.logger.warning(f"No progress record found with ProjectID for MarketID {self.market_id}") | ||
| else: | ||
| Debug.logger.warning(f"ColonisationContribution skipped - conditions not met") | ||
|
|
||
| case 'ColonisationSystemClaim': | ||
| if not self.current_system or not self.system_id: | ||
|
|
@@ -177,10 +188,96 @@ def journal_entry(self, cmdr, is_beta, sys, station, entry, state) -> None: | |
| return | ||
|
|
||
| system = self.find_system({'StarSystem': self.current_system, 'SystemAddress' : self.system_id}) | ||
|
|
||
| # If system doesn't exist, check if we should auto-create it based on RavenColonial assignment | ||
| if system == None: | ||
| Debug.logger.warning(f"Invalid ColonisationConstructionDepot event (no system): {entry}") | ||
| return | ||
| if self.system_id and self.market_id and self.current_system: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two things. First, a ColonisationContribution cannot be done until after you've docked. Docking at a colonisation site will create the system & the build so doing it here is unnecessary. After docking you'll have ColonisationConstructionDepot events which will create the progress. Second, BGS-Tally isn't just an RC client, it can function entirely without RC. This code assumes that if someone has a system in RC they want it synced with BGS-Tally. I chose to only sync with RC if the user specifically enables it.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've been testing the existing release for a couple more hours this morning and feel that the automatic creation of RC projects is actually causing more problems than it solves. What I'm seeing is:
I believe changing the creation to a manual process would fix these issues and make for a more intuitive user experience, and also have a feeling that's why the original developer didn't make it automatic in SRVSurvey. How I think changing this to manual would resolve the issues:
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The workflow for project creation on a pre-planned site looks like this: When the user hits the Create Project button, RC API sends the system body information and any preplanned sites to populate those fields in the UI. User selects their prebuilt site here:
And when they select the site, the Build Name, Construction Category, Model and Body are all filled in. They hit Create and are whisked away to the build page.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Maybe... I really dislike making users enter information that the system already has. It's unnecessary workload and opens the possibility of user error resulting in duplication.
From talking to him, originally projects & sites were entirely distinct and SRVSurvey was just a project client. This has been changing over time though when we last talked his feeling was that projects & sites still weren't properly connecting.
There is something that does need to be fixed which may address much of the above. When RC added commander authentication (using FDev auth) it changed to allow open and closed systems. BGS-Tally currently only checks if the commander is the architect. It doesn't check if the commander has permission to edit the system even if they aren't the architect which it should. |
||
| project_id = RavenColonial(self).check_auto_sync(self.system_id, self.market_id) | ||
| if project_id: | ||
| Debug.logger.info(f"Auto-creating system {self.current_system} with RCSync enabled and ProjectID {project_id}") | ||
| system = self.find_or_create_system({'StarSystem': self.current_system, 'SystemAddress': self.system_id}) | ||
| self.modify_system(system, { | ||
| 'RCSync': True, | ||
| 'Architect': self.cmdr, | ||
| 'Hidden': False | ||
| }) | ||
|
|
||
| # Also create the build for this construction depot | ||
| Debug.logger.info(f"Auto-creating build for market {self.market_id}") | ||
| build = self.find_or_create_build(system, { | ||
| 'MarketID': self.market_id, | ||
| 'Name': self.station if self.station else 'Construction Site', | ||
| 'Body': self.body, | ||
| 'State': BuildState.PROGRESS, | ||
| 'Track': True, | ||
| 'ProjectID': project_id | ||
| }) | ||
| else: | ||
| Debug.logger.warning(f"Invalid ColonisationConstructionDepot event (no system): {entry}") | ||
| return | ||
| else: | ||
| Debug.logger.warning(f"Invalid ColonisationConstructionDepot event (no system): {entry}") | ||
| return | ||
|
|
||
| # Check if we should auto-enable RCSync for existing systems | ||
| project_id = None | ||
| if system.get('RCSync', False) == False and self.system_id and self.market_id: | ||
| project_id = RavenColonial(self).check_auto_sync(self.system_id, self.market_id) | ||
| if project_id: | ||
| Debug.logger.info(f"Auto-enabling RCSync for {system.get('StarSystem', 'Unknown')} with ProjectID {project_id}") | ||
| self.modify_system(system, { | ||
| 'RCSync': True, | ||
| 'Hidden': False | ||
| }) | ||
|
|
||
| # Ensure Hidden=False for systems with RCSync enabled | ||
| if system.get('RCSync', False) == True and system.get('Hidden', True) == True: | ||
| Debug.logger.info(f"Setting Hidden=False for RCSync-enabled system {system.get('StarSystem', 'Unknown')}") | ||
| self.modify_system(system, {'Hidden': False}) | ||
|
|
||
| # Ensure the build exists for this market | ||
| build = self.find_build(system, {'MarketID': self.market_id}) | ||
| if build == None: | ||
| # Try to find by name if not found by MarketID | ||
| build = self.find_build(system, {'Name': self.station}) | ||
| if build: | ||
| Debug.logger.info(f"Found build by name, adding MarketID {self.market_id}") | ||
| self.modify_build(system, build.get('BuildID', ''), {'MarketID': self.market_id}) | ||
| else: | ||
| Debug.logger.info(f"Creating missing build for market {self.market_id}") | ||
| build = self.find_or_create_build(system, { | ||
| 'MarketID': self.market_id, | ||
| 'Name': self.station if self.station else 'Construction Site', | ||
| 'Body': self.body, | ||
| 'State': BuildState.PROGRESS, | ||
| 'Track': True | ||
| }) | ||
|
|
||
| # Ensure MarketID is set on the build | ||
| if build and build.get('MarketID', None) != self.market_id: | ||
| Debug.logger.info(f"Setting MarketID {self.market_id} on build {build.get('BuildID', 'unknown')}") | ||
| self.modify_build(system, build.get('BuildID', ''), {'MarketID': self.market_id}) | ||
|
|
||
| # Ensure tracking is enabled | ||
| if build and build.get('Track', False) == False: | ||
| Debug.logger.info(f"Enabling tracking for build {build.get('BuildID', 'unknown')}") | ||
| self.modify_build(system, build.get('BuildID', ''), {'Track': True}) | ||
| self.dirty = True | ||
| self.bgstally.ui.window_progress.update_display() | ||
|
|
||
| # Set ProjectID on build if we have it and it's missing | ||
| if build and project_id and build.get('ProjectID', None) == None: | ||
| Debug.logger.info(f"Setting ProjectID {project_id} on build {build.get('BuildID', 'unknown')}") | ||
| self.modify_build(system, build.get('BuildID', ''), {'ProjectID': project_id}) | ||
|
|
||
| progress:dict = self.find_or_create_progress(self.market_id) | ||
|
|
||
| # Set ProjectID on progress record if we have it | ||
| if project_id and progress.get('ProjectID', None) != project_id: | ||
| Debug.logger.info(f"Setting ProjectID {project_id} on progress record for market {self.market_id}") | ||
| progress['ProjectID'] = project_id | ||
| self.dirty = True | ||
|
|
||
| if progress.get('ProjectID', None) != None and entry.get('ProjectID', None) == None: | ||
| entry['ProjectID'] = progress.get('ProjectID', None) | ||
| self.update_progress(self.market_id, entry) | ||
|
|
@@ -195,7 +292,18 @@ def journal_entry(self, cmdr, is_beta, sys, station, entry, state) -> None: | |
| # Colonisation ship is always the first build. Construction site can be any build | ||
| if '$EXT_PANEL_ColonisationShip' in f"{self.station}" or 'Construction Site' in f"{self.station}": | ||
| Debug.logger.debug(f"Docked at construction site. Finding/creating system and build") | ||
| if system == None: system = self.find_or_create_system({'StarSystem': self.current_system, 'SystemAddress' : self.system_id}) | ||
| if system == None: | ||
| system = self.find_or_create_system({'StarSystem': self.current_system, 'SystemAddress' : self.system_id}) | ||
| # Check if we should auto-enable RCSync for newly created system | ||
| if self.system_id and self.market_id: | ||
| project_id = RavenColonial(self).check_auto_sync(self.system_id, self.market_id) | ||
| if project_id: | ||
| Debug.logger.info(f"Auto-enabling RCSync for newly created system {self.current_system} with ProjectID {project_id}") | ||
| self.modify_system(system, { | ||
| 'RCSync': True, | ||
| 'Architect': self.cmdr, | ||
| 'Hidden': False | ||
| }) | ||
| build = self.find_or_create_build(system, {'MarketID': self.market_id, 'Name': self.station, 'Body': self.body}) | ||
| build_state = BuildState.PROGRESS | ||
| # Complete station so find it and add/update as appropriate. | ||
|
|
@@ -208,6 +316,17 @@ def journal_entry(self, cmdr, is_beta, sys, station, entry, state) -> None: | |
| if system == None or build == None: | ||
| return | ||
|
|
||
| # Check if we should auto-enable RCSync when docking at construction ship | ||
| if '$EXT_PANEL_ColonisationShip' in f"{self.station}" or 'Construction Site' in f"{self.station}": | ||
| if system.get('RCSync', False) == False and self.system_id and self.market_id: | ||
| project_id = RavenColonial(self).check_auto_sync(self.system_id, self.market_id) | ||
| if project_id: | ||
| Debug.logger.info(f"Auto-enabling RCSync for {system.get('StarSystem', 'Unknown')} on dock") | ||
| self.modify_system(system, { | ||
| 'RCSync': True, | ||
| 'Hidden': False | ||
| }) | ||
|
|
||
| # Update the system details | ||
| if system.get('Name', None) == None: system['Name'] = self.current_system | ||
|
|
||
|
|
||


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.
You can get this information without making an additional call (/api/system/{systemAddress}/{marketId} includes a commanders section).
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.
Ahh thank you! I'm going to leave this unresolved so that I can remember to fix that if these changes intend to get implemented