Fix Bug 1603610 - Enable users to leave comments on translations #1524

abowler2 · 2019-12-18T16:21:01Z

Initial work to add the ability to leave comments on translations.

At the moment I'm just printing the comments into the history panel. Moving forward from here I believe I will need to add a Comment component in order to render and style all the information properly. If that is indeed the case should I add that component to the History module or the Translation module?

Also, I'm not sure if a Prefetch is needed for retrieving the comments in map_entities within models.py, but I was unsuccessful in my attempts at that. Would appreciate your thoughts and insights on this.

TODO:

File a bug to rename Translation.user to Translation.author.
File a bug to make the Add comment fields grow as you type.
./manage.py makemigrations says we need to make some migrations.
Use UserAvatar component wherever possible.

mathjazz

Awesome work, @abowler2!

I think you'll need a Comments component in the Translation component. It should render two other components:

Comment for the display of the comment
AddComment for adding the comment

I believe you'll indeed need to prefetch comments in get_translation_history() before you serialize them. I can help with that if you run into issues.

frontend/src/modules/history/components/Translation.js

frontend/src/core/api/types.js

pontoon/base/models.py

frontend/src/modules/history/components/Translation.js

pontoon/base/admin.py

abowler2 · 2019-12-20T05:21:26Z

Current progress for review. There is obviously still a lot to be done but wanted to check in to make sure all is on track (Note: I used some temporary filler values for titles and inline styling in the components that is not intended to stay 😄 )

In addition to the obvious need for styling, I still have to add the Prefetch in get_translation_history, the Delete and Share buttons in the Comment component, and build out the AddComment component.

Please let me know what changes are needed at this point.

mathjazz

Great progress!

We should move all three Comment* components from core/translation to the newly created core/comments. Looking at the spec, we should benefit from reusing these components for team comments.

pontoon/base/models.py

frontend/src/core/translation/components/Comment.js

frontend/src/core/translation/components/Comments.js

frontend/src/core/translation/components/Comment.js

pontoon/base/views.py

pontoon/base/models.py

frontend/src/core/translation/components/Comment.js

adngdb

This is a great start April! I've made a bunch of comments about good practice, conventions and stuff, but overall the code looks good.

In terms of workflow, maybe it won't be needed in this case (this PR is still of manageable size) but I encourage you to split your work into the smallest possible chunks. For example, you could have one PR for the back-end / models features, using unit tests to make sure it works as expected, and then have a follow-up PR for the front-end work.

Also, please do not forget unit tests! They are your greatest allies to make sure your code works. And even though I'm not good at it, I assure you that test-driven development is great. It forces you to think about what exactly you want to build before coding it, and that's both a great exercise and just good development practice.

pontoon/base/views.py

frontend/src/core/api/types.js

frontend/src/core/translation/components/Comment.js

frontend/src/core/translation/components/Comments.js

frontend/src/core/translation/index.js

frontend/src/modules/history/components/Translation.js

abowler2 · 2019-12-20T22:04:44Z

Made the requested changes. Please let me know if any further changes are needed.

RE: the failing Translation test ~ I refactored the user's avatar to use the new UserImage component which causes the test to fail based on only one a tag being present now. Not sure if I should adjust the test to account for the change or if there is something I missed when using the new component.

adngdb

Great follow-up! I've made a few more comments, all little things around conventions or accessibility.

Please do add unit tests for all your code though! 😇

RE: the failing Translation test ~ I refactored the user's avatar to use the new UserImage component which causes the test to fail based on only one a tag being present now. Not sure if I should adjust the test to account for the change or if there is something I missed when using the new component.

You do need to adjust the test. Since you've added a layer of indirection, you do not want to test the content of the component anymore (instead you want to do that in that component's unit tests) but just verify that the component is created. Eventually that it is passed the right props, if that makes sense.

frontend/src/core/comments/components/Comment.js

frontend/src/core/comments/components/CommentsList.js

frontend/src/core/comments/index.js

frontend/src/core/user/components/UserImage.js

adngdb · 2019-12-23T10:03:32Z

pontoon/base/models.py

-            'translation': self.translation_id
+            "author": self.author.name_or_email,
+            "username": self.author.username,
+            "userGravatarUrlSmall": self.author.gravatar_url(88),


Ah, here's the trick: Python variables should use underscore_case. :) This is confusing, sorry. We effectively have two different languages with different naming conventions. We want to strictly enforce those naming conventions. The only time when they conflict is through the API. And that's where we need some specific code to manually convert those names.

