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 Reply-To function #199

Merged
merged 7 commits into from
Oct 4, 2021
Merged

Conversation

peteroneilljr
Copy link
Contributor

Adding Reply-To function mentioned in issue #140

@mbrt
Copy link
Owner

mbrt commented Sep 27, 2021

Thanks a lot for the contribution!

I was trying to test this out, but I couldn't make it work.
It seems that Gmail doesn't support this at all? https://support.google.com/mail/answer/7190?hl=en.

@mbrt
Copy link
Owner

mbrt commented Sep 27, 2021

Actually I was wrong, it does seem to work, it's just undocumented.

I was able to create a filter that does just that and it seems to work.

FFR, here's the exported version:

<feed xmlns='http://www.w3.org/2005/Atom' xmlns:apps='http://schemas.google.com/apps/2006'>
	<title>Mail Filters</title>
	<id>tag:mail.google.com,2008:filters:...</id>
	<updated>2021-09-27T19:35:54Z</updated>
	<author>
		<name>Me</name>
		<email>me@gmail.com</email>
	</author>
	<entry>
		<category term='filter'></category>
		<title>Mail Filter</title>
		<id>tag:mail.google.com,2008:filter:z...</id>
		<updated>2021-09-27T19:35:54Z</updated>
		<content></content>
		<apps:property name='hasTheWord' value='replyto:mbrt/gmailctl'/>
		<apps:property name='shouldNeverSpam' value='true'/>
		<apps:property name='sizeOperator' value='s_sl'/>
		<apps:property name='sizeUnit' value='s_smb'/>
	</entry>
</feed>

Copy link
Owner

@mbrt mbrt left a comment

Choose a reason for hiding this comment

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

I'm looking through the changes and they look good!

There's only one place I could see missing:

  • README.md, section Tests, about the new replyto field in the test object.

@peteroneilljr
Copy link
Contributor Author

Hi @mbrt was it as simple as updating the list of fields available for the test object? Or is there an actual test object that needs to be updated?

Copy link
Owner

@mbrt mbrt left a comment

Choose a reason for hiding this comment

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

Tests are also failing. See the results here.

One way to fix this is to do the following:

go test ./... --update
go test ./...

The first command should trigger the regeneration of the golden test files here: https://github.com/mbrt/gmailctl/blob/master/pkg/config/lib_test.go#L18. The second will run all the tests. If nothing fails we should be good to go!

README.md Outdated Show resolved Hide resolved
@peteroneilljr
Copy link
Contributor Author

Alright, everything should be fixed now. I was updating all of the same files that were updated when BCC was added as an operator and It seems I updated an autogenerated file used by the Go tests.

@mbrt
Copy link
Owner

mbrt commented Oct 2, 2021

Sorry for the delay in responding.

Actually the file you updated was good, it's the input to the test. The result is checked against the JSON files that are updated through the commands in the comment above.

If you remove it, there's no test for the new field you added, so a future change might break the behavior. Could you please add it back and fix the tests? Then we are truly good to go.

@peteroneilljr
Copy link
Contributor Author

Ok I think the tests are fixed now! :)

Copy link
Owner

@mbrt mbrt left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@mbrt mbrt merged commit 82184ee into mbrt:master Oct 4, 2021
@mbrt
Copy link
Owner

mbrt commented Oct 4, 2021

Thanks for the contribution! :)

@gtback
Copy link

gtback commented Oct 4, 2021

Thanks, @peteroneilljr !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants