Skip to content

Conversation

@kziemianek
Copy link
Contributor

@kziemianek kziemianek commented Mar 30, 2018

Refs: #506

Description

Add pitch pages as described #506

Checklist
  • linter status: 100% pass
  • changes don't break existing behavior
  • commit message follows [commit guidelines]
Affected core subsystem(s)
  • frontend
  • backend
Testing

Performed manual local tests.

Refers/Fixes

Refs #506

@@ -0,0 +1,51 @@
<script id="idea_template" type="text/x-jsrender">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this file needs to be formated properly ...

github_username=request.POST.get('githubUsername'),
summary=request.POST.get('summary'),
more_info=request.POST.get('moreInfo'),
looking_for_capital=True, #request.POST.get('lookingForCapital') returns 'fasle' or 'true'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea how to serialize properly javascript's true/false to python's True/False, any ideas?

Copy link

@mkosowsk mkosowsk Mar 30, 2018

Choose a reason for hiding this comment

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

Per this

looking_for_capital = True if request.POST.get('lookingForCapital') == 'true' else False

or

looking_for_capital = request.POST.get('lookingForCaptail') == 'true'

could be good solutions? 🤔

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 saw this solution, i wonder if there is another...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean framework should provide proper deserializer, no manual checks spread across the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Neat that looks a lot cleaner 👍🏻

@codecov
Copy link

codecov bot commented Mar 30, 2018

Codecov Report

Merging #753 into master will increase coverage by 0.17%.
The diff coverage is 48.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #753      +/-   ##
==========================================
+ Coverage   30.25%   30.43%   +0.17%     
==========================================
  Files         125      126       +1     
  Lines        9013     9102      +89     
  Branches     1157     1161       +4     
==========================================
+ Hits         2727     2770      +43     
- Misses       6178     6224      +46     
  Partials      108      108
Impacted Files Coverage Δ
app/app/urls.py 94.28% <ø> (ø) ⬆️
app/retail/views.py 34.73% <24.07%> (-4.68%) ⬇️
app/app/settings.py 83.33% <66.66%> (+0.18%) ⬆️
app/retail/models.py 85.29% <85.29%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 367eb76...5ac2245. Read the comment docs.

@kziemianek
Copy link
Contributor Author

kziemianek commented Mar 30, 2018

Actual implementation fetches disqus thread's data on client side (most recent values, api limits may be exceeded) and there are no likes and posts counts in model so there is no way to perform sort.

To provide sorting functionality these values should be placed in model but syncing them with disqus may be problematic.

I can think of two syncing modes:

  1. values are updated periodically(no risk to exceed api limits, values may be outdated)
  2. values are updated during ideas list/idea fetch(api limits may be exceeded, most recent values, longer request times).

@mbeacom , @owocki does actual implementation or any of syncing modes satisfy You ?

@mkosowsk
Copy link

mkosowsk commented Mar 30, 2018

Looks great!!! :D

I defer to @owocki and @mbeacom but think option number 2 of values are updated during ideas list/idea fetch would be interesting to run with to start. Nice to have those recent values as part of feedback to the user and I don't think we'll be bumping up against API limits to start 🤔

@kziemianek
Copy link
Contributor Author

kziemianek commented Mar 31, 2018

I'll take a look at webhook later but seems like it's triggered only by new comments.

Unfortunately, webhooks are only available for WordPress plugin...

@mbeacom mbeacom added feature frontend This needs frontend expertise. backend This needs backend expertise. labels Apr 2, 2018
@kziemianek
Copy link
Contributor Author

So if we want that kind of sorting we need to somehow sync our model with disqus threads data.

As @mkosowsk suggested, fetchin disquss threads data right before ideas list fetch might not have a big impact at start (disqus api max page size of 100), but as soon as we hit 200, 300, 400+ ideas on list requests times will get longer. I think we can defer this optimization.

So if we stick with this solution then when request hits ideas/fetch following actions will be taken:

  1. disquss threads fetch
  2. ideas fetch
  3. ideas post_count and like_count update (if changed)
  4. ideas fetch with sorting applied

@owocki
Copy link
Contributor

owocki commented Apr 3, 2018

So if we want that kind of sorting we need to somehow sync our model with disqus threads data.

hopefully there is an API and we can just refresh the model to the disquis data on the serverside and not have to do it everytime a user hits the page

@kziemianek kziemianek force-pushed the pitch branch 3 times, most recently from 7bff4c2 to df128cd Compare April 3, 2018 21:51
@kziemianek
Copy link
Contributor Author

kziemianek commented Apr 3, 2018

syncing will occur every 10 minutes

@owocki owocki mentioned this pull request Apr 4, 2018
@owocki
Copy link
Contributor

owocki commented Apr 4, 2018

very excited for this.. let me know when ready to test

@kziemianek kziemianek changed the title WIP: pitch pages pitch pages Apr 4, 2018
@kziemianek
Copy link
Contributor Author

@owocki 🏁 ready to test

@kziemianek
Copy link
Contributor Author

@owocki hold on, I'll add i18n.

@kziemianek
Copy link
Contributor Author

@owocki finally 🏁

"""Define the structure of idea"""
full_name = models.CharField(max_length=255)
email = models.EmailField(max_length=255)
github_username = models.CharField(max_length=255)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it might be better to reference profile here in order to remove redundancy but I see avatar_url spread across models. What do You think? Maybe refactor in another PR ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@kziemianek Ideally, if we could reference profile and add a related name for reverse as: ideas, that would be 👍

