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

Require user to select category before uploading #2290

Merged
merged 9 commits into from
Oct 1, 2015
Merged

Require user to select category before uploading #2290

merged 9 commits into from
Oct 1, 2015

Conversation

ccosse
Copy link
Contributor

@ccosse ccosse commented Sep 30, 2015

I'm not sure how/where upload.js gets included, but have here added cookie.js alongside upload.js.

2. Require user to choose category before uploading (upload.js)
3. Pass chosen category via cookie (cookie.js, upload.js)
4. Parse category_id from cookie in POST section of views.layer_upload
2. Require user to choose category before uploading (upload.js)
3. Pass chosen category via cookie (cookie.js, upload.js)
4. Parse category_id from cookie in POST section of views.layer_upload
@gamesbook
Copy link
Contributor

You need to be consistent and follow convention wrt to white space; for example:

category_form=CategoryForm(prefix = "category_choice_field", initial= None)

should be:

category_form = CategoryForm(prefix="category_choice_field", initial=None)

i.e. spaces around assignment to variable (= on the left) and no space when assigning to method parameters (= inside the brackets).

@ccosse
Copy link
Contributor Author

ccosse commented Sep 30, 2015

@gamesbook Thanks! Fixing 2 spaces eliminated 20+ errors. Good to see and know how that can go.

jj0hns0n added a commit that referenced this pull request Oct 1, 2015
Require user to select category before uploading
@jj0hns0n jj0hns0n merged commit e35884b into GeoNode:master Oct 1, 2015
@pjdufour
Copy link
Member

pjdufour commented Oct 1, 2015

@ccosse, In the future could you squash your whitespace fix commits and then do a git force push to update to the PR branch. It's a little complicated but easy once you get the hang of it. Travis will automatically restart on a force push.

@ccosse
Copy link
Contributor Author

ccosse commented Oct 1, 2015

Okay I'll figure it out. Thanks.

On Thu, Oct 1, 2015 at 2:46 PM, Patrick Dufour notifications@github.com
wrote:

In the future could you squash your whitespace fix commits and then do a
git force push to update to the PR branch. It's a little complicated but
easy once you get the hang of it. Travis will automatically restart on a
force push.


Reply to this email directly or view it on GitHub
#2290 (comment).

@pjdufour
Copy link
Member

pjdufour commented Oct 1, 2015

Thanks! Happy to assist if you have any future questions.

@BerryDaniel
Copy link

@jj0hns0n @ingenieroariel

I know this passed unit tests, but has anyone tested this out. I used the latest git pull and attempted to upload a tiff, selected a category and received no response.... can someone verify this also?

@jj0hns0n
Copy link
Member

jj0hns0n commented Oct 2, 2015

I gave it a brief test, but did not test with a raster. Ill do some more
testing and we can revert if things are indeed broken.

On Thu, Oct 1, 2015 at 6:45 PM, Daniel Berry notifications@github.com
wrote:

@jj0hns0n https://github.com/jj0hns0n @ingenieroariel
https://github.com/ingenieroariel

I know this passed unit tests, but has anyone tested this out. I used the
latest git pull and attempted to upload a tiff, selected a category and
received no response.... can someone verify this also?


Reply to this email directly or view it on GitHub
#2290 (comment).

@ccosse
Copy link
Contributor Author

ccosse commented Oct 2, 2015

Hi, what's the best way to put something out there for review before
merging? (i.e. this PR). Should I just request to be reviewed discussed
via message attached to the PR itself, or is there another way? I tested
.tif and .zip uploads from Linux only using Firefox. While I'm not aware
of anything "broken", I still plan to improve upon this current
implementation. Is this the way to go about it, i.e. submit a PR and
discuss it from there? That was my intention.

Specific future improvements are:

  1. Use Django's cookie protocol vs. custom one I introduced
  2. Eliminate use of cookie-passing in favor of CategoryForm
  3. More elegant parsing of category-cookie in views.py

That said, it works fine for me so if it's not broken then I'd also like to
see it remain merged, but not closed. Is that possible?
Thanks!
-Charles

On Fri, Oct 2, 2015 at 7:24 AM, Jeffrey Johnson notifications@github.com
wrote:

I gave it a brief test, but did not test with a raster. Ill do some more
testing and we can revert if things are indeed broken.

On Thu, Oct 1, 2015 at 6:45 PM, Daniel Berry notifications@github.com
wrote:

@jj0hns0n https://github.com/jj0hns0n @ingenieroariel
https://github.com/ingenieroariel

I know this passed unit tests, but has anyone tested this out. I used the
latest git pull and attempted to upload a tiff, selected a category and
received no response.... can someone verify this also?


Reply to this email directly or view it on GitHub
#2290 (comment).


Reply to this email directly or view it on GitHub
#2290 (comment).

@ccosse
Copy link
Contributor Author

ccosse commented Oct 2, 2015

Yes, I just did a complete/fresh re-install and indeed Cookie.js was not
loaded. In my own defense, that was the one thing I did mention in the
initial PR message ...

It's a simple fix but still not sure where cookie.js gets loaded
(introduced alongside ../static/geonode/js/upload/upload.js)

On Fri, Oct 2, 2015 at 8:27 AM, Charles Cossé ccosse@gmail.com wrote:

Hi, what's the best way to put something out there for review before
merging? (i.e. this PR). Should I just request to be reviewed discussed
via message attached to the PR itself, or is there another way? I tested
.tif and .zip uploads from Linux only using Firefox. While I'm not aware
of anything "broken", I still plan to improve upon this current
implementation. Is this the way to go about it, i.e. submit a PR and
discuss it from there? That was my intention.

Specific future improvements are:

  1. Use Django's cookie protocol vs. custom one I introduced
  2. Eliminate use of cookie-passing in favor of CategoryForm
  3. More elegant parsing of category-cookie in views.py

That said, it works fine for me so if it's not broken then I'd also like
to see it remain merged, but not closed. Is that possible?
Thanks!
-Charles

On Fri, Oct 2, 2015 at 7:24 AM, Jeffrey Johnson notifications@github.com
wrote:

I gave it a brief test, but did not test with a raster. Ill do some more
testing and we can revert if things are indeed broken.

On Thu, Oct 1, 2015 at 6:45 PM, Daniel Berry notifications@github.com
wrote:

@jj0hns0n https://github.com/jj0hns0n @ingenieroariel
https://github.com/ingenieroariel

I know this passed unit tests, but has anyone tested this out. I used
the
latest git pull and attempted to upload a tiff, selected a category and
received no response.... can someone verify this also?


Reply to this email directly or view it on GitHub
#2290 (comment).


Reply to this email directly or view it on GitHub
#2290 (comment).

@jj0hns0n
Copy link
Member

jj0hns0n commented Oct 2, 2015

Can you post to the dev list to ask for help. I dont have time to look at
this today after all.

On Fri, Oct 2, 2015 at 12:14 PM, Charles B. Cossé notifications@github.com
wrote:

Yes, I just did a complete/fresh re-install and indeed Cookie.js was not
loaded. In my own defense, that was the one thing I did mention in the
initial PR message ...

It's a simple fix but still not sure where cookie.js gets loaded
(introduced alongside ../static/geonode/js/upload/upload.js)

On Fri, Oct 2, 2015 at 8:27 AM, Charles Cossé ccosse@gmail.com wrote:

Hi, what's the best way to put something out there for review before
merging? (i.e. this PR). Should I just request to be reviewed discussed
via message attached to the PR itself, or is there another way? I tested
.tif and .zip uploads from Linux only using Firefox. While I'm not aware
of anything "broken", I still plan to improve upon this current
implementation. Is this the way to go about it, i.e. submit a PR and
discuss it from there? That was my intention.

Specific future improvements are:

  1. Use Django's cookie protocol vs. custom one I introduced
  2. Eliminate use of cookie-passing in favor of CategoryForm
  3. More elegant parsing of category-cookie in views.py

