Skip to content
This repository has been archived by the owner on Jan 2, 2020. It is now read-only.

Added show raw mail function #739

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Added show raw mail function #739

wants to merge 18 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 3, 2016

No description provided.

@ayoyoness
Copy link
Contributor

Hi @eikeniebuhr and @krunoknego,

Thanks a lot for working on this.
It looks good.
I have a few comments:

  1. Could you please make a separate PR for the Swedish translation? It is great but It is better if it goes on its own pull request as it is clearly not related.
  2. It would be good if you can squash the commits touching the same domain, and give a more meaningful commit message. Small commits are great but in the sense that things that naturally belong to each other should go together. For example, there are two successive 'pr fix' commits that does css... there are a number of successive commits for translations... maybe one -separate- commit each for the frontend and backend codes, if possible to separate.
  3. The CI is breaking. There is jshint error: app/js/services/mail_service.js: line 292, col 8, Missing semicolon.
  4. It would also be great if you add tests on both the frontend and backend
  5. I think "get raw mail" sounds better than "get mail raw", doesn't it? :)

Thanks again,
We're so excited you got the ball rolling on this.

@ghost
Copy link
Author

ghost commented Jul 4, 2016

@mnandri Thanks for the reply. We'll resolve the issues as soon as possible. :)

@re-nobre re-nobre added the PR label Jul 4, 2016
@ayoyoness
Copy link
Contributor

ayoyoness commented Jul 4, 2016

@krunoknego
great!
You can find us on IRC (#pixelated on irc.freenode.net), with a slightly faster response time... if you need any help or have queries.

KrunoKnego added 3 commits July 8, 2016 00:03
Conflicts:
	web-ui/app/locales/de_DE/translation.json
	web-ui/app/locales/es_ES/translation.json
@tuliocasagrande
Copy link
Collaborator

Hello @eikeniebuhr and @krunoknego,

Thanks a lot for working on this.
I just wanted to add something concerning translations.

Recently we moved to Transifex for managing our translations. It's easier for maintenance and receiving non-developers contributions.

We decided not to work with translations using git commits. So could you please submit your sv_SE translation to https://www.transifex.com/pixelated/pixelated-user-agent/dashboard/. The only thing you need to git commit is the en_US new tag "get-mail-raw": "Get mail raw",.

Please feel free to contact me if you need any help.

@thaissiqueira
Copy link
Member

Hey @eikeniebuhr and @krunoknego,

How are you doing?

I just installed your fork in my machine and I loved your contribution! (I'm bit late :P)
I saw that you did some of @ayoyoness and @tuliocasagrande's suggestions and will be awesome if you could do the remainder of them.
To keep this code easier to maintain, as a community code, it's super important for us to follow the contributor's guide.
I really hope this merge could be done soon.

@ghost
Copy link
Author

ghost commented Jan 11, 2017

Hello @thaissiqueira,

Sorry for taking it so long to respond. We will resolve all the issues that are still remaining and hopefully we'll have something that is merge ready either today or tomorrow.

@thaissiqueira
Copy link
Member

Cool! Glad for hear it. Thanks for the collaboration.

@ghost
Copy link
Author

ghost commented Jan 13, 2017

It would seem that the CI build is failing because of the code style. I've used autopep8 to automatically fix it (https://pypi.python.org/pypi/autopep8). Can you guide me with what library/command I should fix the code style?

@anikarni
Copy link
Member

Actually, @krunoknego , this is because we changed how we run tests in our snap-ci and your PR wasn't updated. I'm fixing that and you should have a better feedback in a few minutes.

Copy link
Member

@anikarni anikarni left a comment

Choose a reason for hiding this comment

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

Hi @krunoknego and @eikeniebuhr , again, thanks so much for the PR and we really hope to merge it soon.

I fixed snap-ci, so it should be okay now (sorry about that), but I still have a few small comments. These should be quick fixes (mostly just about plain text).

Lastly, I just wanted to ask if you could add a functional test for this. How does that sound?

@@ -1,4 +1,4 @@
Pixelated User Agent Service
ixelated User Agent Service
Copy link
Member

Choose a reason for hiding this comment

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

The "P" here has been removed, I believe, accidentally.

if not os.path.isdir(path):
os.makedirs(path, 0700)
os.makedirs(path, 0o700)
Copy link
Member

Choose a reason for hiding this comment

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

Any reason this changed?

@@ -4,7 +4,7 @@
from twisted.web.resource import Resource
from twisted.web.server import NOT_DONE_YET

from pixelated.resources import respond_json_deferred, BaseResource, handle_error_deferred
from pixelated.resources import respond_json_deferred, respond_json, BaseResource, handle_error_deferred
Copy link
Member

Choose a reason for hiding this comment

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

I believe the import that was added here wasn't actually needed or used. Or is it just not in the diff?

@@ -145,7 +152,8 @@ def getChild(self, action, request):
if action == 'unread':
return MailsUnreadResource(_mail_service)

def _build_mails_response(self, (mails, total)):
def _build_mails_response(self, xxx_todo_changeme):
Copy link
Member

Choose a reason for hiding this comment

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

Is this variable name supposed to have been changed? hehe

@@ -28,6 +28,7 @@
"mark-as-read": "Marcar como lida",
"mark-as-unread": "Marcar como não lida",
"delete": "Deletar",
"get-raw-mail": "Get raw mail",
Copy link
Member

Choose a reason for hiding this comment

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

This can be "Mostrar original"

@@ -28,6 +28,7 @@
"mark-as-read": "Mark as read",
"mark-as-unread": "Mark as unread",
"delete": "Delete",
"get-raw-mail": "Get raw mail",
Copy link
Member

Choose a reason for hiding this comment

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

This can be "View raw email"

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants