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

Marketplace (Commission Search) #180

Merged
merged 51 commits into from Dec 30, 2016

Conversation

taedixon
Copy link
Contributor

@taedixon taedixon commented Oct 28, 2016

This set of changes aims to add a commission search feature to Weasyl. This will allow users to find artists who list their current commission status as open, have at least one commission class defined with a price, and match certain search criteria. The new search interface can be found at /marketplace.

Searchers can specify a "type" of commission, which will limit results to those with a commission class that has a name at least partially matching the type provided. In order to encourage usage of standardized classes to make searching more effective for both artists and customers, a list of default commission classes is provided based on the most popular commission class types already existing. This list is also used as an option to populate the name of a commission class when it is being set up. Of course, free-form input for commission classes works on both sides of the equation as well.

This also adds a way to set "preference tags" and "will-not-draw" tags, through /control/editcommissionprices. These tags allow commission searchers to more easily find artists who specialize in types of content they wish to commission, or to filter out artists who are not willing to draw what they want. The search field relating to these tags in /marketplace is labelled "Content"

Searchers can filter results by setting minimum/maximum price values in their preferred currency. Results will be filtered based on prices set on artist's applicable commission classes after being converted to the searcher's currency. In results, values will be displayed in both the currency specified by the artist and the currency specified in the search (if they differ)

Results are ranked on the following criteria:

  1. Number of matching "Preference" tags
  2. Time of most recent submission
    The intent of this is to reward artists who are active with submitting to the site with higher rankings in commission searches, which also indicates that the commissioner is more likely to receive a response in a timely manner.

Current known issues:

  • The UI of the marketplace results page could probably use the touch of someone with a gift for good web design.

Notes:

  • this change involves a schema upgrade

@codecov-io
Copy link

codecov-io commented Oct 28, 2016

Current coverage is 58.28% (diff: 69.05%)

Merging #180 into master will increase coverage by 0.59%

@@             master       #180   diff @@
==========================================
  Files           151        153     +2   
  Lines         12503      12675   +172   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           7213       7388   +175   
+ Misses         5290       5287     -3   
  Partials          0          0          

Powered by Codecov. Last update c894227...94604d5

"content": content if content else "",
"tags": [i["t"] for i in tags_extract if 'n' not in i["s"]],
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these the only places tags_extract is used? If so, you should be able to write .fetchall() on the query and [i.title for i in tags if 'n' not in i.settings] here, which should be easier to read.

GROUP BY sub.userid
) AS example ON example.userid = p.userid

WHERE p.settings ~ '[os]..?'
Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t really remember how profile.settings works, but is this equivalent to ^[os]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it isn't actually because it turns out what I did produced false positives! I'll just uh.. use yours.

@@ -598,24 +598,15 @@ def get_path():


def text_price_amount(target):
return "%i.%s%i" % (target / 100, "" if target % 100 > 9 else "0", target % 100)
return "%.2f" % round(target / 100, 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn’t %.2f already round? Nice one, anyway!

Copy link
Contributor

Choose a reason for hiding this comment

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

I’m also pretty sure integers are passed into this function at some point, and define.py doesn’t have a from __future__ import division, so you might need to make this target / 100.0.

@charmander charmander modified the milestone: Pre-cleanup Oct 29, 2016
@@ -167,7 +167,7 @@ def is_tag_restriction_pattern_valid(text):
return False


def associate(userid, tags, submitid=None, charid=None, journalid=None):
def associate(userid, tags, submitid=None, charid=None, journalid=None, artistid=None, optoutid=None):
Copy link
Member

Choose a reason for hiding this comment

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

Could you update the docstring for associate() to include the added artistid and optoutid parameters, please?

@@ -463,6 +463,8 @@ def edit_content(userid, content):

def remove_class(userid, classid):
d.execute("DELETE FROM commishclass WHERE (classid, userid) = (%i, %i)", [d.get_int(classid), userid])
# poor man's cascade
d.execute("DELETE FROM commishprice WHERE (classid, userid) = (%i, %i)", [d.get_int(classid), userid])
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way we can get a proper foreign key relationship added to the Alembic migration script, here (for both onupdate and ondelete cascade)? Also, curious, was it a glitch that there wasn't an FK already established to begin with prior to this PR (to me, it seems like it might have been one)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its debatable whether this change belongs in this PR or not to be honest. There was an issue brought up in user acceptance that I couldn't replicate locally so i'd set this to go on testing as a bit of exploratory debugging.
Essentially, if you remove a class without deleting its prices then create a new class, the old prices will be assigned to it. This can also cause other strange behaviour in rendering the commission prices portion of the user profile. So, I'd consider it at least unintended behaviour.
While setting up prices is closely related to marketplace I think the scope of handling this change correctly as well as possibly the numerous other issues that could come up while attempting to modernize commission prices should probably justify shifting the commit into #188

Revert "Remove prices related to a commishclass when that class is removed"

This reverts commit 776e219.
Add link to European Bank of Commerce to marketplace help
Copy link
Member

@kfkitsune kfkitsune left a comment

Choose a reason for hiding this comment

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

Not seeing any issues with this PR at this time. Reviewed up to commit 83bc38f. Based on results on testing, it seems to do what the tin says it should be doing.

@charmander
Copy link
Contributor

Some changes suggested in taedixon#1.

merge in charmander's marketplace design changes
</div>
$# commission description
<div class="formatted-content marketplace-desc-preview">
$if artist.get('description'):
Copy link
Contributor

Choose a reason for hiding this comment

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

Will the 'description' key ever not exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the user sets up their commission prices but does not fill out the "Additional Information" field, the value may be None. If they fill it out then erase it at a later date, it may be an empty string. But in either case, I can't see a situation where the key itself will be missing under the current setup.

@@ -17,6 +17,7 @@
<li><a href="/help/ratings">Content Ratings</a></li>
<li><a href="/help/google-drive-embed">Google Drive Embedding</a></li>
<li><a href="/help/searching">Searching on Weasyl</a></li>
<li><a href="/help/marketplace">Using the Marketplace</a></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

This file appears to use tab indentation; may as well follow that for now.


<h3>What is the Marketplace?</h3>
<div class="formatted-content">
<p>The Marketplace is Weasyl’s Commission Search tool. You can use it to find artists who are open for commissions that match your search criteria.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is “Commission Search” capitalized?

<p>Marketplace currently supports the following search criteria:</p>

<ul>
<li>Type of Commission (e.g. badge)</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

“Commission” probably shouldn’t be capitalized here to match “range and currency” below.

Copy link
Member

@hyena hyena left a comment

Choose a reason for hiding this comment

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

I love this. I want charmander to sign off first, though.

If necessary, cut the conversions. It's not really our job anyway.....

@@ -100,6 +101,9 @@ y Japanese yen
c Canadian dollar
m Mexican peso
u Australian dollar
z New Zealand dollar
Copy link
Member

Choose a reason for hiding this comment

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

UGH

$if artist['pricemin']:
$# display low end of price range in artist's currency
<span>$:{'from ' if artist['pricemax'] else ''}${SYMBOL(artist['pricesettings'])} ${PRICE(artist['pricemin'])}</span>
$if artist['pricemax'] and artist['pricemax'] != artist['pricemin']:
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for making ranges optional.


<h3>Price</h3>
<div class="formatted-content">
<p>Marketplace searches can have a minimum and/or maximum price, and you can specify your preferred currency.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Nit: and optionally maximum price.

$if artist['pricesettings'] != form.currency and artist['localmin']:
$if artist['localmax'] and artist['localmax'] != artist['localmin']:
$# display converted prices (range)
(<span>Approx. ${SYMBOL(form.currency)} ${PRICE(artist['localmin'])}</span>
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for showing this separately.

@hyena
Copy link
Member

hyena commented Dec 29, 2016

Ranges are nice. But what most people are going to want to do is provide add-ons. (e.g. 'additional characters at $5 each'). How do you see people representing that in this implementation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

6 participants