That said, it works fine for me so if it's not broken then I'd also like
to see it remain merged, but not closed. Is that possible?
Thanks!
-Charles

On Fri, Oct 2, 2015 at 7:24 AM, Jeffrey Johnson <
notifications@github.com>
wrote:

I gave it a brief test, but did not test with a raster. Ill do some more
testing and we can revert if things are indeed broken.

On Thu, Oct 1, 2015 at 6:45 PM, Daniel Berry notifications@github.com
wrote:

@jj0hns0n https://github.com/jj0hns0n @ingenieroariel
https://github.com/ingenieroariel

I know this passed unit tests, but has anyone tested this out. I used
the
latest git pull and attempted to upload a tiff, selected a category
and
received no response.... can someone verify this also?


Reply to this email directly or view it on GitHub
<#2290 (comment)
.


Reply to this email directly or view it on GitHub
#2290 (comment).


Reply to this email directly or view it on GitHub
#2290 (comment).

@simod
Copy link
Member

simod commented Oct 6, 2015

@ccosse adding here the comment, the layer upload is broken due to key error 'category' we should fix it asap.
Thanks!

@ccosse
Copy link
Contributor Author

ccosse commented Oct 6, 2015

Simone, I apologize then ... I will look at it first thing when I get to
work in 1 hour. There was PR #2295 which (I thought) fixed #2290 ... but
this doesn't sound like the same thing. Anyway I will definitely fix this
morning. -Charles

On Tue, Oct 6, 2015 at 6:19 AM, Simone Dalmasso notifications@github.com
wrote:

@ccosse https://github.com/ccosse adding here the comment, the layer
upload is broken due to key error 'category' we should fix it asap.
Thanks!


Reply to this email directly or view it on GitHub
#2290 (comment).

@ccosse
Copy link
Contributor Author

ccosse commented Oct 6, 2015

Simone, can you tell me any more info? I just tested on Firefox and Chrome
and it works fine. Not to say that there is no problem, just that I don't
currently have any errors. Was that a django error from layers/views.py
or in javascript console? I'm still on my way to check it out, just
seeking a little more info. Thanks.

On Tue, Oct 6, 2015 at 6:19 AM, Simone Dalmasso notifications@github.com
wrote:

@ccosse https://github.com/ccosse adding here the comment, the layer
upload is broken due to key error 'category' we should fix it asap.
Thanks!


Reply to this email directly or view it on GitHub
#2290 (comment).

@simod
Copy link
Member

simod commented Oct 6, 2015

Thanks for taking a look,
on layer upload I pick a category and submit, looking at the http request
to layers/upload the "category" for doesn't seem to be submitted.

Traceback (most recent call last):

File "/opt/master-geonode/geonode/layers/views.py", line 155, in
layer_upload

topic_id = form.cleaned_data["category"]

KeyError: 'category'

2015-10-06 13:12 GMT+02:00 Charles B. Cossé notifications@github.com:

Simone, can you tell me any more info? I just tested on Firefox and Chrome
and it works fine. Not to say that there is no problem, just that I don't
currently have any errors. Was that a django error from layers/views.py
or in javascript console? I'm still on my way to check it out, just
seeking a little more info. Thanks.

On Tue, Oct 6, 2015 at 6:19 AM, Simone Dalmasso notifications@github.com
wrote:

@ccosse https://github.com/ccosse adding here the comment, the layer
upload is broken due to key error 'category' we should fix it asap.
Thanks!


Reply to this email directly or view it on GitHub
#2290 (comment).


Reply to this email directly or view it on GitHub
#2290 (comment).

Simone

@ccosse
Copy link
Contributor Author

ccosse commented Oct 6, 2015

Should be fixed. I forgot that there had been changes to layers/views.py
at the time of PR #2295.

On Tue, Oct 6, 2015 at 7:15 AM, Simone Dalmasso notifications@github.com
wrote:

