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

Use sold message for on loan works too #2117

Merged
merged 1 commit into from
Jan 31, 2018
Merged

Use sold message for on loan works too #2117

merged 1 commit into from
Jan 31, 2018

Conversation

jonallured
Copy link
Member

Hi force friends, it's been such a long time!!! 👋

Per @madeleineb on #AS-7, use the sold message for on-loan works too.

Per @madeleineb on #AS-7, use the sold message for on-loan works too.
@jonallured
Copy link
Member Author

@mzikherman The code changed here appears to be dead, what would you think about me morphing this PR into one that prunes the desktop/components/contact/default_message.coffee file entirely?

@mzikherman
Copy link
Contributor

That's cool, but a quick search in Force does show this being referenced in a few places, are we sure it's unused?

@jonallured
Copy link
Member Author

jonallured commented Jan 30, 2018

Is anyone really sure about anything?? 😝

This is what made us think that MP is used and not this code in force:

unless artwork.partner.is_pre_qualify
if artwork.contact_message
textarea.bordered-input(
rows='4'
name='message'
required
)
= artwork.contact_message
= ' '

Did you find another place we missed??

@mzikherman
Copy link
Contributor

Yep, the artwork page is backed by Metaphysics, so that's the exact right code that you linked.

I guess I'm trying to think if there are places that one can inquire on Force besides the desktop artwork page.

One that comes to mind is the mobile artwork page. Seems like making an inquiry on a mobile artwork page takes you to a page like: https://www.artsy.net/inquiry/yves-clerc-n-degrees-215 (as an example), and the code seems to reference default_message, though I haven't looked too much further. Have you tested mobile artwork/inquiry pages? We should probably test that.

Besides that mobile artwork page I think you're right that we might be able to nuke the code!

(Emission uses Metaphysics so we should be good there).

@jonallured
Copy link
Member Author

Ok, after much thinking things through, @anandaroop and I have decided to merge this PR for now and follow up later on deleting code. So that left me with trying to prove that this PR does, in fact, correctly affect the default message. After some help from @damassi, I was able to get this screenshot as proof:

screen shot 2018-01-31 at 4 16 55 pm

So, I think this is ready to merge, but let me know if I missed anything!!

@anandaroop
Copy link
Member

Super, thanks for following through and thanks @damassi for the assist.

@anandaroop anandaroop merged commit 2a6c444 into artsy:master Jan 31, 2018
@jonallured jonallured deleted the on-loan-default-inquiry-message branch February 1, 2018 14:30
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.

3 participants