So, here you should revert this change. But keep your type definition in JS. Then on the API function, in front-end, where you get the data, you should have some logic to transform the data, mapping one convention (underscore_case from Python) to the other (camelCase for JS).

Here's where the data gets fetched:

pontoon/frontend/src/core/api/entity.js

Line 115 in 6cc116b

async getHistory(

Side note: if this data was exposed with GraphQL, we wouldn't have that problem, as it handles that conversion automatically for us. Sadly, this is not the case yet, but doing this conversion now means it will be easier later on to switch to fetching that data through GraphQL.

abowler2 · 2019-12-30T05:08:54Z

Sending up the updates from the last review. I'm planning to work on the delete and share button functionality next and then styling the elements.

The one thing I did have issues with was the Copy to Translation title which displays on hover that we discussed last week. It's being displayed from the Localization tag, and I was not able to successfully change it up without breaking it. I figured I would work on it more when I begin the styling, but your thoughts on how I can approach it are always welcome 😄

adngdb · 2019-12-30T14:25:54Z

.vscode/settings.json

@@ -0,0 +1,2 @@
+{
+}


Please don't send this file to the repo. :) (You can add it in the .gitignore file if you want, or in your .git/info/exclude file so that it's only for your local git folder.)

This one is still around. :)

frontend/src/core/comments/components/Comment.js

frontend/src/core/user/components/UserAvatar.js

frontend/src/core/api/entity.js

frontend/src/core/comments/components/Comment.test.js

adngdb · 2019-12-30T15:56:05Z

frontend/src/core/api/entity.js

+            renamedResultsKeys.comments = renamedCommentsKeys;
+        }
+
+        return [renamedResultsKeys];


I find this code too complex, and too specific. Once again, apologies for opening a can of worms here. Let's still get to the end of it, but by using a more generic solution that we can then apply to other places around the API.

I found this article that shows a nice solution to transform code from snake_case to camelCase: https://matthiashager.com/converting-snake-case-to-camel-case-object-keys-with-javascript

I'd take that code and put in the api/base.js file, add a method there to transform objects, and then use that method here to convert the results.

abowler2 · 2019-12-30T20:07:47Z

Updated the camelCase conversion as per @adngdb's note. However, I'm not sure if I 'typed' the argument that is passed in properly. Technically it is being passed an Array<Object> from getHistory, but if I type it as that I get Flow errors within the function. Also, when this is used for other api's it might not be the same. So, I have it just as an Object which does not throw any errors from Flow.

pontoon/base/models.py

mathjazz · 2019-12-30T21:24:07Z

frontend/src/core/comments/components/Comment.js

+    const { comment } = props;
+
+    if (!comment) {
+            return null;


Nit: too much indent.

Nit: Still too much indent.

frontend/src/core/comments/components/CommentsList.js

mathjazz · 2019-12-30T21:30:04Z

Also, when this is used for other api's it might not be the same. So, I have it just as an Object which does not throw any errors from Flow.

I believe this is one of those rare cases where using Object is actually permissible - for the reason you mentioned.

abowler2 · 2019-12-31T00:07:02Z

Early styling of Comment element for review.

One of the issues that came up with the change in structure is that the bottom border is now above the comments since they have been separated into individual divs. I was thinking this could be set up as a conditional class that would be placed based on the existence of comments or not. Let me know if you think a different approach would be better.

mathjazz

Nice progress!

You should move the border styling from the .translation element to the li element. Feel free to add className to it if that makes it easier to define the selector reliably.

The avatars don't show up yet and the comment styling also applies to the metadata comments:

frontend/src/core/comments/components/CommentsList.test.js

frontend/src/core/api/base.js

mathjazz · 2019-12-31T07:35:49Z

.vscode/settings.json

@@ -0,0 +1,2 @@
+{
+}


This one is still around. :)

mathjazz · 2019-12-31T07:38:16Z

frontend/src/core/comments/components/Comment.js

+    const { comment } = props;
+
+    if (!comment) {
+            return null;


Nit: Still too much indent.

frontend/src/core/comments/components/Comment.js

frontend/src/core/comments/components/Comment.css

frontend/public/static/locale/en-US/translate.ftl

frontend/src/core/api/base.js

frontend/src/core/user/components/UserAvatar.js

frontend/src/core/api/base.js

adngdb · 2019-12-31T16:11:04Z

The Black formatting commit has been merged to master, causing conflict on this branch. Please rebase and fix the conflicts as soon as you can, to avoid potentially more conflicts later on! And my apologies for disturbing your workflow, but I strongly believe now was the best time to do this. :-)

