-
Notifications
You must be signed in to change notification settings - Fork 82
Added support for Gitlab #128
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: master
Are you sure you want to change the base?
Conversation
Reviewer's GuideThis PR adds GitLab support alongside existing GitHub integration by introducing a platform selector in the popup UI, branching the data-fetch logic in the background script, implementing GitLab API calls (projects, issues, MRs) with corresponding rendering functions, and updating storage, UI flows, and manifest formatting accordingly. Sequence Diagram for GitLab Data Fetching ProcesssequenceDiagram
actor User
participant PopupUI as popup.html
participant ScrumHelperScript as scrumHelper.js
participant GitLabAPI
User->>PopupUI: Selects GitLab platform
User->>PopupUI: Enters GitLab username
User->>PopupUI: Clicks "Generate Report"
PopupUI->>ScrumHelperScript: allIncluded('popup')
ScrumHelperScript->>ScrumHelperScript: Read platform ('gitlab') and gitlabUsername from storage
ScrumHelperScript->>ScrumHelperScript: fetchGitlabData()
ScrumHelperScript->>GitLabAPI: GET /users/{gitlabUsername}/projects
GitLabAPI-->>ScrumHelperScript: Projects list
ScrumHelperScript->>ScrumHelperScript: Store project IDs and names (gitlabProjectIdToName)
loop For each project
ScrumHelperScript->>GitLabAPI: GET /projects/{projectId}/issues (filtered by author, dates)
GitLabAPI-->>ScrumHelperScript: Issues list for project
ScrumHelperScript->>GitLabAPI: GET /projects/{projectId}/merge_requests (filtered by author, dates)
GitLabAPI-->>ScrumHelperScript: Merge Requests list for project
end
ScrumHelperScript->>GitLabAPI: GET /users?username={gitlabUsername}
GitLabAPI-->>ScrumHelperScript: GitLab user data
ScrumHelperScript->>ScrumHelperScript: Consolidate all issues (gitlabIssuesData)
ScrumHelperScript->>ScrumHelperScript: Consolidate all MRs (gitlabPrsReviewData)
ScrumHelperScript->>ScrumHelperScript: writeGitlabIssuesPrs()
ScrumHelperScript->>ScrumHelperScript: writeGitlabPrsReviews()
ScrumHelperScript->>ScrumHelperScript: writeScrumBody()
ScrumHelperScript->>PopupUI: Display generated report
Entity Relationship Diagram for Chrome Storage UpdateserDiagram
CHROME_STORAGE_LOCAL {
string githubUsername
string gitlabUsername "(new)"
string projectName
boolean enableToggle
string startingDate
string endingDate
boolean showOpenLabel
string userReason
boolean lastWeekContribution
boolean yesterdayContribution
string platform "(new) e.g., 'github' or 'gitlab'"
}
Class Diagram for scrumHelper.js Module ChangesclassDiagram
class scrumHelper.js {
<<JavaScript Module>>
+Object gitlabProjectIdToName
+string gitlabUsername
+string platform
+Array gitlabIssuesData
+Array gitlabPrsReviewData
+Object gitlabUserData
+Object gitlabPrsReviewDataProcessed
+allIncluded(outputTarget: string) : void // Modified to handle 'platform'
+fetchGitlabData() : void // New: Fetches data from GitLab API
+writeGitlabIssuesPrs() : void // New: Formats GitLab issues for report
+writeGitlabPrsReviews() : void // New: Formats GitLab MRs for report
+getStartOfDayISO(dateString: string) : string // New helper
+getEndOfDayISO(dateString: string) : string // New helper
}
note for scrumHelper.js "Key variables and functions added/modified for GitLab integration"
Class Diagram for main.js Module ChangesclassDiagram
class main.js {
<<JavaScript Module>>
+HTMLElement gitlabUsernameElement
+NodeList platformRadios
+HTMLElement githubUsernameContainer
+HTMLElement gitlabUsernameContainer
+handleBodyOnLoad() : void // Modified to load/set platform and gitlabUsername
+handlePlatformChange(platform: string) : void // New: Handles UI changes on platform selection
+handleGitlabUsernameChange() : void // New: Saves gitlabUsername to storage
}
note for main.js "Handles new UI elements for platform selection and GitLab username"
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @Preeti9764 - I've reviewed your changes - here's some feedback:
- Consider abstracting the duplicated GitHub/GitLab data-fetching and rendering logic into reusable helper functions to reduce code duplication.
- The
allIncluded
function is very large and mixes UI, storage, and API logic—split it into smaller, single-purpose functions or modules for better readability and maintainability. - Some AJAX calls (e.g. merge requests) lack error handlers—add consistent error handling for every API request to ensure failures are logged or surfaced to the user.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider abstracting the duplicated GitHub/GitLab data-fetching and rendering logic into reusable helper functions to reduce code duplication.
- The `allIncluded` function is very large and mixes UI, storage, and API logic—split it into smaller, single-purpose functions or modules for better readability and maintainability.
- Some AJAX calls (e.g. merge requests) lack error handlers—add consistent error handling for every API request to ensure failures are logged or surfaced to the user.
## Individual Comments
### Comment 1
<location> `src/scripts/scrumHelper.js:676` </location>
<code_context>
+ });
+ }
+
+ function writeGitlabIssuesPrs() {
+ if (!gitlabIssuesData) return;
+
</code_context>
<issue_to_address>
GitLab issues are only added to lastWeekArray
Currently, planned GitLab tasks are not added to nextWeekArray, unlike the GitHub logic. Please update the classification to ensure planned tasks are included in nextWeekArray as well.
</issue_to_address>
### Comment 2
<location> `src/scripts/scrumHelper.js:563` </location>
<code_context>
+
+ function fetchGitlabData() {
+ // First get user's projects
+ var projectsUrl = `https://gitlab.com/api/v4/users/${gitlabUsername}/projects?per_page=100`;
+ console.log('[GitLab] Fetching projects for user:', gitlabUsername, 'URL:', projectsUrl);
+
</code_context>
<issue_to_address>
Fetching projects by username only returns public projects
To retrieve all projects, including private and group ones, first resolve the user's numeric ID using `/users?username=`, then use `/users/:id/projects` for the fetch.
Suggested implementation:
```javascript
async function fetchGitlabData() {
// First resolve the user's numeric ID
const userIdUrl = `https://gitlab.com/api/v4/users?username=${gitlabUsername}`;
console.log('[GitLab] Resolving user ID for:', gitlabUsername, 'URL:', userIdUrl);
try {
const userResponse = await fetch(userIdUrl);
if (!userResponse.ok) {
throw new Error(`Failed to fetch user ID for ${gitlabUsername}`);
}
const users = await userResponse.json();
if (!users.length) {
throw new Error(`No user found for username: ${gitlabUsername}`);
}
const userId = users[0].id;
const projectsUrl = `https://gitlab.com/api/v4/users/${userId}/projects?per_page=100`;
console.log('[GitLab] Fetching projects for user ID:', userId, 'URL:', projectsUrl);
// Continue with fetching projects as before, using projectsUrl
// ... (rest of your logic here)
} catch (error) {
console.error('[GitLab] Error fetching user projects:', error);
}
```
- If the rest of your code expects `fetchGitlabData` to be synchronous, you may need to update its callers to handle a Promise (i.e., use `await` or `.then()`).
- Move the rest of your project-fetching logic (that used to use `projectsUrl`) inside the `try` block after the new `projectsUrl` is defined.
</issue_to_address>
### Comment 3
<location> `src/scripts/scrumHelper.js:554` </location>
<code_context>
}, 500);
+
+ // Helper functions for ISO date range
+ function getStartOfDayISO(dateString) {
+ return dateString + 'T00:00:00Z';
+ }
</code_context>
<issue_to_address>
getStartOfDayISO/getEndOfDayISO assume valid input
Malformed ISO strings may result if dateString is empty or incorrectly formatted. Please validate the input or handle invalid dates appropriately.
</issue_to_address>
### Comment 4
<location> `src/scripts/main.js:215` </location>
<code_context>
chrome.storage.local.set({ userReason: value });
}
+
+function handlePlatformChange(platform) {
+ chrome.storage.local.set({ platform: platform });
+
</code_context>
<issue_to_address>
Switching platform doesn't clear the other username field
Consider clearing the unused username field and its stored value when switching platforms to prevent retaining outdated data.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
function handlePlatformChange(platform) {
chrome.storage.local.set({ platform: platform });
=======
function handlePlatformChange(platform) {
chrome.storage.local.set({ platform: platform });
// Clear the unused username field and its stored value
if (platform === "github") {
// Clear GitLab username field in the UI if it exists
const gitlabInput = document.getElementById("gitlabUsername");
if (gitlabInput) gitlabInput.value = "";
chrome.storage.local.set({ gitlabUsername: "" });
} else if (platform === "gitlab") {
// Clear GitHub username field in the UI if it exists
const githubInput = document.getElementById("githubUsername");
if (githubInput) githubInput.value = "";
chrome.storage.local.set({ githubUsername: "" });
}
>>>>>>> REPLACE
</suggested_fix>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/scripts/scrumHelper.js
Outdated
}); | ||
} | ||
|
||
function writeGitlabIssuesPrs() { |
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.
issue (bug_risk): GitLab issues are only added to lastWeekArray
Currently, planned GitLab tasks are not added to nextWeekArray, unlike the GitHub logic. Please update the classification to ensure planned tasks are included in nextWeekArray as well.
src/scripts/main.js
Outdated
function handlePlatformChange(platform) { | ||
chrome.storage.local.set({ platform: platform }); | ||
|
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.
suggestion (bug_risk): Switching platform doesn't clear the other username field
Consider clearing the unused username field and its stored value when switching platforms to prevent retaining outdated data.
function handlePlatformChange(platform) { | |
chrome.storage.local.set({ platform: platform }); | |
function handlePlatformChange(platform) { | |
chrome.storage.local.set({ platform: platform }); | |
// Clear the unused username field and its stored value | |
if (platform === "github") { | |
// Clear GitLab username field in the UI if it exists | |
const gitlabInput = document.getElementById("gitlabUsername"); | |
if (gitlabInput) gitlabInput.value = ""; | |
chrome.storage.local.set({ gitlabUsername: "" }); | |
} else if (platform === "gitlab") { | |
// Clear GitHub username field in the UI if it exists | |
const githubInput = document.getElementById("githubUsername"); | |
if (githubInput) githubInput.value = ""; | |
chrome.storage.local.set({ githubUsername: "" }); | |
} |
src/scripts/main.js
Outdated
@@ -1,16 +1,22 @@ | |||
var enableToggleElement = document.getElementById('enable'); | |||
var githubUsernameElement = document.getElementById('githubUsername'); | |||
var gitlabUsernameElement = document.getElementById('gitlabUsername'); |
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.
issue (code-quality): Use const
or let
instead of var
. (avoid-using-var
)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
@hpdang @vedansh-5, can you please review it for now? It is currently fetching all the GitLab activities of your public repositories, and I would appreciate your suggestions on this. |
|
@Preeti9764 yes we should have an option to allow users to select their organization, and it can go under Setting. By default if user doesn't select an organization, scrum helper will pull all git activities of user across all organizations? Also could you please incorporate the latest changes with the setting button into this PR? |
yes, i will add organisation button in setting which is in are latest ui and set to fetch all activites as default if any organisation is not selected. @hpdang do we need this default for both github as well as gitlab cause for doing it for github i need to make changes in api calling of github . |
what is our current logic with github, we don't have to select any org at the moment, it checks FOSSASIA only? |
yes , the default org is fossasia, we can change it to other organisation by making little changes in code which is also added in readme. |
This org name input feature can be tackled in a new PR. @hpdang please share your views on this. thanks |
@hpdang This would be a nice addition along with org input. We can fetch all the user data within a date range when no org name is set. |
@vedansh-5 i have made changes for the same and can push it with this PR. I have also seen some changes since last pr#133 is megred the false is again seen in the subject and scrum subject is not loaded in email and yahoo. can you please check that . Thanks! |
@Preeti9764 I understand you've made code changes to implement the same, which is okay for this PR we wouldn't want to undo your work, ffr-please create another PR if you solve an existing bug or add a new feature, this makes it easier to keep track of the code changes and fixes also its easier to rebase our existing PRs when a new PR is merged, Thanks. |
This probably will be because of rebasing, I'll look into it. Thanks for pointing out! |
We should not introduce too many changes in one PR. Let’s break down the changes and address them one by one:
What do you think? @Preeti9764 @vedansh-5 |
@hpdang ,yes, this could be done . I think this current pr works fine for issue 3. in this all the GitLab activities are fetched for your public repositories. Will raise the other 2 issues differently and we can work on that. I will resolve the conflicts soon so this will ready. |
Affirmative, it would be better the mentioned way. |
3112449
to
6082a6e
Compare
@hpdang @vedansh-5 Check this pr once i have resolved the confilicts and checked it. It is working fine for me and is ready to merge. |
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.
okay i will solve the gap issue
I don't think dropdown make sense for two plateforms, we can make that change as we add support of different plateforms.Thanks! |
Extension to new platforms would be out in upcoming weeks, then we'll have to change the hiding logic for those, if we use drop-down list from now on, then we can just plug in the new platforms there. And since you are introducing this feature it makes more sense to include it - here than to change it later, it'll also get users accustomed of the new UI and would look better aesthetically. Thanks! |
📌 Fixes
Fixes #119
📝 Summary of Changes
Summary by Sourcery
Enable the extension to generate Scrum reports from either GitHub or GitLab by adding a platform toggle and integrating GitLab’s API to fetch projects, issues, and merge requests.
New Features:
Enhancements: