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

Missing shows data. #226

Merged
merged 15 commits into from May 5, 2016
22 changes: 22 additions & 0 deletions lib/date.js
@@ -0,0 +1,22 @@
import moment from 'moment';

export function exhibitionPeriod(startAt, endAt) {
Copy link
Contributor

Choose a reason for hiding this comment

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

wot? not ausstellungssauer - shocking

Copy link
Contributor

Choose a reason for hiding this comment

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

const startMoment = moment(startAt);
const endMoment = moment(endAt);
const thisMoment = moment();

let startFormat = 'MMMM Do';
if (startMoment.year() !== endMoment.year()) {
startFormat = startFormat.concat(', YYYY');
}

let endFormat = 'Do';
if (endMoment.year() !== thisMoment.year()) {
endFormat = endFormat.concat(', YYYY');
}
if (!(startMoment.year() === endMoment.year() && startMoment.month() === endMoment.month())) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dzucconi I didn’t end up using those moment.js methods. I had hoped it could have simplified this conditional, but it really didn’t do much, so instead I opted for plain operators, which everyone should be able to understand.

Copy link
Member

Choose a reason for hiding this comment

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

Works for me 👍

endFormat = 'MMMM '.concat(endFormat);
}

return `${startMoment.format(startFormat)} - ${endMoment.format(endFormat)}`;
Copy link
Member

Choose a reason for hiding this comment

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

This should be an en dash:

}
Copy link
Member

Choose a reason for hiding this comment

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

Moment has some methods that might make some of this logic easier to read:

if (start.isSame(end, 'year')) { ... }

Copy link
Member

Choose a reason for hiding this comment

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

In general I've been shying away from including predetermined date formatting in the schema in favor of exposing a moment formatting interface directly. Though I can see this can't really be handled that way. I'd like to avoid the nonsense we have in Force though where there are many subtly different date string formatting methods on models.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general I've been shying away from including predetermined date formatting in the schema

Yeah, I do get that, but we just keep re-implementing these things. So I’m thinking that maybe it’s time for that shift in moving view models into the ☁️ ?

I'd like to avoid the nonsense we have in Force though where there are many subtly different date string formatting methods on models.

I’m not familiar with the Force code-base, but did have a hard time figuring out all the date methods when doing some cursory reading for this work.

I agree that I’d also like to avoid that, so don’t hold back when reviewing!

Copy link
Member

Choose a reason for hiding this comment

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

Yah; I'm with it after thinking about it. I think historically this has been a problem because of a lack of consistency site-wide when designs have been specified but there's now a move to actually consolidate. See the image of the spec posted below. It'll be nice to not have to worry about it for sure.

6 changes: 3 additions & 3 deletions lib/loaders/total.js
Expand Up @@ -3,9 +3,9 @@ import { toKey } from '../helpers';
import gravity from '../apis/gravity';
import httpLoader from './http';