abowler2 · 2019-12-31T16:30:36Z

The avatars don't show up yet and the comment styling also applies to the metadata comments:

That is really weird ~ Here is what it looks like on my side:

mathjazz · 2020-01-21T00:37:31Z

frontend/src/core/comments/components/CommentsList.js

+
+import type { TranslationComment } from 'core/api';
+import type { HistoryTranslation } from 'modules/history'
+import type { UserState } from 'core/user';


Nit: we sort imports by module, i.e. the value following from.

mathjazz · 2020-01-21T10:35:48Z

frontend/src/modules/history/components/Translation.js

-        let canReject = (canReview || (ownTranslation && !translation.approved)) && !isReadOnlyEditor;
+        let canDelete = (isTranslator || ownTranslation) && !isReadOnlyEditor;
+        let canReject = (isTranslator || (ownTranslation && !translation.approved)) && !isReadOnlyEditor;
+        let canComment = (isTranslator || ownTranslation);


According to the "Permissions" section of the spec, any logged in contributor canComment, so the value should be user.isAuthenticated. And then you don't need the !! part of the !!canComment below.

I think it's still valuable to have the canComment variable defined along with other CanSomething permissions.

abowler2 · 2020-01-21T16:11:46Z

Sorry, pushed and then realized the translation test was broken. Working on it now

mathjazz · 2020-01-21T18:53:35Z

frontend/src/modules/history/components/Translation.test.js

@@ -286,6 +302,7 @@ describe('<TranslationBase>', () => {
                translation={ translation }
                entity={ DEFAULT_ENTITY }
                locale={ DEFAULT_LOCALE }
+                user={ 'Andy_Dwyer' }


While this probably makes the test pass, this is better: user={ { username: 'Andy_Dwyer' } }.

mathjazz · 2020-01-22T17:06:42Z

These are the CSS fixes I promised, which:

Make the Comment toggle and Show diff toggle look & feel the same way.
Unify the display of comments and the Add comment form.
Fix a few bugs.

Please apply the patch.

mathjazz · 2020-01-22T17:44:17Z

@abowler2 One more patch please - it makes the input fields styled consistently.

diff --git a/frontend/src/core/comments/components/AddComment.css b/frontend/src/core/comments/components/AddComment.css
index a3fd9f5d1..0f0915bfd 100644
--- a/frontend/src/core/comments/components/AddComment.css
+++ b/frontend/src/core/comments/components/AddComment.css
@@ -1,10 +1,10 @@
 .comments-list .add-comment textarea {
-    background-color: transparent;
-    border: solid 1px #5E6475;
+    background: #333941;
+    border: none;
     border-radius: 4px;
     color: #FFFFFF;
     font-size: 11px;
-    height: 22px;
-    line-height: 22px;
+    height: 24px;
+    line-height: 24px;
     padding: 6px;
 }
diff --git a/frontend/src/modules/machinery/components/Machinery.css b/frontend/src/modules/machinery/components/Machinery.css
index b988b51a0..438089d2e 100644
--- a/frontend/src/modules/machinery/components/Machinery.css
+++ b/frontend/src/modules/machinery/components/Machinery.css
@@ -23,7 +23,7 @@
 }
 
 .machinery > .search-wrapper input[type=search] {
-    background: #4D5967;
+    background: #333941;
     border: none;
     border-radius: 20px;
     box-sizing: border-box;
diff --git a/frontend/src/modules/search/components/SearchBox.css b/frontend/src/modules/search/components/SearchBox.css
index 0817c68cb..fa4e2b90f 100644
--- a/frontend/src/modules/search/components/SearchBox.css
+++ b/frontend/src/modules/search/components/SearchBox.css
@@ -4,7 +4,7 @@
 }
 
 .search-box input[type="search"] {
-    background: #4D5967;
+    background: #333941;
     border: none;
     border-radius: 20px;
     box-sizing: border-box;

frontend/src/core/comments/components/Comment.css

adngdb

Nearly there! Excellent work @abowler2!

adngdb · 2020-01-23T10:20:35Z

frontend/public/static/locale/en-US/translate.ftl

+## Allows user to leave comments on translations
+
+comments-AddComment--input =
+    .placeholder = Write a comment...


Nit: please use the actual character here: ….

frontend/public/static/locale/en-US/translate.ftl

adngdb · 2020-01-23T10:43:14Z

frontend/public/static/locale/en-US/translate.ftl

+        [one] { $commentCount } Comment
+       *[other] { $commentCount } Comments
+    }
+    .title = Toggle translation comment


I'd do "comment" -> "comments".

adngdb · 2020-01-23T11:03:58Z

frontend/src/core/comments/components/AddComment.js

+                    id='comment-input'
+                    name='comment'
+                    dir='auto'
+                    placeholder={ `Write a comment ${'\u2026'}` }


No space before the ellipsis, and I believe you can simply use the actual character, no need to use the Unicode representation: … (you can just copy and paste it ;) ).

Note that, whenever you change localized content, what actually matters is what is in the en-US FTL file, not what is in the code. That's there only to help developers understand what the content is without having to go look into the .ftl files, but that content will never be shown to users. We should aim to have content in code always in sync with content in the en-US reference FTL files, even if that's a bit painful to do.

adngdb · 2020-01-23T11:07:26Z

frontend/src/core/comments/components/AddComment.test.js

+
+const DEFAULT_USER = {
+    user: 'RSwanson',
+    username: 'Ron_Swanson',


frontend/src/core/comments/index.js

adngdb · 2020-01-23T11:18:04Z

frontend/src/core/user/components/UserAvatar.js

+                />
+            </Localized>
+        </div>;
+


Nit: please remove this empty line.

abowler2 · 2020-01-23T16:54:26Z

frontend/src/modules/history/components/Translation.js

-                                    />
+                                        className={ commentCount === 0 ? 'toggle-comments' :
+                                            'toggle-comments'
+                                        }


Since the className is now the same regardless of commentCount this should just set the className to toggle-comments without the ternary, correct?

I went ahead and made this change. Just let me know if I'm missing something and the logic needs to be reapplied.

That ternary was weird anyways. :) "If true X else X". :)

Originally there were two different classes based on the condition, which made more sense 😄

mathjazz · 2020-01-23T19:26:34Z

frontend/src/modules/history/components/Translation.js

+                                    id={ commentCount === 0 ?
+                                        'history-Translation--button-comment' :
+                                        'history-Translation--button-comments'
+                                    }


It would be more readable if each <button> would be wrapped in their own <Localized> element.

I tried that first but couldn't get it working. I'll have another go to see if I can work it out as I agree it would be much easier to understand what is happening there.

mathjazz · 2020-01-23T19:30:07Z

frontend/src/modules/history/components/Translation.js

-                                    />
+                                        className={ commentCount === 0 ? 'toggle-comments' :
+                                            'toggle-comments'
+                                        }


That ternary was weird anyways. :) "If true X else X". :)

mathjazz

Great work, only found two nits!

mathjazz · 2020-01-23T19:31:46Z

frontend/src/modules/history/components/Translation.js

+                                            { commentCount === 1 ?
+                                                `${commentCount} Comment` :
+                                                `${commentCount} Comments`
+                                            }


You should simplify this to a simple string, e.g.:

{ `${commentCount} Comments` }

See:
#1524 (comment)

mathjazz · 2020-01-23T20:49:56Z

frontend/src/modules/history/components/Translation.js


