Reimplement translation prs #143

metanerd · 2020-06-25T10:49:37Z

Summary

Before the migration from pootle to weblate, we got translation PRs for review into the i18n channel. This PR implements the same logic for weblate.

Ticket Link

khos2ow

LGTM, very minimal comments though.

khos2ow · 2020-06-25T15:58:09Z

server/enterprise.go

@@ -14,7 +14,7 @@ import (
 )

 func (s *Server) triggerEETestsForOrgMembers(pr *model.PullRequest) {
-	if s.IsOrgMember(pr.Username) {
+	if s.IsOrgMember(pr.Username) || s.IsKnownUser(pr.Username) {


Is this needed? I mean do we want to run enterprise tests for external known users?

I renamed it to IsTrustedUser(). I am for adding this, because the translation bot only modifies translation files and otherwise I have to add the EETests label and then check in again in an hour. But if you have objections, I am fine with not whitelisting the CLAExcluded bots. mattermost/mattermost#14872

No, I'm fine and there's no harm running EE tests anyway.

khos2ow · 2020-06-25T15:59:11Z

server/translations.go

+
+	dataMsg := fmt.Sprintf("####[%v translations PR %v](%v)\n", pr.RepoName, time.Now().UTC().Format(time.RFC3339), pr.URL)
+	msg := dataMsg + s.Config.TranslationsMattermostMessage
+	mlog.Debug("Sending Mattermost message", mlog.String("message", msg))


I'd suggest changing the message to something like Translation instead of Mattermost for clarity.

Why does it make it clearer?

Because the payload is being sent to translation service and not Mattermost. Isn't it?

It is sent to mattermost. It contains the links to the PRs for translations: https://community.mattermost.com/core/pl/sqzth7ryibgrbqj196trrs4hec

khos2ow · 2020-06-25T15:59:40Z

server/translations.go

+
+	webhookRequest := &Payload{Username: "Weblate", Text: msg}
+	if err := s.sendToWebhook(s.Config.TranslationsMattermostWebhookURL, webhookRequest); err != nil {
+		mlog.Error("Unable to post to Mattermost webhook", mlog.Err(err))


The same, so if any error happens and one looks at the logs won't get confused.

khos2ow · 2020-06-25T16:00:17Z

server/webhook.go

-func (s *Server) sendToWebhook(payload *Payload) error {
+func (s *Server) sendToWebhook(webhookURL string, payload *Payload) error {
+	if webhookURL == "" {
+		err := errors.New("no Mattermost webhook URL set: unable to send message")


The same, may be drop Mattermost?

agnivade

Pushing it out of my queue until some tests are added as discussed offline. Please re-request review when the tests are added. Thanks.

agnivade

I also want to understand how does this change know that this is a translation PR? From what I understand, it is unconditionally posting the message to the webhook URL for every PR.

Perhaps I have missed something that I am not aware of?

server/enterprise.go

server/translations.go

server/config.go

server/github.go

server/webhook_integration_test.go

agnivade

Getting there. But needs some more work. Please take a look at my comments. Thanks!

server/cla.go

agnivade · 2020-07-03T04:22:52Z

server/utils.go

 	}
+	defer r.Body.Close()


I am not seeing this resolved. Please see my comment #143. We can just use closeBody here.

agnivade · 2020-07-03T04:24:21Z

server/webhook.go

+		badRequestR := &http.Response{
+			StatusCode: http.StatusBadRequest,
+			Body:       ioutil.NopCloser(bytes.NewBufferString(err.Error())),
+		}


This does not appear to be resolved?

agnivade · 2020-07-03T04:29:08Z

server/webhook.go

+type WebhookValidationError struct {
+	err string
+}
+
+func (e *WebhookValidationError) Error() string {
+	return fmt.Sprintf("%v", e.err)
+}
+


This implements only partially what I suggested. This just wraps the error with a type, and does not do anything else. As it is, it's not very useful. I was suggesting that a field added to the struct which will indicate what is not set- url/username/text.

Please also add godocs to both the struct and the method.

Can you please send me an example for what you mean by this?

Sure.

// WebhookValidationError contains an error in the webhook payload. type WebhookValidationError struct { field string } // Error implements the error interface. func (e *WebhookValidationError) Error() string { return "invalid" + e.field } func newWebhookValidationError(field string) *WebhookValidationError { return &WebhookValidationError{ field: field, } } // from code if webhookURL == "" { return newWebhookValidationError("webhook URL") } // in test var whError *WebhookValidationError require.True(errors.As(err, &whError)) assert.Equal(whError.field, "webhook URL")

Let me know if that helps. (Code is untested, I just wrote here directly)

This helps immensely! Thank you so much!

agnivade · 2020-07-03T04:30:44Z

server/cla.go

+	if err != nil {
+		return nil, err
+	}
+	r, err := http.DefaultClient.Do(req)
 	if err != nil {
 		mlog.Error("Unable to get CLA list", mlog.Err(err))


Unrelated to this PR, but this is an anti-pattern. Logging and then returning the same error. We need to do a sweep to fix all of these.

agnivade · 2020-07-03T04:36:12Z

server/webhook_test.go

+	if testing.Short() {
+		t.Skip("skipping integration test")
+	}


Any reason for this?

Same for below.

I want to differentiate between running unit tests and implementation tests. I looked up on how to accomplish this. This was, what came up as a go to solution.

But this is an unit test from what I understand. There is no external dependency with any other service. It's purely inside the code.

OK, removed.

agnivade · 2020-07-03T04:36:22Z

server/webhook_test.go

+	}
+
+	s := &Server{
+		StartTime: time.Now(),


Why do we need this in the test?

Same for below.

We are calling s.sendToWebhook(context.Background(), mattermost.URL, validPayload). Is there another way?

Sorry, I meant the StartTime field. Is that needed?

server/webhook_test.go

…attermost-mattermod into reimplement-translation-prs

agnivade

Apart from the payload validation, just some tiny things remaining.

server/webhook.go

server/cla.go

Co-authored-by: Agniva De Sarker <agnivade@yahoo.co.in>

agnivade

Thanks for your patience with this @metanerd. Looks great now.

metanerd · 2020-07-03T14:46:21Z

Thanks to you, sir!

metanerd added 2 commits June 25, 2020 12:33

Reimplement translation PRs.

38d57c8

Use string for time.

ad34752

metanerd requested review from khos2ow and agnivade June 25, 2020 10:49

Merge branch 'master' into reimplement-translation-prs

8cfaef2

jaydeland approved these changes Jun 25, 2020

View reviewed changes

hanzei added the 2: Dev Review Requires review by a core committer label Jun 25, 2020

khos2ow reviewed Jun 25, 2020

View reviewed changes

agnivade reviewed Jun 26, 2020

View reviewed changes

metanerd added 3 commits June 26, 2020 16:08

Add tests.

9cb98b5

Resolve conflicts.

62fe12e

Merge branch 'master' into reimplement-translation-prs

569895d

metanerd requested review from khos2ow and agnivade June 26, 2020 14:17

Avoid named returns 🤷

9c2074a

khos2ow approved these changes Jun 26, 2020

View reviewed changes

agnivade requested changes Jun 29, 2020

View reviewed changes

metanerd added 3 commits July 2, 2020 12:44

Address comments.

b2f5e63

Merge master.

58ec618

Renaming unclear method.

a9ded67

metanerd requested a review from agnivade July 2, 2020 12:00

agnivade requested changes Jul 3, 2020

View reviewed changes

metanerd added 6 commits July 3, 2020 08:18

Merge branch 'master' into reimplement-translation-prs

b2f25bc

Address comments.

c3a750c

Further address comments.

e016444

Merge branch 'master' into reimplement-translation-prs

5d895a5

Rm gap.

79d2e2e

Merge branch 'reimplement-translation-prs' of github.com:mattermost/m…

f5bb1f6

…attermost-mattermod into reimplement-translation-prs

metanerd requested a review from agnivade July 3, 2020 07:28

Merge branch 'master' into reimplement-translation-prs

bfd4938

metanerd added 2 commits July 3, 2020 11:11

Rm skip tests

a0f5b0d

Merge branch 'reimplement-translation-prs' of github.com:mattermost/m…

b0e5145

…attermost-mattermod into reimplement-translation-prs

agnivade reviewed Jul 3, 2020

View reviewed changes

server/webhook.go Outdated Show resolved Hide resolved

server/webhook.go Outdated Show resolved Hide resolved

server/cla.go Outdated Show resolved Hide resolved

metanerd and others added 2 commits July 3, 2020 12:40

Update server/cla.go

cf12593

Co-authored-by: Agniva De Sarker <agnivade@yahoo.co.in>

improve PR

5bafa9e

agnivade approved these changes Jul 3, 2020

View reviewed changes

Merge branch 'master' into reimplement-translation-prs

b050b50

metanerd merged commit 888c042 into master Jul 4, 2020

metanerd deleted the reimplement-translation-prs branch July 4, 2020 09:16

agnivade added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Jul 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reimplement translation prs #143

Reimplement translation prs #143

metanerd commented Jun 25, 2020

khos2ow left a comment

khos2ow Jun 25, 2020

metanerd Jun 26, 2020 •

edited

khos2ow Jun 26, 2020

khos2ow Jun 25, 2020

metanerd Jun 26, 2020

khos2ow Jun 26, 2020

metanerd Jun 26, 2020

khos2ow Jun 25, 2020

khos2ow Jun 25, 2020

agnivade left a comment

agnivade left a comment

agnivade left a comment

agnivade Jul 3, 2020

agnivade Jul 3, 2020

agnivade Jul 3, 2020

metanerd Jul 3, 2020

agnivade Jul 3, 2020

metanerd Jul 3, 2020

agnivade Jul 3, 2020

agnivade Jul 3, 2020

metanerd Jul 3, 2020

agnivade Jul 3, 2020

metanerd Jul 3, 2020

agnivade Jul 3, 2020

metanerd Jul 3, 2020

agnivade Jul 3, 2020

agnivade left a comment

agnivade left a comment

metanerd commented Jul 3, 2020

Reimplement translation prs #143

Reimplement translation prs #143

Conversation

metanerd commented Jun 25, 2020

Summary

Ticket Link

khos2ow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metanerd Jun 26, 2020 • edited

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

agnivade left a comment

Choose a reason for hiding this comment

agnivade left a comment

Choose a reason for hiding this comment

agnivade 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

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

agnivade left a comment

Choose a reason for hiding this comment

agnivade left a comment

Choose a reason for hiding this comment

metanerd commented Jul 3, 2020

metanerd Jun 26, 2020 •

edited