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

Fix Bug 1666199 Unify download URL schema #1692

Conversation

anujpandey785
Copy link
Contributor

1)Changed the url for translation memory (line 133) in UserMenu.js file
2)Changed the url for translations (line 153) in UserMenu.js file
3)Removed the parameters locale and slug passed in the url for
downloading translation memory (line 59) in pontoon/base/ urls.py
4)Modified the view download_translation_memory in pontoon/base/views.py
to accept only 2 parameters i.e. request and filename and introduced a
new variable vars to extract locale and slug using split function.
5)Changed the view name download in pontoon/base/views.py to translations
(line 694) and changed the urls,name corresponding to it in
pontoon/base/ urls.py (line 81)

1)Changed the url for translation memory (line 133)  in UserMenu.js file
2)Changed the url for translations (line 153)  in UserMenu.js file
3)Removed the parameters locale and slug passed in the url for
  downloading translation memory (line 59)  in pontoon/base/ urls.py
4)Modified the view download_translation_memory in pontoon/base/views.py
  to accept only 2 parameters i.e. request and filename and introduced a
  new variable vars to extract locale and slug using split function.
5)Changed the view name download in pontoon/base/views.py to translations
  (line 694) and changed the urls,name corresponding to it in
  pontoon/base/ urls.py (line 81)
@@ -56,7 +56,7 @@
),
# Download translation memory
url(
r"^(?P<locale>[A-Za-z0-9\-\@\.]+)/(?P<slug>[\w-]+)/(?P<filename>.+)\.tmx$",
r"^translation-memory/(?P<filename>.+)\.tmx$",
Copy link
Collaborator

@jotes jotes Oct 1, 2020

Choose a reason for hiding this comment

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

hey @anujpandey785
The patch looks good, but I have a small suggestion that potentially can reduce the amount of code.

The regex for this contains a variable that can be removed because it gets overridden in the view.

r"^translation-memory/(?P<locale>[A-Za-z0-9\-\@\.]+)\.(?P<slug>[\w-]+)\.tmx$",

I think it's safe to leave only locale and slug as the view parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hii @jotes
Thanks for reviewing the patch .Sorry, I had overlooked the fact that the variable filename is being overridden. Yeah that helps us reducing 5 lines of code I have made the suggested changes , pushing the changes now. Tysm :)

Copy link
Contributor Author

@anujpandey785 anujpandey785 Oct 1, 2020

Choose a reason for hiding this comment

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

@jotes Done
But as we have changed the url for downloading the translational memory it would have its effect on button for downloading translation memory located in the project dashboard as well , we should change the url there too ?
Also, I am not sure about what's causing it but the tests which are failing in the Travis CI, do we also need to make changes in the code for the failing test cases( the test_tmx.py file ) ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll take a look at it today :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, the tests are failing because of the changes in the URLs. I think it's a good idea to use reverse and replace the hardcoded URLs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, the tests are failing because of the changes in the URLs. I think it's a good idea to use reverse and replace the hardcoded URLs.

Thanks for the suggestion. I am working on it.

1)Restored the variables locale and slug and deleted the variable filename
(as it is being overridden in the view) from the regex passed in the url for
 downloading translational memory (line 59) in the file pontoon/base/urls.py
2)Consequently added locale and slug parameters and removed the filename parameter
  in the view  download_translation_memory (line 758) in the file
  pontoon/base/views.py
Copy link
Collaborator

@mathjazz mathjazz left a comment

Choose a reason for hiding this comment

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

Thanks for the patch.

I've noticed the following issues:

  • I'm unable to download TM from the dashboards (locale code is included in the URL)
  • I'm unable to download translations (404)

Please also fix the tests.

@@ -78,7 +78,7 @@
views.get_translations_from_other_locales,
name="pontoon.other_locales",
),
url(r"^download/", views.download, name="pontoon.download"),
url(r"^translations/", views.translations, name="pontoon.translations"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd make that less ambiguous. Maybe views.download_translations and pontoon.download.translations?

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 should have taken a hint from download_translation_memory

@anujpandey785
Copy link
Contributor Author

anujpandey785 commented Oct 3, 2020

  • I'm unable to download translations (404)

@mathjazz Could you please elaborate more about the error in this part as i am able to download translations in my local copy.

  • Is the Download translations option in the user menu giving a 404 error?

@mathjazz
Copy link
Collaborator

mathjazz commented Oct 5, 2020

Is the Download translations option in the user menu giving a 404 error?

Correct.

1)Modified the harcoded url in the download button on the dashboard
  to Download Translation Memory in heading_info.html(Line 145)
2)Changed the view name for download translations from ```translations```
  to ```download_translations`` also made corresponding changes to view
  name to ```pontoon.download.translations``` in base/urls.py (Line 83 and 84)
3)Modified test_tmx.py namely the tests named ``` test_view_tmx_locale_file_dl```
  and ```test_view_tmx_bad_params``` by removing the hardcoded urls and making
  use of reverse.
Copy link
Collaborator

@mathjazz mathjazz left a comment

Choose a reason for hiding this comment

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

Well done!

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.

3 participants