Fixed issue #172 #182

ayusht2810 · 2023-04-21T06:35:08Z

Summary

Added a feature to open the incident modal using the slash command and to create incidents using the slash command.
Added a feature in the incident modal to subscribe to the newly created incident.
Added a feature to open the incident modal from post menu actions and auto-fill the "Short description" and "Description" fields with the post data.

Screenshots

Ticket Link

Fixes #172

…gin-servicenow into MI-2984

[MI-2984] Fix issue mattermost#172

codecov-commenter · 2023-04-21T06:47:59Z

Codecov Report

Patch coverage: 44.64% and project coverage change: +1.57% 🎉

Comparison is base (fefb220) 65.22% compared to head (bcfcd59) 66.79%.
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #182      +/-   ##
==========================================
+ Coverage   65.22%   66.79%   +1.57%     
==========================================
  Files          16       16              
  Lines        1990     2027      +37     
==========================================
+ Hits         1298     1354      +56     
+ Misses        653      633      -20     
- Partials       39       40       +1

Files Changed	Coverage Δ
server/plugin/client.go	`76.11% <0.00%> (-1.55%)`	⬇️
server/plugin/plugin.go	`60.00% <0.00%> (-2.07%)`	⬇️
server/plugin/command.go	`76.88% <25.92%> (-4.02%)`	⬇️
server/plugin/util.go	`64.55% <50.00%> (+0.64%)`	⬆️
server/plugin/api.go	`75.76% <100.00%> (+6.75%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

trilopin

Thanks for your work @ayusht2810

here I write some minor things I noticed from a first review:

distance between buttons/labels in modal seems too small

add comments modal has cta button enabled before adding context inside the textarea (update state is correct)
Update-State modal raises in multiple devices at the same time, not only on the window that was triggered (it doesn't happen with the create incident modal)

server/constants/constants.go

server/plugin/command.go

trilopin · 2023-05-09T10:45:43Z

Additionally I get It seems that you are not authorized to manage subscriptions in ServiceNow. Please follow the instructions for setting up user permissions available in the plugin's documentation. The instructions can also be viewed by running the "/servicenow help" command. message when I try to raise the modal with /servicenow create incident (it works fine for post dialog)

I could understand that I lack permissions for subscriptions, but should they apply to create-incident? if so, should it apply for create incident from post dialog too?

…e to phase-2

ayusht2810 · 2023-05-10T12:36:29Z

@trilopin regarding point
3. Update-State modal raises in multiple devices at the same time, not only on the window that was triggered (it doesn't happen with the create incident modal). We fixed this issue previously in a PR but this caused the issue of modals not opening through the Playbooks plugin. You can refer to these issues for more detailed discussion.
#120
#167

trilopin · 2023-05-10T12:56:45Z

@hanzei @mickmister

[MI-3071] Fix review fixes for issue 172 of importing incident featur…

ayusht2810 · 2023-05-10T16:56:10Z

@trilopin fixed the review fixes

lieut-data

Just a handful of comments, but nothing blocking.

server/plugin/api.go

server/plugin/util.go

lieut-data · 2023-05-16T17:17:26Z

server/plugin/client.go

@@ -270,9 +270,13 @@ func (c *client) GetMe(userEmail string) (*serializer.ServiceNowUser, int, error
 }

 func (c *client) CreateIncident(incident *serializer.IncidentPayload) (*serializer.IncidentResponse, int, error) {
+	queryParams := url.Values{
+		constants.SysQueryParamDisplayValue: {"true"},


Can you clarify what this does?

this query param helps in retrieving the display values instead of their IDs for the fields

mickmister

Just a few non-blocking comments

mickmister · 2023-05-17T13:29:37Z

webapp/src/containers/createIncident/createIncidentMenu.tsx

+                    onClick={handleClick}
+                >
+                    <img
+                        src={`${Utils.getBaseUrls().publicFilesUrl}${Constants.SERVICENOW_ICON_URL}`}


We'll want to make sure we take into account the server's Site URL when contacting the plugin via HTTP. At the moment we are just parsing the browser's URL:

mattermost-plugin-servicenow/webapp/src/utils/index.tsx

Lines 18 to 23 in fefb220

const url = new URL(window.location.href);

const baseUrl = `${url.protocol}//${url.host}`;

const pluginUrl = `${baseUrl}/plugins/${pluginId}`;

const pluginApiBaseUrl = `${pluginUrl}/api/v1`;

const mattermostApiBaseUrl = `${baseUrl}/api/v4`;

const publicFilesUrl = `${pluginUrl}/public/`;

server/plugin/command.go

webapp/src/containers/createIncident/index.tsx

DHaussermann · 2023-05-17T21:21:51Z

HI @ayusht2810
I have done a round of testing on this and it seems to be working. I still have some edge cases to explore and such.

I do have a coupe questions with the business logic on the "caller" field. From what I can tell...

When creating an incident not based on a post - the creator may assign any Service Now user as the caller.
When creating an incident based on a post of a user not connected to Service Now - the creator may assign any Service Now user as the caller.
When creating an incident based on a post of a user who is connected to Service Now - The author of the post must be assigned as the caller
Is this ☝️ correct?

If so...

Are we certain we want to enforce that the author of the post must be the caller if they are connected to Service Now? As it is, the short description can be edited from the post content and it may be the case that you want to create an incident based on another users post
When using slash command to create (or creating based on a post from a user not connected to SN) any user can be the caller. The problem is that both the Mattermost username and the ServiceNow username are shown.

In this image - both users are the same person and called will be abel.tutor in SN regardless of which option I select. Can we add some kind of logic to hide Mattermost username if Service Now username is already included? I am worried this could cause alot of "duplication" and confusion.
When caller is locked to the author of the post, can the field be read-only? Even though there is not affect on the SN side, it could be confusing that they can attempt to change this value

…cident in the same channel in which it was created instead of bot dm

…issions function

…sers (#58) * [MI-3182] Fix issue: ServiceNow icon not visible in app bar for new users * [MI-3182] Self review fix * [MI-3182] Review fix Co-authored-by: Manoj Malik <manoj@brightscout.com> --------- Co-authored-by: Manoj Malik <manoj@brightscout.com>

raghavaggarwal2308 · 2023-06-19T12:49:58Z

@DHaussermann The icon issue should be fixed now. Can you please confirm?

DHaussermann

Tested and passed
Icon and all other know issues are now resolved.

LGTM!

DHaussermann · 2023-06-20T21:17:44Z

@raghavaggarwal2308 Thanks for the effort on this. I have 2 related questions that do no block the PR.

Do you know what's happening with the App Bar? This plugin shows in the app bar even when it does not seem enabled for other plugins (see below) I imagine this is intentional. Do you recall anything about this plugin always showing in the app bar?
Can you please clarify for me if this PR will now be cherry picked to a v2.x.x branch?

raghavaggarwal2308 · 2023-06-22T11:15:26Z

@DHaussermann

I cannot understand the issue you are getting here. I tried disabling the app bar and the plugin, but the icon was not visible in both cases. Can you please send us the steps to reproduce the issue?
The issue Modify the Create Incident Message Action Dialog and back-port it to the 2.X stream #172 was to move the create incident dialog from v3.x.x to v2.x.x. So, once this PR is merged we will create a new v2.x.x release.

DHaussermann · 2023-06-23T15:35:00Z

Yes I suppose you have answered my question as that is the issue.
To clarify, when the app bar is disabled, the icon will not show in the channel header the way the other icons do. This must be by design and I simply don't recall. Can you find a technical reason why the icon does not show in the channel header? This was done intentionally correct?
Thanks. I understand 👍

DHaussermann · 2023-06-23T15:35:31Z

@raghavaggarwal2308 sorry I missed the at mention for you ☝️

raghavaggarwal2308 · 2023-06-23T15:43:01Z

@DHaussermann The icon will be visible in the channel header when the app bar is disabled but the icon color will be changed. Please check the below screenshots. Let me know if you are unable to see the icon.

DHaussermann · 2023-06-23T15:54:42Z

@raghavaggarwal2308 Sorry. I was confused. For some unrelated reason the state of the app bar is not showing in my system console.

In my screen above I thought the icon was somehow forcing the app bar on because the other 2 plugins are in the header.
What's actually happening is app bar is enabled but the other two plugin on this server version are too old to move to the app bar.

Yes, SN plugin works as expected and moves to the channel header when needed.
Thanks for looking :)

I believe we can merge this now.

DHaussermann · 2023-06-29T13:34:31Z

@raghavaggarwal2308 please ping mw once we have this merged and packaged into a release for release testing.

Is there unresolved code review conversations still to sort out?

raghavaggarwal2308 · 2023-06-29T13:51:56Z

@mickmister Can you please merge this PR if nothing is pending?

mickmister

I gave the PR another review. In general looks good 👍 Just a few comments for discussion

webapp/src/utils/index.tsx

webapp/src/plugin_constants/index.ts

server/plugin/util.go

webapp/src/containers/createIncident/subscribeToNewIncident.tsx

server/plugin/api_test.go

webapp/src/app.tsx

…ident modal)

…cident modal) (#59) * [MI-3273] Rview fixes on ServiceNow PR mattermost#182 (Add create incident modal) * [MI-3273] Review fix * [MI-3273] Added logic to get the MM site URL from redux and pass it into getBaseUrl function * [MI-3273] Fix ci * [MI-3273] Review fix * [MI-3273] Fix failing testcases * [MI-3273] Review fixes

raghavaggarwal2308 · 2023-08-01T09:56:45Z

@mickmister FIxed the review comments mentioned by you

mickmister

LGTM, just some thoughts for improvement

webapp/src/containers/createIncident/createIncidentMenu.tsx

webapp/src/utils/index.tsx

mickmister · 2023-08-01T10:06:45Z

webapp/src/services/index.ts

+// eslint-disable-next-line import/no-unresolved
+import {BaseQueryApi} from '@reduxjs/toolkit/dist/query/baseQueryTypes';


Is this type still useful if the import can't be resolved?

@mickmister Yes, it's useful, we are using it as the type of parameter in handleBaseQuery function.

But if it can't be resolved, how does it work? Is it the case that your editor can resolve it, but for whatever reason resolver being used by eslint cannot find it?

@mickmister Yes, it is being resolved by the editor but not by eslint.

I wonder why that's the case. I would prefer we didn't have to do this, but it's pretty negligible in this case

webapp/src/index.tsx

…t Modal)

…t Modal) (#60)

raghavaggarwal2308 · 2023-09-15T12:38:12Z

@mickmister Fixed the review comments mentioned by you

mickmister · 2023-09-15T16:30:38Z

@raghavaggarwal2308 please ping mw once we have this merged and packaged into a release for release testing.

Merging due to @DHaussermann's approval above

ayusht2810 and others added 10 commits April 18, 2023 16:57

Fix issue mattermost#172

ae29a12

[MI-2984] Fix ci

44e0ef9

[MI-2984] Remove extra constant

32aa385

[MI-2984] Review fixes

68e589b

[MI-2984] Fix incident post menu option not opening

3a211f3

[MI-2984] Update command error and minor fix

8f6f4eb

[MI-2984] Revert update set

7452b1f

Merge branch 'fix_issue_172' of github.com:Brightscout/mattermost-plu…

6c618fe

…gin-servicenow into MI-2984

[MI-2984] Fix test cases

5e69adb

Merge pull request #52 from Brightscout/MI-2984

e7c9e2f

[MI-2984] Fix issue mattermost#172

trilopin self-requested a review May 8, 2023 18:59

trilopin added the 2: Dev Review Requires review by a core committer label May 8, 2023

trilopin suggested changes May 9, 2023

View reviewed changes

server/constants/constants.go Outdated Show resolved Hide resolved

server/plugin/command.go Outdated Show resolved Hide resolved

[MI-3071] Fix review fixes for issue 172 of importing incident featur…

ff5da60

…e to phase-2

ayusht2810 self-assigned this May 10, 2023

ayusht2810 mentioned this pull request May 10, 2023

[MI-3071] Fix review fixes for issue 172 of importing incident featur… Brightscout/mattermost-plugin-servicenow#55

Merged

Merge pull request #55 from Brightscout/MI-3071

ca9c88b

[MI-3071] Fix review fixes for issue 172 of importing incident featur…

DHaussermann requested review from mickmister and DHaussermann May 15, 2023 17:48

jwilander requested a review from lieut-data May 16, 2023 15:29

lieut-data approved these changes May 16, 2023

View reviewed changes

mickmister approved these changes May 17, 2023

View reviewed changes

raghavaggarwal2308 added 2 commits May 18, 2023 14:29

[MI-3091] Updated logic to post the conformation message of create in…

cdd0c11

…cident in the same channel in which it was created instead of bot dm

[MI-3091] Removed the redundant argument channelType in HsChannelPerm…

aba39dc

…issions function

raghavaggarwal2308 mentioned this pull request Jun 16, 2023

[MI-3182] Fix issue: ServiceNow icon not visible in app bar for new users Brightscout/mattermost-plugin-servicenow#58

Merged

DHaussermann approved these changes Jun 20, 2023

View reviewed changes

DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Jun 23, 2023

mickmister reviewed Jul 5, 2023

View reviewed changes

raghavaggarwal2308 added a commit to Brightscout/mattermost-plugin-servicenow that referenced this pull request Jul 11, 2023

[MI-3273] Rview fixes on ServiceNow PR mattermost#182 (Add create inc…

4b04c41

…ident modal)

mattermost deleted a comment from raghavaggarwal2308 Jul 17, 2023

raghavaggarwal2308 mentioned this pull request Jul 23, 2023

[MI-3273] Review fixes on ServiceNow PR #182 (Add create incident modal) Brightscout/mattermost-plugin-servicenow#59

Merged

mickmister approved these changes Aug 1, 2023

