Missing shows data. #226

Merged
merged 15 commits into from May 5, 2016

Conversation

Projects
None yet
6 participants
@alloy
Contributor

alloy commented May 3, 2016

This is for artsy/emission#11

test/schema/partner_show.js
PartnerShow.__Rewire__('gravity', gravity);
+ PartnerShow.__Rewire__('total', total);

This comment has been minimized.

@alloy

alloy May 3, 2016

Contributor

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?

@alloy

alloy May 3, 2016

Contributor

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?

This comment has been minimized.

@alloy

alloy May 4, 2016

Contributor

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

@alloy

alloy May 4, 2016

Contributor

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

This comment has been minimized.

@dzucconi

dzucconi May 4, 2016

Contributor

:Taking a look:

@dzucconi

dzucconi May 4, 2016

Contributor

:Taking a look:

This comment has been minimized.

@dzucconi

dzucconi May 4, 2016

Contributor

k peep 8e34232
and
bf3e2ea

@dzucconi

dzucconi May 4, 2016

Contributor

k peep 8e34232
and
bf3e2ea

lib/loaders/total.js
@@ -5,7 +5,7 @@ import httpLoader from './http';
export const total = ({ path, options }) => {
const key = toKey(path, assign({}, options, {
- size: 1,
+ size: 0,

This comment has been minimized.

@alloy

alloy May 4, 2016

Contributor

This change depends on artsy/gravity#9992 being deployed.

@alloy

alloy May 4, 2016

Contributor

This change depends on artsy/gravity#9992 being deployed.

@alloy alloy referenced this pull request in artsy/emission May 4, 2016

Closed

[metaphysics] Missing shows data #11

3 of 4 tasks complete
@@ -64,6 +65,11 @@ const PartnerShowType = new GraphQLObjectType({
press_release: markdown(),
start_at: date,
end_at: date,
+ exhibition_period: {

This comment has been minimized.

@alloy

alloy May 4, 2016

Contributor

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

@alloy

alloy May 4, 2016

Contributor

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

This comment has been minimized.

@sarahscott

sarahscott May 4, 2016

Contributor

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

@sarahscott

sarahscott May 4, 2016

Contributor

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

This comment has been minimized.

@orta

orta May 4, 2016

Member

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

@orta

orta May 4, 2016

Member

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

@alloy alloy referenced this pull request May 4, 2016

Closed

Specify time zone defaults. #227

2 of 2 tasks complete
@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy May 4, 2016

Contributor

Related to the exhibition period addition, and dates in general, we should think about timezones. I filed #227 for that purpose.

Contributor

alloy commented May 4, 2016

Related to the exhibition period addition, and dates in general, we should think about timezones. I filed #227 for that purpose.

lib/date.js
+ }
+
+ return `${startMoment.format(startFormat)} - ${endMoment.format(endFormat)}`;
+}

This comment has been minimized.

@dzucconi

dzucconi May 4, 2016

Contributor

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

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

dzucconi May 4, 2016

Contributor

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

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

This comment has been minimized.

@dzucconi

dzucconi May 4, 2016

Contributor

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.

@dzucconi

dzucconi May 4, 2016

Contributor

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.

This comment has been minimized.

@alloy

alloy May 4, 2016

Contributor

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!

@alloy

alloy May 4, 2016

Contributor

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!

This comment has been minimized.

@dzucconi

dzucconi May 4, 2016

Contributor

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.

@dzucconi

dzucconi May 4, 2016

Contributor

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.

lib/date.js
+ endFormat = 'MMMM '.concat(endFormat);
+ }
+
+ return `${startMoment.format(startFormat)} - ${endMoment.format(endFormat)}`;

This comment has been minimized.

@dzucconi

dzucconi May 4, 2016

Contributor

This should be an en dash:

@dzucconi

dzucconi May 4, 2016

Contributor

This should be an en dash:

lib/loaders/total.js
@@ -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 })

This comment has been minimized.

@dzucconi

dzucconi May 4, 2016

Contributor

Does this work? It ... shouldn't

@dzucconi

dzucconi May 4, 2016

Contributor

Does this work? It ... shouldn't

This comment has been minimized.

@dzucconi

dzucconi May 4, 2016

Contributor

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

@dzucconi

dzucconi May 4, 2016

Contributor

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

This comment has been minimized.

@alloy

alloy May 4, 2016

Contributor

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.

@alloy

alloy May 4, 2016

Contributor

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.

schema/partner_show.js
+ resolve: ({ id, partner }, options) => {
+ return total(`partner/${partner.id}/show/${id}/artworks`, options);
+ },
+ },

This comment has been minimized.

@dzucconi

dzucconi May 4, 2016

Contributor

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

@dzucconi

dzucconi May 4, 2016

Contributor

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

This comment has been minimized.

@alloy

alloy May 4, 2016

Contributor

Got it, will do 👍

@alloy

alloy May 4, 2016

Contributor

Got it, will do 👍

.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');

This comment has been minimized.

@dzucconi

dzucconi May 4, 2016

Contributor

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

@dzucconi

dzucconi May 4, 2016

Contributor

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

This comment has been minimized.

@alloy

alloy May 4, 2016

Contributor

Yep, this did not work without my change above.

@alloy

alloy May 4, 2016

Contributor

Yep, this did not work without my change above.

test/lib/date.js
+ period.should.equal('January 12th - 19th');
+ });
+ });
+});

