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

Add enable on update (fixes #36) #79

Merged
merged 1 commit into from
Jan 23, 2020

Conversation

grubbins
Copy link
Contributor

Config item EnableOnUpdate allows processing on updated posts too.

@grubbins grubbins force-pushed the add-enable-on-update branch from fdd22b1 to 0c963af Compare November 15, 2019 00:53
@hanzei hanzei requested review from levb and jespino November 15, 2019 04:30
@hanzei hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Nov 15, 2019
@levb levb requested a review from aaronrothschild November 18, 2019 14:34
@levb levb added the 1: PM Review Requires review by a product manager label Nov 18, 2019
Copy link
Contributor

@levb levb left a comment

Choose a reason for hiding this comment

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

@levb
Copy link
Contributor

levb commented Nov 18, 2019

This will need to merge #80 to build

@grubbins
Copy link
Contributor Author

I'll look at a test case, yes.

I am working on getting the CLA signed...

Copy link
Contributor

@jespino jespino left a comment

Choose a reason for hiding this comment

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

Besides the tests comments, looks good to me. Thanks @grubbins! 🎉

@jespino
Copy link
Contributor

jespino commented Nov 21, 2019

/check-cla

Copy link

@aaronrothschild aaronrothschild left a comment

Choose a reason for hiding this comment

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

LGTM

@aaronrothschild aaronrothschild removed the 1: PM Review Requires review by a product manager label Nov 25, 2019
@hanzei hanzei added the Awaiting Submitter Action Blocked on the author label Nov 25, 2019
@levb
Copy link
Contributor

levb commented Dec 3, 2019

/check-cla

@levb
Copy link
Contributor

levb commented Dec 3, 2019

@grubbins were you able to sign the CLA?

@mattermod
Copy link
Contributor

This issue 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!

/cc @jasonblais @hanzei

@jasonblais
Copy link
Contributor

Hi @grubbins, let us know if you have any questions about the CLA? Happy to help clarify anything

@grubbins
Copy link
Contributor Author

Sorry for the delay all - the CLA will be fine but I need to get it signed at my employer and then resubmit this change under a new account that's a member of their organisation account.

@jasonblais
Copy link
Contributor

@grubbins no worries, thanks for the heads up!

@jasonblais
Copy link
Contributor

/check-cla

@jasonblais
Copy link
Contributor

/update-branch

@hanzei hanzei removed 2: Dev Review Requires review by a core committer Awaiting Submitter Action Blocked on the author Lifecycle/1:stale labels Jan 20, 2020
@hanzei hanzei requested review from levb and DHaussermann January 20, 2020 13:05
Copy link
Contributor

@levb levb left a comment

Choose a reason for hiding this comment

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

All good, but 1/5 I'd like to see a test added.

@grubbins grubbins force-pushed the add-enable-on-update branch from dffa8c1 to 2feacc6 Compare January 20, 2020 21:58
@grubbins
Copy link
Contributor Author

Added this to plugin_test, and CLA finally in place.

@grubbins
Copy link
Contributor Author

Actually no. The test is ineffective. Give me a minute...

@grubbins grubbins force-pushed the add-enable-on-update branch from 2feacc6 to cc1f690 Compare January 20, 2020 22:11
@grubbins
Copy link
Contributor Author

OK now the test actually does fail if I remove the update handling :)

I did not explicitly test that the plugin does not modify an updated post if the config item is disabled (although I did see that behaviour).