View reviewed changes

raghavaggarwal2308 added a commit to Brightscout/mattermost-plugin-servicenow that referenced this pull request Sep 14, 2023

[MI-3513] Review fixes on ServiceNow PR mattermost#182(Create Inciden…

3b2a0c6

…t Modal)

raghavaggarwal2308 mentioned this pull request Sep 14, 2023

[MI-3513] Review fixes on ServiceNow PR #182(Create Incident Modal) Brightscout/mattermost-plugin-servicenow#60

Merged

raghavaggarwal2308 added 2 commits September 15, 2023 18:06

[MI-3513] Review fixes on ServiceNow PR mattermost#182(Create Inciden…

0d61889

…t Modal) (#60)

Merge branch 'master' into fix_issue_172

0471648

mickmister approved these changes Sep 15, 2023

View reviewed changes

mickmister merged commit bd58e0b into mattermost:master Sep 15, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed issue #172 #182

Fixed issue #172 #182

ayusht2810 commented Apr 21, 2023 •

edited by mickmister

Loading

codecov-commenter commented Apr 21, 2023 •

edited

Loading

trilopin left a comment •

edited

Loading

trilopin commented May 9, 2023

ayusht2810 commented May 10, 2023

trilopin commented May 10, 2023

ayusht2810 commented May 10, 2023

lieut-data left a comment

lieut-data May 16, 2023

ayusht2810 May 30, 2023

mickmister left a comment

mickmister May 17, 2023

DHaussermann commented May 17, 2023

raghavaggarwal2308 commented Jun 19, 2023

DHaussermann left a comment

DHaussermann commented Jun 20, 2023

raghavaggarwal2308 commented Jun 22, 2023 •

edited

Loading

DHaussermann commented Jun 23, 2023

DHaussermann commented Jun 23, 2023

raghavaggarwal2308 commented Jun 23, 2023

DHaussermann commented Jun 23, 2023

DHaussermann commented Jun 29, 2023 •

edited

Loading

raghavaggarwal2308 commented Jun 29, 2023

mickmister left a comment

raghavaggarwal2308 commented Aug 1, 2023

mickmister left a comment

mickmister Aug 1, 2023

raghavaggarwal2308 Sep 14, 2023

mickmister Sep 15, 2023

raghavaggarwal2308 Sep 17, 2023

mickmister Sep 19, 2023

raghavaggarwal2308 commented Sep 15, 2023

mickmister commented Sep 15, 2023

	const url = new URL(window.location.href);
	const baseUrl = `${url.protocol}//${url.host}`;
	const pluginUrl = `${baseUrl}/plugins/${pluginId}`;
	const pluginApiBaseUrl = `${pluginUrl}/api/v1`;
	const mattermostApiBaseUrl = `${baseUrl}/api/v4`;
	const publicFilesUrl = `${pluginUrl}/public/`;

		// eslint-disable-next-line import/no-unresolved
		import {BaseQueryApi} from '@reduxjs/toolkit/dist/query/baseQueryTypes';

Fixed issue #172 #182

Fixed issue #172 #182

Conversation

ayusht2810 commented Apr 21, 2023 • edited by mickmister Loading

Summary

Screenshots

Ticket Link

codecov-commenter commented Apr 21, 2023 • edited Loading

Codecov Report

trilopin left a comment • edited Loading

Choose a reason for hiding this comment

trilopin commented May 9, 2023

ayusht2810 commented May 10, 2023

trilopin commented May 10, 2023

ayusht2810 commented May 10, 2023

lieut-data left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHaussermann commented May 17, 2023

raghavaggarwal2308 commented Jun 19, 2023

DHaussermann left a comment

Choose a reason for hiding this comment

DHaussermann commented Jun 20, 2023

raghavaggarwal2308 commented Jun 22, 2023 • edited Loading

DHaussermann commented Jun 23, 2023

DHaussermann commented Jun 23, 2023

raghavaggarwal2308 commented Jun 23, 2023

DHaussermann commented Jun 23, 2023

DHaussermann commented Jun 29, 2023 • edited Loading

raghavaggarwal2308 commented Jun 29, 2023

mickmister left a comment

Choose a reason for hiding this comment

raghavaggarwal2308 commented Aug 1, 2023

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghavaggarwal2308 commented Sep 15, 2023

mickmister commented Sep 15, 2023

ayusht2810 commented Apr 21, 2023 •

edited by mickmister

Loading

codecov-commenter commented Apr 21, 2023 •

edited

Loading

trilopin left a comment •

edited

Loading

raghavaggarwal2308 commented Jun 22, 2023 •

edited

Loading

DHaussermann commented Jun 29, 2023 •

edited

Loading