to layers/upload the "category" for doesn't seem to be submitted.

It is true that the category form is not being submitted ... I resorted to
passing the selected category via cookie (cookie.js and upload.js) until I
can figure out how to add the category form to the file-upload's formset.
I spent a lot of time on that part but without success (yet). I plan to
revisit that soon and any suggestion about how to get the category form
included in the formset would be great. Otherwise I'll still figure it out
with time.

Traceback (most recent call last):

File "/opt/master-geonode/geonode/layers/views.py", line 155, in
layer_upload

topic_id = form.cleaned_data["category"]

KeyError: 'category'

2015-10-06 13:12 GMT+02:00 Charles B. Cossé notifications@github.com:

Simone, can you tell me any more info? I just tested on Firefox and
Chrome
and it works fine. Not to say that there is no problem, just that I don't
currently have any errors. Was that a django error from layers/views.py
or in javascript console? I'm still on my way to check it out, just
seeking a little more info. Thanks.

On Tue, Oct 6, 2015 at 6:19 AM, Simone Dalmasso <
notifications@github.com>
wrote:

@ccosse https://github.com/ccosse adding here the comment, the layer
upload is broken due to key error 'category' we should fix it asap.
Thanks!


Reply to this email directly or view it on GitHub
#2290 (comment).


Reply to this email directly or view it on GitHub
#2290 (comment).

Simone


Reply to this email directly or view it on GitHub
#2290 (comment).

@simod
Copy link
Member

simod commented Oct 6, 2015

ok, are you going to open a PR?

2015-10-06 13:56 GMT+02:00 Charles B. Cossé notifications@github.com:

Should be fixed. I forgot that there had been changes to layers/views.py
at the time of PR #2295.

On Tue, Oct 6, 2015 at 7:15 AM, Simone Dalmasso notifications@github.com
wrote:

to layers/upload the "category" for doesn't seem to be submitted.

It is true that the category form is not being submitted ... I resorted to
passing the selected category via cookie (cookie.js and upload.js) until I
can figure out how to add the category form to the file-upload's formset.
I spent a lot of time on that part but without success (yet). I plan to
revisit that soon and any suggestion about how to get the category form
included in the formset would be great. Otherwise I'll still figure it out
with time.

Traceback (most recent call last):

File "/opt/master-geonode/geonode/layers/views.py", line 155, in
layer_upload

topic_id = form.cleaned_data["category"]

KeyError: 'category'

2015-10-06 13:12 GMT+02:00 Charles B. Cossé notifications@github.com:

Simone, can you tell me any more info? I just tested on Firefox and
Chrome
and it works fine. Not to say that there is no problem, just that I
don't
currently have any errors. Was that a django error from layers/views.py
or in javascript console? I'm still on my way to check it out, just
seeking a little more info. Thanks.

On Tue, Oct 6, 2015 at 6:19 AM, Simone Dalmasso <
notifications@github.com>
wrote:

@ccosse https://github.com/ccosse adding here the comment, the
layer
upload is broken due to key error 'category' we should fix it asap.
Thanks!


Reply to this email directly or view it on GitHub
<#2290 (comment)
.


Reply to this email directly or view it on GitHub
#2290 (comment).

Simone


Reply to this email directly or view it on GitHub
#2290 (comment).


Reply to this email directly or view it on GitHub
#2290 (comment).

Simone

@simod
Copy link
Member

simod commented Oct 22, 2015

This clashes with the multiple layer upload as they may have different categories, any chance we find a solution? Otherwise we'll have to revert.

@ccosse
Copy link
Contributor Author

ccosse commented Oct 22, 2015

Oh boy. Okay I can try to fix it later today. I'll let you know.

On Thu, Oct 22, 2015 at 5:34 AM, Simone Dalmasso notifications@github.com
wrote:

This clashes with the multiple layer upload as they may have different
categories, any chance we find a solution? Otherwise we'll have to revert.


Reply to this email directly or view it on GitHub
#2290 (comment).

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.

6 participants