@@ -220,13 +221,54 @@ func TestSpecialCases(t *testing.T) {

for _, tt := range tests {
t.Run(tt.inputMessage, func(t *testing.T) {
post := &model.Post{
Message: tt.inputMessage,
{
Copy link
Contributor

Choose a reason for hiding this comment

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

LOL, that's an interesting way of formatting! 0/5 a slight preference to make each "block" a separate t.Run, and use the test names for comments - makes the logs pretty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I don't have a lot of experience structuring go tests, but I also felt the whole test could probably be re-arranged to more clearly report which test failed. I decided that would be better tackled by others...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I'll do it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW in case it's not obvious, the separate blocks are there to ensure we destroy the post instances because the plugin mutates the ones passed in... and it seemed even more messy to declare them all with separate names.

@levb
Copy link
Contributor

levb commented Jan 20, 2020

@DHaussermann ready for you to test

@DHaussermann
Copy link

DHaussermann commented Jan 21, 2020

@grubbins Thanks for this improvement! It is functional. When I update a post the pattern is applied as expected.
The one concern I have is that when I tested it locally I did not see the line "enableonupdate": appear in my config when I uploaded the build. I had to manually add the line to test this change. Is it possible this is an issue with the code change and not something I'm doing? Is this something you can reproduce?

@grubbins
Copy link
Contributor Author

when I tested it locally I did not see the line "enableonupdate": appear in my config when I uploaded the build

I am not sure what's expected here actually. I would not expect the config file to be updated when a plugin is uploaded. Maybe next time mattermost saves its config (i.e. exits, or you hit a save button in system console) it would write all the config fields?

@grubbins
Copy link
Contributor Author

@DHaussermann I just tried uploading the plugin to our test server (over-writing existing). There is no config section for this plugin in config.json and it does not seem to automatically create anything. I believe you just have to configure this file manually.

@DHaussermann
Copy link

Strange. I believe the line above it for enableadmincommand appeared in the config once a version that includes this is uploaded. So, I was expecting the new line to appear with a value of false that I could go change.
Screen Shot 2020-01-21 at 9 49 17 PM

@levb do you have any thoughts on this?

@grubbins
Copy link
Contributor Author

@DHaussermann I believe that is because the enableadmincommand configuration is controllable via the System Console, therefore mattermost can write it to config.json - but none of the other configuration of this plugin is available via the System Console.

@levb
Copy link
Contributor

levb commented Jan 22, 2020

@grubbins since you implemented the config setting as a global flag, perhaps it is indeed worth adding it to plugin.json so it is a visible setting? I'm 0/5 on this.

Also, this made me think that perhaps it should have been implemented as a per-link setting, rather than a global one? I hate to go back and re-do it but it really does seem more appropriate, what do you think?

none of the other configuration of this plugin is available via the System Console

This is only done this way because of the limitations of what we could do with the system console (fixed, predeclared configuration with a limited set of types). We are going to be soon running a campaign to elevate most commonly-used links to visible in config, on a case-by-case basis. If you are interested in co-starting it, please ping me on community, we can discuss and file the first ticket for it. cc @jfrerich

@grubbins
Copy link
Contributor Author

I could look at adding it to the plugin.json yes!

I don't want to make it per-link. I added this functionality in the first case because my users were very confused that the behaviour works on new posts but not updates (and assumed this was a bug), and everyone asked for it to work on update; I think it would be even more confusing if this happened for some link rules and not others.

Conversely the argument for leaving this feature turned off, if I understand rightly, is that it gives users a way to undo/suppress the autolink action. In that case I guess the users on that mattermost instance would need to be told this, so they can make use of it. Again, I think it would then be more confusing for them if this worked for some link rules and not others. (I also strongly feel that if we want an intuitive way for users to suppress the autolinking, turning off enable on update is not the solution)

So I would really prefer to get this merged, and if the need for per-link setting is really there then this global flag can act as a default and we could later add over-rides per link - how about that?

@grubbins grubbins force-pushed the add-enable-on-update branch from cc1f690 to a163b89 Compare January 22, 2020 21:44
@DHaussermann
Copy link

@grubbins the change implemented is working. As it is now, this is already very useful! I have no issue with addressing the follow-up concerns in a separate issue. However, since the flag is not exposed in the UI, we would at least need to update the read-me to include this option (I did not see a change for this).

cc @aaronrothschild ☝️.

@grubbins
Copy link
Contributor Author

@DHaussermann agreed - I pushed another version which has the new setting in plugin.json. I hope that's enough for it to appear in System Console but I did not have time to test that yet...

@grubbins
Copy link
Contributor Author

OK, I tested it again with the entry in plugin.json and indeed the new option is now visible via System Console. It is not immediately written to config.json, but as expected if I save the config in System Console then the new configuration item is written to config.json.

@levb
Copy link
Contributor

levb commented Jan 23, 2020

@grubbins I am convinced! Thanks for the detailed reasoning!

@levb
Copy link
Contributor

levb commented Jan 23, 2020

@DHaussermann I think we just need your ok on the newly visible flag in the UI.

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

  • Confirmed new True / False config exists in the UI
  • Added test coverage for this in release testing
    LGTM!
    Thanks @grubbins

@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 Jan 23, 2020
@levb levb merged commit dedc5ac into mattermost-community:master Jan 23, 2020
@aaronrothschild
Copy link

@levb @grubbins I got this comment from a customer - thoughts on this scenario? Separate PR (I know this is merged already)?

  1. Autolink- a recent (January) community contribution will actually fix this issue- Add enable on update (fixes #36) #79 . I talked to the PM for integrations and he’s working on getting an update/release scheduled for auto link so that we can include this. Timing TBD but hopefully soon. I’ll keep an eye on it.

Interesting, thanks for pointing me at the pull request. It looks like it will allow an individual auto-link rule to indicate that it should be re-run on edit, and assumes that most auto-link rules are idempotent. I'm not sure if mine are, but perhaps I can make them work correctly on re-run.

However, even if I can make it work I'm not sure it's exactly what users would expect. For example, suppose I have a ticketing system auto-link that converts something like XYZ-5432 into XYZ-5432. If I meant to link to XYZ-54321, then when I edit I need to modify the link and the link text. Ideally, editing a post that's been auto-linked would show up as the pre-auto-linked text ("XYZ-5432") when editing and then have the auto-link re-applied.

@levb
Copy link
Contributor

levb commented Feb 12, 2020

@aaronrothschild - I think it is a new ticket, and maybe something for the platform team to consider (versioning the post, in its most general manifestation).

@jfrerich jfrerich mentioned this pull request Mar 19, 2020
3 tasks
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.

8 participants