-        if (!translation.uid) {
-            return <span>{ translation.user }</span>;
+    renderCommentButton(commentCount: number) {


renderCommentToggle would be consistent with the naming used by the Diff toggle.

mathjazz · 2020-01-23T20:50:09Z

frontend/src/modules/history/components/Translation.js

-        if (!translation.uid) {
-            return <span>{ translation.user }</span>;
+    renderCommentButton(commentCount: number) {
+        if(commentCount === 0) {


Nit: Missing space.

mathjazz · 2020-01-23T20:50:33Z

frontend/src/modules/history/components/Translation.js

+            return <Localized
+                id='history-Translation--button-comment'
+                attrs={{ title: true }}
+                $commentCount={ commentCount }


We don't use this variable.

mathjazz · 2020-01-23T20:50:44Z

frontend/src/modules/history/components/Translation.js

+                    >
+                        { 'Comment' }
+                    </button>
+            </Localized>
        }



Nit: remove blank line.

mathjazz · 2020-01-23T20:51:29Z

frontend/src/modules/history/components/Translation.js

+                    { commentCount === 1 ?
+                        `1 Comment` :
+                        `${commentCount} Comments`
+                    }


See #1524 (comment).

mathjazz · 2020-01-24T17:02:00Z

Alright, we're done here! Great work, April!

Before we merge, please disable the feature until we land Team Comment by:

Returning HTTP error 501 in the beginning of the add_comment() view, e.g.

    # TODO: Remove as part of bug 1361318
    return JsonResponse(
        {"status": False, "message": "Not Implemented"},
        status=501,
    )

Returning null in the beginning of the renderCommentToggle() function.

mathjazz

Excellent job, @abowler2! \o/

mathjazz changed the title ~~[WIP] Fix Bug 1603610 - Enable reviewers to leave comments on translations~~ [WIP] Fix Bug 1603610 - Enable users to leave comments on translations Dec 18, 2019

mathjazz reviewed Dec 18, 2019

View reviewed changes

pontoon/base/admin.py Outdated Show resolved Hide resolved

mathjazz reviewed Dec 20, 2019

View reviewed changes

adngdb reviewed Dec 20, 2019

View reviewed changes

adngdb reviewed Dec 23, 2019

View reviewed changes

adngdb reviewed Dec 30, 2019

View reviewed changes

mathjazz reviewed Dec 30, 2019

View reviewed changes

mathjazz reviewed Dec 31, 2019

View reviewed changes

adngdb reviewed Dec 31, 2019

View reviewed changes

abowler2 added 12 commits December 31, 2019 09:16

Initial setup of model for translation comments

af6fcef

Made updates to and migrated Comment model

8cc1297

Send comment info to the frontend

1d3885a

Refactoring

7f72c2b

Initial steps to render comments on frontend

7cb311a

Requested revisions

22e3cce

Serialize comments and initial work on Comments component

0efd8f0

Refactored to add Comment and AddComment components

7cdbc07

Refactoring of comment components

476afcc

Requested refactoring

ebf1d67

Fixed linting errors

3df52ab

Refactoring and requested changes

1fcc776

mathjazz reviewed Jan 21, 2020

View reviewed changes

Hid comment button when no comments and not authenticated

4e2e1c4

mathjazz reviewed Jan 21, 2020

View reviewed changes

Updated permissions for comments

62cca2b

abowler2 added 2 commits January 21, 2020 08:47

Made change to allow tests to pass

58e5975

Changed the changes and fixed the test

e1bf8e6

mathjazz reviewed Jan 21, 2020

View reviewed changes

updated test

95687ee

Applied CSS patch

8adcef8

abowler2 added 2 commits January 22, 2020 09:50

Additional CSS changes

9e56237

Fixed error with className

a7a13a7

mathjazz reviewed Jan 22, 2020

View reviewed changes

frontend/src/core/comments/components/Comment.css Show resolved Hide resolved

Added font-weight to comments

ad32c94

adngdb reviewed Jan 23, 2020

View reviewed changes

abowler2 commented Jan 23, 2020

View reviewed changes

Fixed localization and syntax issues. Made needed migrations

9230e0c

mathjazz reviewed Jan 23, 2020

View reviewed changes

Refactored comment button

137eeec

mathjazz reviewed Jan 23, 2020

View reviewed changes

Requested changes to Comment toggle

1bccc01

abowler2 changed the title ~~[WIP] Fix Bug 1603610 - Enable users to leave comments on translations~~ Fix Bug 1603610 - Enable users to leave comments on translations Jan 24, 2020

Disabled comments until team comments complete

6ad244f

mathjazz approved these changes Jan 24, 2020

View reviewed changes

mathjazz merged commit 597c496 into mozilla:master Jan 24, 2020

abowler2 deleted the translation-comments branch January 24, 2020 20:03

Fix Bug 1603610 - Enable users to leave comments on translations #1524

Fix Bug 1603610 - Enable users to leave comments on translations #1524

Conversation

abowler2 commented Dec 18, 2019 • edited by mathjazz

mathjazz left a comment

Choose a reason for hiding this comment

abowler2 commented Dec 20, 2019

mathjazz left a comment

Choose a reason for hiding this comment

adngdb left a comment

Choose a reason for hiding this comment

abowler2 commented Dec 20, 2019

adngdb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abowler2 commented Dec 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abowler2 commented Dec 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathjazz commented Dec 30, 2019

abowler2 commented Dec 31, 2019

mathjazz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adngdb commented Dec 31, 2019

abowler2 commented Dec 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abowler2 commented Jan 21, 2020

Choose a reason for hiding this comment

mathjazz commented Jan 22, 2020 • edited

mathjazz commented Jan 22, 2020

adngdb 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

mathjazz 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

mathjazz commented Jan 24, 2020 • edited

mathjazz left a comment

Choose a reason for hiding this comment

abowler2 commented Dec 18, 2019 •

edited by mathjazz

mathjazz commented Jan 22, 2020 •

edited

mathjazz commented Jan 24, 2020 •

edited