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

Translations download as zip(.po files) button fix #2445

Merged
merged 6 commits into from Apr 10, 2019

Conversation

mrsaicharan1
Copy link
Member

@mrsaicharan1 mrsaicharan1 commented Apr 1, 2019

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the Upstream development branch.
  • The acceptance, integration, unit tests and linter pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Short description of what this resolves:

Translations as zip can now be downloaded on the admin panel
Related server PR

Fixes #2443

@mrsaicharan1
Copy link
Member Author

Commit history is not related to this. Fixing the pr

@mrsaicharan1 mrsaicharan1 changed the title Translations download as zip(.po files) button fix [WIP]Translations download as zip(.po files) button fix Apr 1, 2019
Copy link
Contributor

@abhinavk96 abhinavk96 left a comment

Choose a reason for hiding this comment

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

A lot of unrelated commits from some other PRs. Sync your repo.

@mrsaicharan1 mrsaicharan1 force-pushed the translation-b branch 3 times, most recently from 51ce0fb to 53d89c2 Compare April 1, 2019 13:13
@mrsaicharan1
Copy link
Member Author

mrsaicharan1 commented Apr 1, 2019

@CosmicCoder96 Can you confirm if the link redirect is right here?

@mrsaicharan1 mrsaicharan1 changed the title [WIP]Translations download as zip(.po files) button fix Translations download as zip(.po files) button fix Apr 1, 2019
@abhinavk96
Copy link
Contributor

@mrsaicharan1 no - this link is supposed to be used for downloading actual .po files like you download pdfs for tickets. The URL has to come from the server where it will be stored.

@mrsaicharan1
Copy link
Member Author

mrsaicharan1 commented Apr 2, 2019

@CosmicCoder96 I've implemented the feature for downloading .po files here in this PR on the server. Won't routing it to /admin/content/translations/all do it?

@abhinavk96
Copy link
Contributor

abhinavk96 commented Apr 2, 2019

@mrsaicharan1 You created a namespace on the server - by specifying that the url_prefix is /admin/content/translations/all so you basically created an endpoint on the API. How will routing to that particular url-prefix on the frontend get the file -- that route doesn't exist. You are confusing ember FE routes with API endpoints. They are not interchangeable.

@mrsaicharan1
Copy link
Member Author

mrsaicharan1 commented Apr 2, 2019

@CosmicCoder96 Please correct me if I'm wrong, I'll be creating a model for the translations route where it fetches the data from server asynchronously. Then, I'll be setting the sourceUrl to this.store.findRecord('translations-link') for redirection. Am I right?

yarn lock fix

URL added for admin translations
@mrsaicharan1
Copy link
Member Author

@CosmicCoder96 Appended the url using the loader service.

@fossasia fossasia deleted a comment Apr 8, 2019
app/templates/admin/content/translations.hbs Outdated Show resolved Hide resolved
app/controllers/admin/content/translations.js Outdated Show resolved Hide resolved
Copy link
Contributor

@abhinavk96 abhinavk96 left a comment

Choose a reason for hiding this comment

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

Also, you are not using the info available in isLoading anywhere -- use it in conjunction with https://semantic-ui.com/elements/button.html#loading

app/controllers/admin/content/translations.js Outdated Show resolved Hide resolved
@mrsaicharan1 mrsaicharan1 force-pushed the translation-b branch 2 times, most recently from c6132fb to 79d7781 Compare April 8, 2019 08:37
Initialized isLoading

setting isLoading to False finally

moving logic to controller file

removed redundant variable assignments

Button isLoading added in template code

Added translations isloading
@mrsaicharan1
Copy link
Member Author

@CosmicCoder96
I believe that the downloads should be working but there's a permanent redirect for the resource.
127.0.0.1 - - [09/Apr/2019 13:21:16] "OPTIONS /v1/admin/content/translations/all HTTP/1.1" 308 -

@abhinavk96
Copy link
Contributor

@mrsaicharan1 what do you mean by permanent redirect?

@mrsaicharan1
Copy link
Member Author

@mrsaicharan1 what do you mean by permanent redirect?

HTTP 308

@abhinavk96
Copy link
Contributor

abhinavk96 commented Apr 9, 2019

@mrsaicharan1 Not sure how it's showing on your side: on testing this PR for me it shows a 404
vendor-6d3d450f0fa475b013d3067764daa7a9.js:8357 OPTIONS https://open-event-api-dev.herokuapp.com/v1/admin/content/translations/all? 404 (NOT FOUND)

@mrsaicharan1
Copy link
Member Author

@CosmicCoder96 I've tried out the API request locally and the download works properly when I navigate to https://localhost:5000/v1/admin/content/translations/all

@abhinavk96
Copy link
Contributor

@mrsaicharan1 I have redeployed the server on heroku -- I guess the build of your merged PR is still under progress there. Will test it in a few mins.

@abhinavk96
Copy link
Contributor

@mrsaicharan1 I got this locally, any ideas?
image

@mrsaicharan1
Copy link
Member Author

mrsaicharan1 commented Apr 9, 2019

@CosmicCoder96 I think it's due to the deletion of the zip file as a celery task. On the server side in app/api/admin_translations.py, if you comment out the lines for deletion as a celery task, the zip is being downloaded. Can you confirm this?
Screenshot from 2019-04-09 14-26-44

@abhinavk96
Copy link
Contributor

@mrsaicharan1 I will push a pr to create a download file service.. try it after that.

translationsDownload() {
this.set('isLoading', true);
this.get('loader')
.load('/admin/content/translations/all')
Copy link
Contributor

Choose a reason for hiding this comment

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

@mrsaicharan1 Use the service I defined in #2540
here.

this.get('loader')
        .downloadFile('/admin/content/translations/all')
        .then(()=> {
         this.get('notify').success(this.get('l10n').t('Translations Zip generated successfully.'));
          })

Copy link
Member Author

Choose a reason for hiding this comment

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

The downloader service helped a lot! It's working perfectly now :)
Screenshot from 2019-04-09 18-24-44

@abhinavk96
Copy link
Contributor

@mrsaicharan1 Open an issue on the server for the celery task of translation helpers. It is failing for the test deployment with the same error message which I posted a screenshot of before.

@mrsaicharan1
Copy link
Member Author

@CosmicCoder96 Fixed the translations donwload on the server fossasia/open-event-server#5767

@abhinavk96 abhinavk96 merged commit 2b62f63 into fossasia:development Apr 10, 2019
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.

admin:Translations download button not functional
3 participants