Verify Translation in Development #9878

imajit · 2021-07-02T17:14:23Z

Fixes #9686
To review of FTOs easily, now the contributor can quickly change the required files and run it on Gitpod. As some of the FTOs related to translation need UI checks, in PR contributors can add the Gitpod UI screenshots.

I read about this method to show the button in development mode only, not sure if this is the best practice, also some numbers are attached to the link whenever the button is pressed, it doesn't affect the result though not sure how it is getting added each time.

gitpod-io · 2021-07-02T17:14:25Z

qlty-cloud-legacy · 2021-07-02T17:15:29Z

app/controllers/home_controller.rb

+    redirect_to '/change_locale/zh-CN'
+  end
+
+def switch_off_translation


Inconsistent indentation detected.

qlty-cloud-legacy · 2021-07-02T17:15:29Z

app/controllers/home_controller.rb

+  end
+
+def switch_off_translation
+    UserTag.remove_if_exists(current_user.uid,'translation-helper')


Use 2 (not 4) spaces for indentation.

qlty-cloud-legacy · 2021-07-02T17:15:30Z

app/controllers/home_controller.rb

  end

+  def switch_on_translation
+    UserTag.create_if_absent(current_user.uid,'translation-helper')


Space missing after comma.

qlty-cloud-legacy · 2021-07-02T17:15:30Z

app/controllers/home_controller.rb

+  end
+
+def switch_off_translation
+    UserTag.remove_if_exists(current_user.uid,'translation-helper')


Space missing after comma.

codecov · 2021-07-02T17:21:05Z

Codecov Report

Merging #9878 (0047d80) into main (d12a468) will increase coverage by 3.88%.
The diff coverage is 77.27%.

@@            Coverage Diff             @@
##             main    #9878      +/-   ##
==========================================
+ Coverage   78.24%   82.12%   +3.88%     
==========================================
  Files          98       98              
  Lines        5947     5964      +17     
==========================================
+ Hits         4653     4898     +245     
+ Misses       1294     1066     -228

Impacted Files	Coverage Δ
app/channels/room_channel.rb	`71.42% <0.00%> (ø)`
app/controllers/tag_controller.rb	`80.42% <ø> (+0.61%)`	⬆️
app/controllers/user_tags_controller.rb	`82.43% <50.00%> (-1.86%)`	⬇️
app/helpers/application_helper.rb	`85.41% <66.66%> (+5.20%)`	⬆️
app/models/tag.rb	`97.53% <100.00%> (+12.03%)`	⬆️
app/models/user.rb	`86.12% <100.00%> (+3.32%)`	⬆️
app/services/search_service.rb	`95.09% <0.00%> (+0.04%)`	⬆️
app/controllers/notes_controller.rb	`84.87% <0.00%> (+0.40%)`	⬆️
app/models/revision.rb	`88.29% <0.00%> (+3.19%)`	⬆️
... and 11 more

noi5e · 2021-07-12T18:02:50Z

@imajit Nice work!

imajit · 2021-07-13T10:45:08Z

The tests commit fails , is this because the code the tests are checking is not there in the code-base yet ?

jywarren · 2021-07-13T15:57:47Z

app/controllers/home_controller.rb

    end
  end

+  def switch_on_translation


maybe we should name this

Suggested change

def switch_on_translation

def switch_on_translation_helper

Is this only for development mode? If so, couldn't we use an existing route somehow -- https://github.com/publiclab/plots2/blob/main/test/functional/user_tags_controller_test.rb#L8-L15 maybe? To add the tag? though i guess it's 2 steps.

jywarren · 2021-07-13T19:04:05Z

The tests commit fails , is this because the code the tests are checking is not there in the code-base yet ?

If the code is not there yet, then yes! But you can then add the code and the test would then begin to pass. That would be "Test Driven Development" - a nice pattern!

jywarren · 2021-07-13T19:05:20Z

config/routes.rb

  post 'comment/react/delete/:id' => 'comment#react_delete'
  post 'comment/react/update/:id' => 'comment#react_update'

+  get 'switch_on_translation' => 'home#switch_on_translation'


Yes, if possible if we could use existing routes, we'd be able to skip some of the tests because we wouldn't be introducing a whole new route, you know? That would keep codebase complexity down a bit, which is really helpful in the long run on a big project like this.

imajit · 2021-07-15T22:15:28Z

I initially kept this button in the welcome message I could not see it anymore so I just placed it in the top part. I added an extra parameter to the existing route and changed the controller so now no new routes or controllers are needed. Thanks @jywarren for the suggestion it was a bit more complex but yes it keeps the codebase simple now 😄 . I'm not sure why codecov is failing though for codeclimate I guess I'll add a commit later.
@noi5e @jywarren should I move the button to some other place ?

jywarren · 2021-07-16T15:12:53Z

This is looking great! As to another place, it's only in development mode, but maybe it'd confuse people? What if we put it in the navbar menu? And don't worry about CodeCov, i'm working on that in #9905

Thanks!

imajit · 2021-07-16T15:20:57Z

The top Nav-bar right ? the one with the search bar

Tlazypanda · 2021-07-17T09:04:28Z

Ohh @imajit can't say for sure but I think yes @jywarren meant the top nav bar 😅 looping him here in case I missed it

imajit · 2021-07-17T09:43:45Z

This is how it looks in top-nav

jywarren · 2021-07-17T17:28:36Z

Would it be possible to put it in the same dropdown as under the profile picture (the small grey circle to the right side)? Thank you!

…

On Sat, Jul 17, 2021 at 5:43 AM Ajit Mujumdar ***@***.***> wrote: This is how it looks in top-nav [image: ezgif com-gif-maker(2)] <https://user-images.githubusercontent.com/38528640/126032897-5c9367d2-3152-4d49-b1da-fc8a7db23128.gif> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9878 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6JZRVCBPZNMRQUUC77DTYFGFXANCNFSM47XCTCRQ> .

imajit · 2021-07-18T07:41:41Z

qlty-cloud-legacy · 2021-07-18T07:41:53Z

app/controllers/user_tags_controller.rb

        @output[:errors] << I18n.t('user_tags_controller.value_cannot_be_empty')
      end
+      if params[:translationswitch]
+        redirect_to URI.parse('/change_locale/zh-CN').path and return


Use && instead of and.

qlty-cloud-legacy · 2021-07-18T07:41:53Z

app/controllers/user_tags_controller.rb

    end

+    if params[:translationswitch]
+      redirect_to URI.parse('/change_locale/en').path and return


Use && instead of and.

qlty-cloud-legacy · 2021-07-18T07:41:55Z

Code Climate has analyzed commit 0047d80 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Style	2

View more on Code Climate.

noi5e · 2021-07-19T03:54:34Z

Looks good to me! @jywarren, the Switch on Translation button is now in the nav, so I believe this one is merge-ready

jywarren · 2021-07-20T18:59:32Z

Excellent, thank you so much!!!

* Added check verify translation button * Adding spaces * Added route tests * Reusing existing routes * Removed tests * Moved button to Top-Nav * Moved button to profile dropdown Co-authored-by: imajit <[email protected]>

github-actions bot added the work-in-progress label Jul 2, 2021

qlty-cloud-legacy bot reviewed Jul 2, 2021

View reviewed changes

github-actions bot added ready-for-review work-in-progress and removed work-in-progress labels Jul 2, 2021

noi5e approved these changes Jul 12, 2021

View reviewed changes

github-actions bot added the work-in-progress label Jul 12, 2021

imajit added 3 commits July 13, 2021 02:21

Added check verify translation button

854ddf3

Adding spaces

214c56e

Added route tests

200ef11

imajit force-pushed the verifytranslationbutton branch from 58eaa73 to 200ef11 Compare July 12, 2021 20:53

jywarren reviewed Jul 13, 2021

View reviewed changes

imajit added 2 commits July 16, 2021 03:01

Reusing existing routes

add138e

Removed tests

90ca424

github-actions bot removed the work-in-progress label Jul 15, 2021

Moved button to Top-Nav

599d05c

Moved button to profile dropdown

0047d80

imajit requested a review from a team as a code owner July 18, 2021 07:40

github-actions bot added the work-in-progress label Jul 18, 2021

qlty-cloud-legacy bot reviewed Jul 18, 2021

View reviewed changes

github-actions bot removed the work-in-progress label Jul 18, 2021

imajit mentioned this pull request Jul 19, 2021

UI test for Translation Function #9856

Merged

jywarren merged commit 23d4e63 into main Jul 20, 2021

Verify Translation in Development #9878

Verify Translation in Development #9878

Uh oh!

Conversation

imajit commented Jul 2, 2021

Uh oh!

gitpod-io bot commented Jul 2, 2021

Uh oh!

qlty-cloud-legacy bot Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

noi5e commented Jul 12, 2021

Uh oh!

imajit commented Jul 13, 2021

Uh oh!

jywarren Jul 13, 2021

Choose a reason for hiding this comment

Uh oh!

jywarren commented Jul 13, 2021

Uh oh!

jywarren Jul 13, 2021

Choose a reason for hiding this comment

Uh oh!

imajit commented Jul 15, 2021

Uh oh!

jywarren commented Jul 16, 2021

Uh oh!

imajit commented Jul 16, 2021

Uh oh!

Tlazypanda commented Jul 17, 2021

Uh oh!

imajit commented Jul 17, 2021

Uh oh!

jywarren commented Jul 17, 2021 via email

Uh oh!

imajit commented Jul 18, 2021

Uh oh!

qlty-cloud-legacy bot Jul 18, 2021

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot Jul 18, 2021

Choose a reason for hiding this comment

Uh oh!

qlty-cloud-legacy bot commented Jul 18, 2021

Uh oh!

noi5e commented Jul 19, 2021

Uh oh!

jywarren commented Jul 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Jul 2, 2021 •

edited

Loading