export const total = (path, options = {}) => {
export const total = ({ path, options }) => {
const key = toKey(path, assign({}, options, {
size: 1,
size: 0,
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 change depends on https://github.com/artsy/gravity/pull/9992 being deployed.

Copy link
Member

Choose a reason for hiding this comment

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

total_count: 1,
}));

Expand All @@ -20,7 +20,7 @@ export const total = (path, options = {}) => {
export const totalLoader = httpLoader(total);

const load = (path, options = {}) =>
totalLoader.load(path, options)
totalLoader.load({ path, options })
Copy link
Member

Choose a reason for hiding this comment

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

Does this work? It ... shouldn't

Copy link
Member

Choose a reason for hiding this comment

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

I'm confused as to why I was passing this options in the first place: I believe this only accepts a string key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I found was an example in the DataLoader docs here https://github.com/facebook/dataloader#using-with-graphql, which passes an array:

queryLoader.load([
  'SELECT toID FROM friends WHERE fromID=? LIMIT ?', user.id, first
])

I figured I’d just give passing an arbitrary single argument a try and its worked. However, now I’m thinking that maybe it’s silently breaking anything? Like caching or something? I’m not familiar atm with what DataLoader does under the hood.

.then(response => response.total);

export default load;
19 changes: 19 additions & 0 deletions schema/partner_show.js
Expand Up @@ -4,6 +4,8 @@ import {
} from '../lib/helpers';
import { find } from 'lodash';
import gravity from '../lib/loaders/gravity';
import total from '../lib/loaders/total';
import { exhibitionPeriod } from '../lib/date';
import cached from './fields/cached';
import date from './fields/date';
import markdown from './fields/markdown';
Expand Down Expand Up @@ -63,6 +65,11 @@ const PartnerShowType = new GraphQLObjectType({
press_release: markdown(),
start_at: date,
end_at: date,
exhibition_period: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Eigen we called this ausstellungsdauer, but I’ve noticed that that was not something that was shared across clients, e.g. Force calls it ‘running dates’.

I went with the translation of ‘ausstellungsdauer’, but I’m open to a naming contest.

/cc @orta

Copy link
Contributor

Choose a reason for hiding this comment

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

'Exhibition period' makes more sense than 'running dates' IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

hah, just found this discussion - yeah, I think realistically exhibition period is a much better name.

type: GraphQLString,
description: 'A formatted description of the start to end dates',
resolve: ({ start_at, end_at }) => exhibitionPeriod(start_at, end_at),
},
artists: {
type: new GraphQLList(Artist.type),
resolve: ({ artists }) => artists,
Expand Down Expand Up @@ -131,6 +138,18 @@ const PartnerShowType = new GraphQLObjectType({
.then(exclude(options.exclude, 'id'));
},
},
artworks_count: {
type: GraphQLInt,
args: {
artist_id: {
type: GraphQLString,
description: 'The slug or ID of an artist in the show.',
},
},
resolve: ({ id, partner }, options) => {
return total(`partner/${partner.id}/show/${id}/artworks`, options);
},
},
Copy link
Member

Choose a reason for hiding this comment

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

It makes more sense to me to expose all the counts on a schema as it's own nested object—like:

counts: {
type: new GraphQLObjectType({
name: 'ArtistCounts',
fields: {
artworks: numeral(({ published_artworks_count }) =>
published_artworks_count),
follows: numeral(({ follow_count }) =>
follow_count),
auction_lots: numeral(({ auction_lots_count }) =>
auction_lots_count),
for_sale_artworks: numeral(({ forsale_artworks_count }) =>
forsale_artworks_count),
},
}),
resolve: (artist) => artist,

Also note the numeral field which exposes formatting options to the client and returns a String or Integer depending on whether or not you've specified a format

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, will do 👍

meta_image: {
type: Image.type,
resolve: ({ id, partner, image_versions, image_url }) => {
Expand Down
31 changes: 31 additions & 0 deletions test/lib/date.js
@@ -0,0 +1,31 @@
import moment from 'moment';
import { exhibitionPeriod } from '../../lib/date';

describe('date', () => {
describe('exhibitionPeriod', () => {
it('includes the start and end date', () => {
const period = exhibitionPeriod(moment('2011-01-12'), moment('2014-04-19'))
period.should.equal('January 12th, 2011 - April 19th, 2014');
});

it('does not include the year of the start date if it’s the same year as the end date', () => {
const period = exhibitionPeriod(moment('2011-01-12'), moment('2011-04-19'))
period.should.equal('January 12th - April 19th, 2011');
});

it('does not include the month of the end date if it’s the same as the start date', () => {
const period = exhibitionPeriod(moment('2011-01-12'), moment('2011-01-19'))
period.should.equal('January 12th - 19th, 2011');
});

it('does not include the year of the end date if it’s in the current year', () => {
const period = exhibitionPeriod(moment('2011-01-12'), moment().format('YYYY-04-19'))
period.should.equal('January 12th, 2011 - April 19th');
});

it('does not include a year at all if both start and end date are in the current year', () => {
const period = exhibitionPeriod(moment().format('YYYY-01-12'), moment().format('YYYY-01-19'))
period.should.equal('January 12th - 19th');
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@briansw FYI here are the test cases.

});
});
Copy link
Member

Choose a reason for hiding this comment

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

I spoke with @briansw about where we want to land on unifying the date range formatting:

screen shot 2016-05-04 at 10 42 56 am

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so:

  • Use endash
  • Get rid of ordinal suffix
  • Use month short-hand

I’m unsure what the ‘2+ mos’ part is trying to convey, can you clarify?

Copy link

@briansw briansw May 4, 2016

Choose a reason for hiding this comment

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

Just that when the show spans two or more months, we need to add the second month, but don't display it when the duration is contained to one month. Here's the description in list format:

  • Space endash space between dates
  • Get rid of ordinal indicators
  • Use month shorthand
  • No periods in month shorthand
  • Don't display the month twice if duration is contained to one month (Aug 5 – 21)
  • Only display the year if it's not the current year
  • Don't display the 0 in single digit days, ie, 08 should be 8

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 unsure what the ‘2+ mos’ part is trying to convey, can you clarify?

When it the date range goes outside of one single month, example

4 changes: 2 additions & 2 deletions test/lib/loaders/total.js
Expand Up @@ -14,10 +14,10 @@ describe('total', () => {

total.__Rewire__('gravity', gravity);

return total('foo/bar')
return total('foo/bar', { extra_option: 1 })
.then(n => {
gravity.args[0][0]
.should.equal('foo/bar?size=1&total_count=1');
.should.equal('foo/bar?extra_option=1&size=0&total_count=1');
Copy link
Member

Choose a reason for hiding this comment

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

I could have sworn this worked previously. I guess that's the change you made above that I'm confused by

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, this did not work without my change above.


n.should.equal(50);
});
Expand Down
64 changes: 64 additions & 0 deletions test/schema/partner_show.js
Expand Up @@ -7,6 +7,7 @@ describe('PartnerShow type', () => {

beforeEach(() => {
const gravity = sinon.stub();
const total = sinon.stub();

gravity.returns(Promise.resolve({
id: 'new-museum-1-2015-triennial-surround-audience',
Expand All @@ -16,11 +17,17 @@ describe('PartnerShow type', () => {
displayable: true,
}));

total
.onCall(0).returns(Promise.resolve(42))
.onCall(1).returns(Promise.resolve(2));

PartnerShow.__Rewire__('gravity', gravity);
PartnerShow.__Rewire__('total', total);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The stubbing of total works, but when I call it (in schema/partner_show.js) it doesn’t appear to return the promises I expect it to and the tests fail with null values.

Any idea what I’m doing wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dzucconi I’d really like some input on this one, currently its tests are failing.

Copy link
Member

Choose a reason for hiding this comment

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

:Taking a look:

Copy link
Member

Choose a reason for hiding this comment

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

k peep 8e34232
and
bf3e2ea

});

afterEach(() => {
PartnerShow.__ResetDependency__('gravity');
PartnerShow.__ResetDependency__('total');
});

it('includes a formattable start and end date', () => {
Expand Down Expand Up @@ -49,6 +56,25 @@ describe('PartnerShow type', () => {
});
});

it('includes a formatted exhibition period', () => {
const query = `
{
partner_show(id: "new-museum-1-2015-triennial-surround-audience") {
exhibition_period
}
}
`;

return graphql(schema, query)
.then(({ data }) => {
data.should.eql({
partner_show: {
exhibition_period: 'February 25th - May 24th, 2015'
},
});
});
});

it('includes the html version of markdown', () => {
const query = `
{
Expand All @@ -70,4 +96,42 @@ describe('PartnerShow type', () => {
});
});
});

it('includes the total number of artworks', () => {
const query = `
{
partner_show(id: "new-museum-1-2015-triennial-surround-audience") {
artworks_count
}
}
`;

return graphql(schema, query)
.then(({ data }) => {
data.should.eql({
partner_show: {
artworks_count: 42,
},
});
});
});

it('includes the number of artworks by a specific artist', () => {
const query = `
{
partner_show(id: "new-museum-1-2015-triennial-surround-audience") {
artworks_count(artist_id: "juliana-huxtable")
}
}
`;

return graphql(schema, query)
.then(({ data }) => {
data.should.eql({
partner_show: {
artworks_count: 2,
},
});
});
});
});