Skip to content
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

[GH-61]: RHS Support #300

Merged
merged 32 commits into from
Mar 2, 2023
Merged

[GH-61]: RHS Support #300

merged 32 commits into from
Mar 2, 2023

Conversation

raghavaggarwal2308
Copy link
Contributor

@raghavaggarwal2308 raghavaggarwal2308 commented Jun 3, 2022

Summary

Added functionality to displays the details of users's open merge requests, merge requests needing review, assignments and unreads (todos) in the right-hand sidebar onclick of the buttons(respective icons) in the left-hand sidebar/team sidebar.

  • Created a new "SidebarRight" component and registered it as a RHS component and stored the "showRHSPluginAction" action in redux to use whenever needed to display the sidebar.
  • Stored the state of RHS in redux to display the different possible views.
  • Created API endpoint to fetch additional details of merge requests to display the merge request's commit status and number of approvals.
  • Added warning icon after merge request title if there are conflicts in the merge request.

Screenshots

  • RHS with open merge requests
    Screenshot from 2022-06-09 22-17-30

  • RHS with unreads (todos)
    Screenshot from 2022-06-03 23-35-03

  • RHS with no items to display
    Screenshot from 2022-06-03 23-40-38

Ticket

Fixes #61 (Adding RHS support and Improve RHS UI and content)

* Added RHS Support
1. Opening right hand sidebar onclick of LHS buttons.
2. Displaying users merge requests, assignments, reviews and todos.
3. Displaying Labels on reviews , merge requests and assignments.

* Adding RHS to gitlab
1. Displaying unread todos in RHS.

* Review fixes
1. String in sigle quotes.
2. indentation.
3.removing extra spaces

* Review Fixes
1. Fixing dependencies.
2. Adding EOF.

* used string literals

* use nullish coalescing operator

* Error in api
1. Error while fetching reviews and pr's.

* review fixes

* review fixes

* Review fixes.
1. Upgraded dependency versions.
2. Convertes sider_right.jsx to .tsx

* Fixed linting errors and changed some eslint config
Added a new dependency eslint-plugin-react-hooks and used that in .eslintrc.json
Added some new rules in eslintrc related to typescript from the Github plugin

* Fixed some more spacing issues

* Apply suggestions from code review

* Review fixes
1. Upgraded mattermost-redux version.
2. Moved constants.

* Changed file extensions

Co-authored-by: Manoj <manoj@brightscout.com>
@mattermod
Copy link
Contributor

Hello @raghavaggarwal2308,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

Per the Mattermost Contribution Guide, we need to add you to the list of approved contributors for the Mattermost project.

Please help complete the Mattermost contribution license agreement?
Once you have signed the CLA, please comment with /check-cla and confirm that the CLA check is green.

This is a standard procedure for many open source projects.

Please let us know if you have any questions.

We are very happy to have you join our growing community! If you're not yet a member, please consider joining our Contributors community channel to meet other contributors and discuss new opportunities with the core team.

@raghavaggarwal2308 raghavaggarwal2308 changed the title [GL-61]: RHS Support #103 [GL-61]: RHS Support Jun 3, 2022
@codecov
Copy link

codecov bot commented Jun 3, 2022

Codecov Report

Patch coverage: 6.66% and project coverage change: -0.14 ⚠️

Comparison is base (2534105) 32.40% compared to head (3336353) 32.27%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #300      +/-   ##
==========================================
- Coverage   32.40%   32.27%   -0.14%     
==========================================
  Files          21       21              
  Lines        3419     3433      +14     
==========================================
  Hits         1108     1108              
- Misses       2201     2215      +14     
  Partials      110      110              