@kziemianek
Copy link
Contributor Author

No further work planned but labeled as WIP :)
If something else needs to be done in the scope of this PR hit me up!

@kziemianek
Copy link
Contributor Author

@owocki @mbeacom @thelostone-mc few fixes in last commits:

It appears the username isn't rendering on the list or detail views:

fixed

Recommended doesn't display updated count on list or details view.

the command should update this value, it gets updated after a while, you want it updated right after hitting hearth ?

‘github username’ input — shouldnt this be just the ’github login

changed label to github login

when i submit and im not logged in

fixed

when i submit and i am logged in

unfortunetly I cannot reproduce this problem

javascript injection! bwahahhahaha!

cool! fixed :)

the ‘ideas/new’ and the ‘ideas show’ page need links back to the main ideas page i think

there is a link in navbar, should i add next?

URIs - why ideas/list and not ideas/ ?

changed to ideas/

URIs — why ideas/idea/2/show and not ideas/- ?

changed to ideas/ideas/... what is slug?

@owocki
Copy link
Contributor

owocki commented May 2, 2018

changed to ideas/ideas/... what is slug?

can we change to ideas/<pk>-<slug> ? i think the double ideas is wasteful in the URI

slug == the slugify method on this django util https://docs.djangoproject.com/en/2.0/ref/utils/

@owocki
Copy link
Contributor

owocki commented May 2, 2018

sweet looking fwd to testing now

@kziemianek
Copy link
Contributor Author

@owocki sorry the url is ideas/idea/<int:idea_id>

@kziemianek
Copy link
Contributor Author

@thelostone-mc, i made some changes according to Your comments, thanks a lot :)
here is the summary:

Nothing happens when I click on the heart icon ? (I'm logged in)

i've described it in last comment

the username doesn't turn up next to the submitted by

i've described it in last comment

Could you update the styling of the dropdown (checkout the bounty/new) The CSS is already there, you just need to update the html

You mean these dropdowns with search built-in ?

Also the page is using a mix of Futura & Muli. Could we stick to Muli ?

Changed it to Muli

Rather than defining the font-size , to ensure uniformity could we stick to using the classes defined

does this meet your expectations ?
changed_fonts

Could we add some breathing space between disqus component and ideas (margin top/bottom of 1em ? )

definetly yes!

Is it just me / does it look odd, when the Has & Needs section are empty. Would it make sense to leave the default entry as -

added - for empty has and needs

@thelostone-mc
Copy link
Contributor

You mean these dropdowns with search built-in ?

Yup! We'll throw in the dropdown without search later and update this then

@kziemianek looks better ^_^

@kziemianek
Copy link
Contributor Author

@thelostone-mc I also updated select to use jquery select2 with hidden search box.

@kziemianek
Copy link
Contributor Author

@mbeacom are we waiting for something? have you seen my last comments :) ?

Copy link
Contributor

@mbeacom mbeacom left a comment

Choose a reason for hiding this comment

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

Generally, lgtm. Deploying to staging.

designers_exists = bool(request.POST.get('designers_exists'))
customer_exists = bool(request.POST.get('customer_exists'))
profile = request.user.profile if request.user.is_authenticated else None
idea = Idea(full_name=full_name, email=email, github_username=github_username, summary=summary, more_info=more_info,
Copy link
Contributor

Choose a reason for hiding this comment

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

v2 we should probably move this to a form.

@mbeacom
Copy link
Contributor

mbeacom commented May 17, 2018

Seeing the same ideas after clicking Show More

screenshot 2018-05-17 09 04 40

@mbeacom
Copy link
Contributor

mbeacom commented May 17, 2018

Also, I recommended the cooking discussion (of course! 😆) and it appears we'll need to add custom caching rules to the list page.

@kziemianek
Copy link
Contributor Author

@mbeacom i bet duplicated idea was dropped to next page after another idea becoming more trending, so it was returned by paginator again.

As a workaround, I can add check to ensure only one idea with given id is displayed in list.


function loadMoreIdeas() {
fetchIdeas(++pageIdx, pageSize, sorting(), (result) => {
console.log('test');

Choose a reason for hiding this comment

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

Unexpected console statement. (no-console)

designers_exists = bool(request.POST.get('designers_exists'))
customer_exists = bool(request.POST.get('customer_exists'))
profile = request.user.profile if request.user.is_authenticated else None
idea = Idea(full_name=full_name, email=email, github_username=github_username, summary=summary, more_info=more_info,

Choose a reason for hiding this comment

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

E501 line too long (124 > 120 characters)

@ghost ghost assigned mbeacom May 31, 2018
@ghost ghost added the in progress label May 31, 2018
from django.core.paginator import EmptyPage, PageNotAnInteger, Paginator
from django.core.validators import validate_email
from django.http import HttpResponse, JsonResponse
from django.http import HttpResponse, HttpResponseRedirect, JsonResponse

Choose a reason for hiding this comment

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

F401 'django.http.HttpResponse' imported but unused

@kziemianek kziemianek closed this Aug 17, 2018
@kziemianek kziemianek deleted the pitch branch August 17, 2018 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend This needs backend expertise. frontend This needs frontend expertise.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants