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.

Sign up for GitHub

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

Jump to bottom

Add frontend for working with storage #18

Merged
merged 3 commits into from
Jul 27, 2021
Merged

Add frontend for working with storage #18

merged 3 commits into from
Jul 27, 2021

Conversation

romanhabibov
Copy link
Contributor

Implement GET method and simple html frontend. Now, it is possible
to work with the storage using a browser.

Closes #10

@romanhabibov romanhabibov requested a review from LeonidVas June 9, 2021 01:54
@Totktonada
Copy link
Member

Please, remove the .DS_Store file.

How icons you added are licensed and who is their author?

Whether index.html is derived work (based on some other work)? If so, how it is licensed and who is the author?

@romanhabibov romanhabibov force-pushed the romanhabibov/index branch 2 times, most recently from 50932d4 to f5dd55c Compare June 14, 2021 16:33
LeonidVas
LeonidVas requested changes Jun 16, 2021
View reviewed changes
Copy link
Contributor

@LeonidVas LeonidVas left a comment

Choose a reason for hiding this comment

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

Hi! Thank you for the patch. I recommend you use the "flake8" tool to check your code. And read the "PEP8".
See some comments bellow:

@LeonidVas
Copy link
Contributor

Please, remove the .DS_Store file.

How icons you added are licensed and who is their author?

Whether index.html is derived work (based on some other work)? If so, how it is licensed and who is the author?

+1
@romanhabibov ?

@LeonidVas
Copy link
Contributor

LeonidVas commented Jun 16, 2021 •
edited
Loading

Yet few comments / thoughts:

  • We should not show the entire bucket, but start from the bucket/base/path (S3_BASE_PATH)
  • Maybe we want to have a filter for files that shouldn't be shown (what do you think @Totktonada)
  • Split the patch into commits

LeonidVas
LeonidVas reviewed Jun 17, 2021
View reviewed changes
LeonidVas
LeonidVas reviewed Jun 17, 2021
View reviewed changes
@LeonidVas
Copy link
Contributor

Please, remove the .DS_Store file.
How icons you added are licensed and who is their author?
Whether index.html is derived work (based on some other work)? If so, how it is licensed and who is the author?

+1
@romanhabibov ?

I think we should use something like this one.

LeonidVas
LeonidVas reviewed Jun 17, 2021
View reviewed changes
LeonidVas
LeonidVas reviewed Jun 17, 2021
View reviewed changes
LeonidVas
LeonidVas reviewed Jun 17, 2021
View reviewed changes
@romanhabibov romanhabibov force-pushed the romanhabibov/index branch 2 times, most recently from 8c02ca2 to 5c4f4ae Compare June 26, 2021 10:27
@romanhabibov
Copy link
Contributor Author

Please, remove the .DS_Store file.
How icons you added are licensed and who is their author?
Whether index.html is derived work (based on some other work)? If so, how it is licensed and who is the author?

+1
@romanhabibov ?

I think we should use something like this one.

I used icons from https://icons.getbootstrap.com/ and added the license.

@romanhabibov
Copy link
Contributor Author

romanhabibov commented Jun 26, 2021 •
edited
Loading

Yet few comments / thoughts:

  • We should not show the entire bucket, but start from the bucket/base/path (S3_BASE_PATH)

Done.

  • Maybe we want to have a filter for files that shouldn't be shown (what do you think @Totktonada)

???

  • Split the patch into commits

Done.

@romanhabibov romanhabibov self-assigned this Jun 26, 2021
LeonidVas
LeonidVas requested changes Jul 2, 2021
View reviewed changes
@LeonidVas
Copy link
Contributor

Why do you ignore comments:
1522eb2#r652537052
1522eb2#r652689089

@LeonidVas
Copy link
Contributor

Yet few comments / thoughts:

  • We should not show the entire bucket, but start from the bucket/base/path (S3_BASE_PATH)

Done.

No. I can still see the whole bucket

(rws) ┌─[leonid@vasya-hp]─[~/work/mail/rws]
└──╼ env | grep S3_BASE_PATH
S3_BASE_PATH=restored

image

@LeonidVas
Copy link
Contributor

  • Maybe we want to have a filter for files that shouldn't be shown (what do you think @Totktonada)

???

For example (no legal) we only want to display files with ".deb and .rpm" extensions.

@romanhabibov romanhabibov force-pushed the romanhabibov/index branch 2 times, most recently from ba481dd to 7ba17de Compare July 8, 2021 10:53
@romanhabibov
Copy link
Contributor Author

Yet few comments / thoughts:

  • We should not show the entire bucket, but start from the bucket/base/path (S3_BASE_PATH)

Done.

No. I can still see the whole bucket

(rws) ┌─[leonid@vasya-hp]─[~/work/mail/rws]
└──╼ env | grep S3_BASE_PATH
S3_BASE_PATH=restored

image

Done. See the current version.

@romanhabibov
Copy link
Contributor Author

  • Maybe we want to have a filter for files that shouldn't be shown (what do you think @Totktonada)

???

For example (no legal) we only want to display files with ".deb and .rpm" extensions.

Let's do it as a separate issue.

@romanhabibov romanhabibov requested a review from LeonidVas July 8, 2021 11:10
@LeonidVas LeonidVas mentioned this pull request Jul 12, 2021
Closed
LeonidVas
LeonidVas requested changes Jul 12, 2021
View reviewed changes
Copy link
Contributor

@LeonidVas LeonidVas left a comment

Choose a reason for hiding this comment

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

From @Totktonada:

  • CSS is uploaded every time (needs to be cached)
  • CSS has a path with two slashes
  • All fields must be left aligned
  • The date field must be large enough to contain the date

@romanhabibov
Copy link
Contributor Author

From @Totktonada:

  • CSS is uploaded every time (needs to be cached)

added SEND_FILE_MAX_AGE_DEFAULT.

  • CSS has a path with two slashes

Fixed.

  • All fields must be left aligned

Done. In css file.

  • The date field must be large enough to contain the date

Now it contains.

@romanhabibov romanhabibov requested a review from LeonidVas July 19, 2021 09:46
@LeonidVas LeonidVas closed this Jul 23, 2021
@LeonidVas
Copy link
Contributor

LeonidVas commented Jul 23, 2021 •
edited
Loading

  • The date field must be large enough to contain the date

Now it contains.

image

Why don't you use one row for date and for size (it usually contains 2 rows now)? As it was before

LeonidVas
LeonidVas reviewed Jul 23, 2021
View reviewed changes
Copy link
Contributor

@LeonidVas LeonidVas left a comment

Choose a reason for hiding this comment

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

Sorry I will reopen PR.

@LeonidVas LeonidVas reopened this Jul 23, 2021
LeonidVas
LeonidVas requested changes Jul 23, 2021
View reviewed changes
LeonidVas
LeonidVas requested changes Jul 23, 2021
View reviewed changes
@romanhabibov
s3controller: drop get method from controller
3610a14 
Drop it, because it will be implemented in the s3view.

Part of #10
@romanhabibov romanhabibov force-pushed the romanhabibov/index branch 3 times, most recently from ce9e0c0 to e3c5877 Compare July 23, 2021 16:21
@Totktonada
Copy link
Member

Why don't you use one row for date and for size (it usually contains 2 rows now)?

I guess the reason is here:

table {
<...>
    max-width: 700px;
<...>
}

LeonidVas
LeonidVas requested changes Jul 26, 2021
View reviewed changes
Copy link
Contributor

@LeonidVas LeonidVas left a comment

Choose a reason for hiding this comment

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

I think we are already close to the finish

s3repo/model.py Outdated
def get_file(self, path):
"""Download a file from S3."""
NotImplementedError("get_file hasn't been implemented yet.")
"""Get a file from S3 as a boto3 get_object() object."""
Copy link
Contributor

Choose a reason for hiding this comment

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

As StreamingBody - https://botocore.amazonaws.com/v1/documentation/api/latest/reference/response.html#botocore.response.StreamingBody

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

s3repo/view.py Outdated
return readable_size
size /= 1024.0

return readable_size
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be best to return something like "%3.1f B" % (size).

static/common/css/fileindex.css Outdated
margin-top: 2em;
min-width: 400px;
text-align: center;
width: 100%;
Copy link
Contributor

Choose a reason for hiding this comment

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

With width: 100%;
CSS_1
Without width: 100%;
CSS_2
As for me, the second is prettier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Second. Fixed.

@romanhabibov romanhabibov requested a review from LeonidVas July 26, 2021 11:31
@LeonidVas LeonidVas force-pushed the romanhabibov/index branch from d189ccd to a48282a Compare July 26, 2021 19:37
@LeonidVas
Copy link
Contributor

@romanhabibov I suggest making several fixups if you are ok, please, squash them in to your commits.

@romanhabibov
s3model: implement GET method
e58aa8c 
Implement GET method as the two S3AsyncModel class methods for
gettiing directory and file. These method will be used by view
to display directories and send files to user.

Part of #10
@romanhabibov
s3view: implement view
3224d20 
Implement view class and web frontend for working with model. This
class includes GET method for displaying direcories and sending
files.

Closes #10
LeonidVas
LeonidVas approved these changes Jul 27, 2021
View reviewed changes
Copy link
Contributor

@LeonidVas LeonidVas left a comment

Choose a reason for hiding this comment

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

LGTM.

@LeonidVas LeonidVas merged commit 8566e5e into master Jul 27, 2021
@LeonidVas LeonidVas deleted the romanhabibov/index branch July 27, 2021 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LeonidVas LeonidVas LeonidVas approved these changes

@Totktonada Totktonada Awaiting requested review from Totktonada

Assignees

@romanhabibov romanhabibov

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

infra: implement RPM / Deb packages listings
3 participants
@romanhabibov @Totktonada @LeonidVas