Impacted Files Coverage Δ
server/api.go 23.14% <6.66%> (-0.69%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@raghavaggarwal2308
Copy link
Contributor Author

/check-cla

@raghavaggarwal2308 raghavaggarwal2308 changed the title [GL-61]: RHS Support [GH-61]: RHS Support Jun 3, 2022
* Fixed issue with label details
1. Created a new func to get label details from api

* Moved struct to top of file

* Review fixes
1. Added logic to get label details in "yourAssignments" func.

* Fixes name of struct
* Added RHS Support
1. Opening right hand sidebar onclick of LHS buttons.
2. Displaying users merge requests, assignments, reviews and todos.
3. Displaying Labels on reviews , merge requests and assignments.

* Adding RHS to gitlab
1. Displaying unread todos in RHS.

* Review fixes
1. String in sigle quotes.
2. indentation.
3.removing extra spaces

* Displayed PR status

* Improving RHS UI
1. Added status icon to PR's and reviews.
2. Added details of how many reviewers approved the PR.

* fixed error in command_test file

* Review Fixes
1. Fixing dependencies.
2. Adding EOF.

* used string literals

* use nullish coalescing operator

* Error in api
1. Error while fetching reviews and pr's.

* review fixes

* review fixes

* Review fixes.
1. Upgraded dependency versions.
2. Convertes sider_right.jsx to .tsx

* Fixed issue with label details
1. Created a new func to get label details from api

* Review fixes
1. Fixed warning in "npm run lint" command
2. Logged error while fetching pr details.
3. Removed extra variables.

* Indentation fixes

* Added space after semicolons

* Review fixes
1. Added label details functionality to assignments.
2. Fixed error messages.

* Review fixes
1. Removed trailing spaces.
2. Removed logger.Logger from "fetchPrDetails" func.

* Review fixes
1. Fixed spacing in error message.

* Review fixes
1. Passed logger to function
@raghavaggarwal2308
Copy link
Contributor Author

Merged the following changes is this PR:

  • Created API endpoint to fetch additional details of merge requests to display the merge request's commit status and number of approvals.
  • Added warning icon after merge request title if there are conflicts in the merge request.

Ticket

Fixes #61 (Improve RHS UI and content)

@mickmister mickmister added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Jun 12, 2022
webapp/.eslintrc.json Outdated Show resolved Hide resolved
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
// See LICENSE.txt for license information.

const config = {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for this babel changes, what is the reason behind adding this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The files I added are for typescript. Earliar there was not typescript file in the code, so no loaders were configured for .ts and .tsx files. That's why I added babel-loader and this file contains the babel-loader configurations.

"@primer/octicons-react": "10.1.0",
"core-js": "3.6.5",
"csstype": "3.0.3",
"mattermost-redux": "5.27.0",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed there was a very specific version of redux. Was it for a specific reason? my question is like, is there any risk of regression for upgrading to 5.27?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a type defined in mattermost-redux named "Theme" it was not present in the version specified earliar, that's why updated it to this version.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, if there are not risk of regressions with this update I am ok with it.

@@ -179,8 +242,8 @@ export function getGitlabUser(userID) {
const user = getState()[`plugins-${id}`].gitlabUsers[userID];
if (
user &&
user.last_try &&
Date.now() - user.last_try < GITLAB_USER_GET_TIMEOUT_MILLISECONDS
user.last_try &&
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -168,6 +213,24 @@ export function getUnreads() {
};
}

/**
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

import {formatTimeSince} from '../../utils/date_utils';
import {GitlabItemsProps, Label} from "../../types/gitlab_items"

export const notificationReasons: Record<string, string> = {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mickmister what is the process to define/validate the wording for the plugin?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cwarnermm Do you mind looking through the wording of the user-facing strings shown here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cwarnermm Would you be able to take a look at the user-facing strings here?

return (
<div key={item.id} style={style.container}>
<div>
<strong>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 nice use of semantic html

@mattermod
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

@mickmister
Copy link
Member

@raghavaggarwal2308 Please make sure these changes are compatible with the chanegs from #333

@raghavaggarwal2308
Copy link
Contributor Author

@mickmister Tested, the changes are compatible with the changes of PR #333

@mickmister mickmister removed 2: Dev Review Requires review by a core committer Lifecycle/1:stale labels Feb 8, 2023
@mickmister
Copy link
Member

@DHaussermann This is ready for QA review

@mattermost-build
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

@hanzei
Copy link
Collaborator

hanzei commented Feb 21, 2023

/update-branch

@mattermost-build
Copy link
Contributor

We don't have permissions to update this PR, please contact the submitter to apply the update.

@hanzei
Copy link
Collaborator

hanzei commented Feb 21, 2023

@DHaussermann Gentle reminder to review the PR

@DHaussermann
Copy link

Hi @raghavaggarwal2308
This is looking good! I only have 1 small thing.

Is it easy to add 1 whitespace character between the end of the link and the beginning of the remaining data where the project name is?

image

image

@raghavaggarwal2308
Copy link
Contributor Author

@DHaussermann Fixed 👍🏽 (Added whitespace character between the end of the link and the beginning of the remaining data)

Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and passed

  • Ensured all record types now show in LHS
  • Link are functional
  • Styling shows as expected based on what is available in GitHub
  • Ensured labels also show as applicable
  • Tested in light and dark themes
  • Tested in browser and dektop app

LGTM!
Huge thanks for implementing this @raghavaggarwal2308 🎉 This is a huge improvement.

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Mar 2, 2023
@hanzei hanzei merged commit d041aba into mattermost:master Mar 2, 2023
@mickmister mickmister mentioned this pull request May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Backport github plugin improvements