This comment has been minimized.

@dzucconi

dzucconi May 4, 2016

Contributor

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

@dzucconi

dzucconi May 4, 2016

Contributor

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

This comment has been minimized.

@alloy

alloy May 4, 2016

Contributor

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?

@alloy

alloy May 4, 2016

Contributor

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?

This comment has been minimized.

@briansw

briansw May 4, 2016

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
@briansw

briansw May 4, 2016

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

This comment has been minimized.

@orta

orta May 4, 2016

Member

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

@orta

orta May 4, 2016

Member

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

lib/loaders/total.js
const key = toKey(path, assign({}, options, {
- size: 1,
+ size: 0,

This comment has been minimized.

@@ -0,0 +1,22 @@
+import moment from 'moment';
+
+export function exhibitionPeriod(startAt, endAt) {

This comment has been minimized.

@orta

orta May 4, 2016

Member

wot? not ausstellungssauer - shocking

@orta

orta May 4, 2016

Member

wot? not ausstellungssauer - shocking

This comment has been minimized.

@dzucconi

This comment has been minimized.

Show comment
Hide comment
@dzucconi

dzucconi May 4, 2016

Contributor

By adding the errors key to the then and logging that out it would have told you that it was unable to call id on partner, which is needed to make the request.

By adding the errors key to the then and logging that out it would have told you that it was unable to call id on partner, which is needed to make the request.

This comment has been minimized.

Show comment
Hide comment
@dzucconi

dzucconi May 4, 2016

Contributor

@alloy ^^

Contributor

dzucconi replied May 4, 2016

@alloy ^^

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy May 4, 2016

Contributor

By adding the errors key to the then

What a n00b 😭

Contributor

alloy replied May 4, 2016

By adding the errors key to the then

What a n00b 😭

@dzucconi

This comment has been minimized.

Show comment
Hide comment
@dzucconi

dzucconi May 4, 2016

Contributor

You could do what you were doing with the stub here but I think it's a bit clearer to just reset it on every spec

You could do what you were doing with the stub here but I think it's a bit clearer to just reset it on every spec

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy May 4, 2016

Contributor

Oh yeah, totally 👍

Contributor

alloy replied May 4, 2016

Oh yeah, totally 👍

@dzucconi

This comment has been minimized.

Show comment
Hide comment
@dzucconi

dzucconi May 4, 2016

Contributor

So the loader accepts either a key or an array of keys, which have to be strings. toKey basically just constructs the appropriate query string, which we also use as redis cache keys. Our HTTP loader checks redis for that key before making the request

Contributor

dzucconi commented on 8e34232 May 4, 2016

So the loader accepts either a key or an array of keys, which have to be strings. toKey basically just constructs the appropriate query string, which we also use as redis cache keys. Our HTTP loader checks redis for that key before making the request

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy May 4, 2016

Contributor

Got it, thanks for fixing.

Contributor

alloy replied May 4, 2016

Got it, thanks for fixing.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy May 4, 2016

Contributor

@dzucconi Will you PR those fixes you made into this branch?

Contributor

alloy commented May 4, 2016

@dzucconi Will you PR those fixes you made into this branch?

@dzucconi

This comment has been minimized.

Show comment
Hide comment
@dzucconi

dzucconi May 4, 2016

Contributor

alloy#1 you got it

Contributor

dzucconi commented May 4, 2016

alloy#1 you got it

+ artist_id: {
+ type: GraphQLString,
+ description: 'The slug or ID of an artist in the show.',
+ },

This comment has been minimized.

@alloy

alloy May 4, 2016

Contributor

@dzucconi I was unsure how to make numeral work with this field that takes an optional argument. Is this a requirement for getting this merged or is it something that can be added when needed?

@alloy

alloy May 4, 2016

Contributor

@dzucconi I was unsure how to make numeral work with this field that takes an optional argument. Is this a requirement for getting this merged or is it something that can be added when needed?

This comment has been minimized.

@dzucconi

dzucconi May 4, 2016

Contributor

Yeah it probably needs to be edited to accept that, but it's fine—you'll just have to process it to add commas or whatever else you wanna do on your client in the meantime.

@dzucconi

dzucconi May 4, 2016

Contributor

Yeah it probably needs to be edited to accept that, but it's fine—you'll just have to process it to add commas or whatever else you wanna do on your client in the meantime.

lib/loaders/total.js
+const load = (path, options = {}) => {
+ const key = toKey(path, assign({}, options, {
+ size: 1,
+ total_count: 0,

This comment has been minimized.

@alloy

alloy May 4, 2016

Contributor

The failing test for this had me stumped for a bit ;)

@alloy

alloy May 4, 2016

Contributor

The failing test for this had me stumped for a bit ;)

This comment has been minimized.

@dzucconi

dzucconi May 4, 2016

Contributor

w 👀 ps

@dzucconi

dzucconi May 4, 2016

Contributor

w 👀 ps

test/lib/date.js
- const period = exhibitionPeriod(moment().format('YYYY-01-12'), moment().format('YYYY-01-19'))
- period.should.equal('January 12th - 19th');
+ const period = exhibitionPeriod(moment().format('YYYY-01-01'), moment().format('YYYY-01-19'))
+ period.should.equal('Jan 1 – 19');
});

This comment has been minimized.

@alloy

alloy May 5, 2016

Contributor

@briansw FYI here are the test cases.

@alloy

alloy May 5, 2016

Contributor

@briansw FYI here are the test cases.

lib/date.js
if (startMoment.year() !== endMoment.year()) {
startFormat = startFormat.concat(', YYYY');
}
- let endFormat = 'Do';
+ let endFormat = 'D';
if (endMoment.year() !== thisMoment.year()) {
endFormat = endFormat.concat(', YYYY');
}
if (!(startMoment.year() === endMoment.year() && startMoment.month() === endMoment.month())) {

This comment has been minimized.

@alloy

alloy May 5, 2016

Contributor

@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.

@alloy

alloy May 5, 2016

Contributor

@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.

This comment has been minimized.

@dzucconi

dzucconi May 5, 2016

Contributor

Works for me 👍

@dzucconi

dzucconi May 5, 2016

Contributor

Works for me 👍

- totalLoader.load(path, options)
+const load = (path, options = {}) => {
+ const key = toKey(path, assign({}, options, {
+ size: 0,

This comment has been minimized.

@dzucconi

dzucconi May 5, 2016

Contributor

Is this change deployed to gravity production? I think that's probably the only thing holding up this PR?

@dzucconi

dzucconi May 5, 2016

Contributor

Is this change deployed to gravity production? I think that's probably the only thing holding up this PR?

This comment has been minimized.

@alloy

alloy May 5, 2016

Contributor

Yep, it has been deployed.

@alloy

alloy May 5, 2016

Contributor

Yep, it has been deployed.

@alloy alloy changed the title from [WIP] Missing shows data. to Missing shows data. May 5, 2016

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy May 5, 2016

Contributor

@dzucconi Added last missing data for our shows view, should be ready for review/merge 👍

Contributor

alloy commented May 5, 2016

@dzucconi Added last missing data for our shows view, should be ready for review/merge 👍

@dzucconi

This comment has been minimized.

Show comment
Hide comment
@dzucconi

dzucconi May 5, 2016

Contributor

Looks good! Thanks for bearing with 👍 👍

Contributor

dzucconi commented May 5, 2016

Looks good! Thanks for bearing with 👍 👍

@dzucconi dzucconi merged commit 1863c65 into artsy:master May 5, 2016

1 check passed

semaphoreci The build passed on Semaphore.
Details

@alloy alloy deleted the alloy:shows branch May 6, 2016

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy May 6, 2016

Contributor

Thanks for the assistance, @dzucconi !

Contributor

alloy commented May 6, 2016

Thanks for the assistance, @dzucconi !

+ args: {
+ max_days: {
+ type: GraphQLInt,
+ description: 'Before this many days no update will be generated',

This comment has been minimized.

@orta

orta May 6, 2016

Member

"Before this many days no update will be generated" doesn't make too much sense, perhaps "the maximum amount of days to show a relative day string, instead of the full time range"?

@orta

orta May 6, 2016

Member

"Before this many days no update will be generated" doesn't make too much sense, perhaps "the maximum amount of days to show a relative day string, instead of the full time range"?

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