[mm-66, mm-12, mm-47] - resolves all three issues #85

maisnamrajusingh · 2021-07-29T05:57:32Z

Summary

This PR adds the date selection UI, the ability to requeue and item for the next meeting and updates the agenda items on deletion. The PRs were supposed to go out separately but things got intertwined because things would need to change in the subsequent issues that we worked on. Please bear this one @mickmister.

Ticket Link

#12 - Re-queue an item for next meeting
#66 - Improve date format selection UI
#47 - Agenda item number should get updated if the items of the agenda are deleted

All of issues mattermost-community#66, mattermost-community#12 and mattermost-community#47 in come commit

maisnamrajusingh · 2021-07-29T06:00:22Z

@mickmister sorry the lint failed. Let me fix this first before you review it.

codecov · 2021-07-30T09:35:55Z

Codecov Report

Merging #85 (2b97830) into master (2f22484) will decrease coverage by 4.83%.
The diff coverage is 5.83%.

@@            Coverage Diff             @@
##           master      #85      +/-   ##
==========================================
- Coverage   26.03%   21.19%   -4.84%     
==========================================
  Files           7        7              
  Lines         315      453     +138     
==========================================
+ Hits           82       96      +14     
- Misses        216      339     +123     
- Partials       17       18       +1

Impacted Files	Coverage Δ
server/command.go	`0.00% <0.00%> (ø)`
server/utils.go	`62.74% <0.00%> (-31.20%)`	⬇️
server/meeting.go	`62.06% <50.00%> (-0.98%)`	⬇️
server/main.go	`0.00% <0.00%> (ø)`
server/manifest.go	`100.00% <0.00%> (ø)`
server/configuration.go	`0.00% <0.00%> (ø)`
server/plugin.go	`25.74% <0.00%> (+3.16%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f22484...2b97830. Read the comment docs.

mickmister

Hi @maisnamrajusingh, I've skimmed over the PR, and I think it would be much more straightforward to review this if the PRs were split up. We can talk through this on Mattermost or video call to see how we can split up the code. What do you think? Sorry for my late response on this.

mickmister · 2021-08-02T15:06:40Z

webapp/src/actions/index.js

+    if (!currentChannel) {
+        currentChannel = await Client4.getChannelByName(currentTeamId, 'town-square');
+    }


Mattermost has loaded if we are running this command right?

(R1) Most of this function snippet is borrowed from https://github.com/larkox/mattermost-plugin-badges/blob/709bf26a2700a7b1830d9dd436e30d0fc08bd187/webapp/src/actions/actions.ts#L105

should we skip this check?

mickmister · 2021-08-02T15:06:47Z

webapp/src/actions/index.js

+    try {
+        //@ts-ignore Typing in mattermost-redux is wrong
+        return Client4.executeCommand(command, args);


Can we use the action at mattermost-redux/actions/integrations.ts instead of accessing the client directly here?

Sure, will replace with suggested one.

mickmister · 2021-08-02T15:07:35Z

webapp/src/actions/index.js

+
+    try {
+        //@ts-ignore Typing in mattermost-redux is wrong


Can you explain why the typing is wrong? What error is it giving, and how does it differ from what you expect?

mickmister · 2021-08-02T15:09:08Z

webapp/src/components/meeting_settings/meeting_settings.jsx

@@ -17,8 +17,9 @@ export default class MeetingSettingsModal extends React.PureComponent {
        super(props);

        this.state = {
-            hashtag: '{{Jan02}}',
+            hashtagPrefix: 'Prefix',


Looks like everything in this file is scoped to #66

mickmister · 2021-08-02T15:09:48Z

webapp/src/index.js

@@ -1,5 +1,5 @@

-import {updateSearchTerms, updateSearchResultsTerms, updateRhsState, performSearch, openMeetingSettingsModal} from './actions';
+import {updateSearchTerms, updateSearchResultsTerms, updateRhsState, performSearch, openMeetingSettingsModal, requeueItem} from './actions';


This file is scoped to #12

mickmister · 2021-08-02T15:10:03Z

webapp/src/actions/index.js

+}
+
+export function requeueItem(itemId) {


This file is scoped to #12

mickmister · 2021-08-02T15:10:34Z

server/meeting.go

 		}
+		//---- requeue Logic


Can we remove this line?

mickmister · 2021-08-10T03:50:41Z

@maisnamrajusingh @sanjaydemansol Since the code is mixed together as it currently stands, it's difficult to compartmentalize which features are being reviewed with each block of code. If you can, please take another attempt at separating the code out separately. If the features were implemented in succession, then you can use the different commits as base branches for the sequential PRs.

For instance, the first commit says it fixes #66. Does this mean that commit can be used as a PR for that issue? Then you can use that branch as the base branch for the next PR, and do the same for the next feature. It's unfortunate that the completed work is currently in this form, but this is a way to separate it out into individual reviews. Ideally the code is just separated because these features don't seem very related.

@sanjaydemansol please mention me on replies to comments on GitHub. Since I'm not the PR author, I won't get notifications for the comments unless I'm mentioned.

maisnamrajusingh · 2021-08-12T05:55:31Z

@maisnamrajusingh @sanjaydemansol Since the code is mixed together as it currently stands, it's difficult to compartmentalize which features are being reviewed with each block of code. If you can, please take another attempt at separating the code out separately. If the features were implemented in succession, then you can use the different commits as base branches for the sequential PRs.

For instance, the first commit says it fixes #66. Does this mean that commit can be used as a PR for that issue? Then you can use that branch as the base branch for the next PR, and do the same for the next feature. It's unfortunate that the completed work is currently in this form, but this is a way to separate it out into individual reviews. Ideally the code is just separated because these features don't seem very related.

@sanjaydemansol please mention me on replies to comments on GitHub. Since I'm not the PR author, I won't get notifications for the comments unless I'm mentioned.

I think Sanjay is already doing this but I'll just confirm. Will close the PR for now.

Sanjay and others added 5 commits July 13, 2021 16:47

Improve date format selection UI mattermost-community#66

3395a30

Fixes mattermost-community#12

752d468

added missed static files

1ae256a

Fixes mattermost-community#47

e9447a8

Merge pull request #7 from demansoltech/issue-47

1698681

All of issues mattermost-community#66, mattermost-community#12 and mattermost-community#47 in come commit

maisnamrajusingh requested a review from jfrerich as a code owner July 29, 2021 05:57

Delete webapp/src/components/modals/channel_subscriptions directory

2b97830

jfrerich requested a review from mickmister July 31, 2021 16:18

This was linked to issues Aug 2, 2021

Re-queue an item for next meeting #12

Open

Agenda item number should get updated if the items of the agenda are deleted #47

Closed

Improve date format selection UI #66

Open

hanzei added the 2: Dev Review Requires review by a core committer label Aug 2, 2021

mickmister reviewed Aug 2, 2021

View reviewed changes

maisnamrajusingh closed this Aug 12, 2021

hanzei added Invalid This doesn't seem right and removed 2: Dev Review Requires review by a core committer labels Sep 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mm-66, mm-12, mm-47] - resolves all three issues #85

[mm-66, mm-12, mm-47] - resolves all three issues #85

maisnamrajusingh commented Jul 29, 2021 •

edited by jfrerich

Loading

maisnamrajusingh commented Jul 29, 2021

codecov bot commented Jul 30, 2021 •

edited

Loading

mickmister left a comment

mickmister Aug 2, 2021

sanjaydemansol Aug 4, 2021 •

edited

Loading

sanjaydemansol Aug 4, 2021

mickmister Aug 2, 2021

sanjaydemansol Aug 4, 2021

mickmister Aug 2, 2021

sanjaydemansol Aug 4, 2021

mickmister Aug 2, 2021

mickmister Aug 2, 2021

mickmister Aug 2, 2021

mickmister Aug 2, 2021

sanjaydemansol Aug 4, 2021

mickmister commented Aug 10, 2021

maisnamrajusingh commented Aug 12, 2021

		@@ -1,5 +1,5 @@

		import {updateSearchTerms, updateSearchResultsTerms, updateRhsState, performSearch, openMeetingSettingsModal} from './actions';
		import {updateSearchTerms, updateSearchResultsTerms, updateRhsState, performSearch, openMeetingSettingsModal, requeueItem} from './actions';

[mm-66, mm-12, mm-47] - resolves all three issues #85

[mm-66, mm-12, mm-47] - resolves all three issues #85

Conversation

maisnamrajusingh commented Jul 29, 2021 • edited by jfrerich Loading

Summary

Ticket Link

maisnamrajusingh commented Jul 29, 2021

codecov bot commented Jul 30, 2021 • edited Loading

Codecov Report

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanjaydemansol Aug 4, 2021 • edited Loading

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

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

mickmister commented Aug 10, 2021

maisnamrajusingh commented Aug 12, 2021

maisnamrajusingh commented Jul 29, 2021 •

edited by jfrerich

Loading

codecov bot commented Jul 30, 2021 •

edited

Loading

sanjaydemansol Aug 4, 2021 •

edited

Loading