feat: parse open-payments intervals

[rico191013]

Context

• fixes Grants detail page - make it better #1325

Changes

[beniaminmunteanu]

How do you feel about an implementation of this method that does not mutate the grants list?
(by using .map)
(This would also imply changes on the usage of the function)
(did not test it out)

Suggested change

	private parseIntervals(grants: Grant[]) {
	for (const grant of grants)
	for (const accessElement of grant.access)
	if (accessElement.limits?.interval) {
	const time = accessElement.limits?.interval.split('/')
	const duration = moment.duration(time[2])
	const date = moment(time[1])
	const months = duration.asMonths()
	if (months === 1)
	accessElement.limits!.interval = `every month from ${date.format('ddd')}`
	else if (months > 1 && months < 12)
	accessElement.limits!.interval = `every ${months} month from ${date.format('ddd')}`
	else if (months === 12)
	accessElement.limits!.interval = `every year from ${date.format('ddd')}`
	else if (months > 12) {
	const years = duration.asYears()
	accessElement.limits!.interval = `every ${years} year from ${date.format('ddd')}`
	} else if (months < 1 && months > 0) {
	const days = duration.asDays()
	accessElement.limits!.interval = `every ${days} days from ${date.format('ddd')}`
	}
	}
	private parseIntervals(grants: Grant[]): Grant[] {
	return grants.map(grant => {
	const access = grant.access.map(accessElement => {
	if (accessElement.limits?.interval) {
	const time = accessElement.limits.interval.split('/')
	const duration = moment.duration(time[2])
	const date = moment(time[1])
	const months = duration.asMonths()
	let interval: string;
	if (months === 1) {
	interval = `every month from ${date.format('ddd')}`;
	} else if (months > 1 && months < 12) {
	interval = `every ${months} month from ${date.format('ddd')}`;
	} else if (months === 12) {
	interval = `every year from ${date.format('ddd')}`;
	} else if (months > 12) {
	const years = duration.asYears();
	interval = `every ${years} year from ${date.format('ddd')}`;
	} else if (months < 1 && months > 0) {
	const days = duration.asDays();
	interval = `every ${days} days from ${date.format('ddd')}`;
	}
	return {
	...accessElement,
	limits: {
	...accessElement.limits,
	interval: interval,
	},
	};
	}
	return accessElement;
	});
	return { ...grant, access };
	});
	}

[beniaminmunteanu]

If you find it decreases readability, I guess we could settle for mutating the grants in the parse method as well.
Up to you & the others

[sanducb]

@beniaminmunteanu I think that your suggested changes do not decrease readability, but the else if branches do so. I suggested a small change to those branches here, but I think that this can be somehow simplified even more.

[beniaminmunteanu]

Usage of the non-mutating function would be sth like:
(Not tested)

Suggested change

	const grants = await this.rafikiAuthService.listGrantsWithPagination(args)
	this.parseIntervals(grants.grants.edges.map((it) => it.node))
	return grants
	const grants = await this.rafikiAuthService.listGrantsWithPagination(args)
	const parsedGrants = this.parseIntervals(grants.grants.edges.map((it) => it.node))
	return {
	...grants,
	grants: {
	...grants.grants,
	edges: grants.grants.edges.map((edge, index) => ({ ...edge, node: parsedGrants[index] }))
	}
	}

[rico191013]

It would be harder to read than the current one.
and it would have more time complexity because we have one more for loop here

[beniaminmunteanu]

anything that does not mutate data will implicitly have 1 extra loop

[beniaminmunteanu]

doesn't deploy pipeline need to adapt to this as well?
Also i'd prefer keeping such upgrades outside of feature PR's like this one

[sanducb]

I also agree that we should keep these updates outside of feature PRs unless it's necessary.

[rico191013]

If I didn't make this change, CI would encounter an error that the pnpm version should be updated.

[beniaminmunteanu]

ok i saw this

Should still be a separate PR for that merged b4 this one, but ok

[sanducb]

I also agree that we should keep these updates outside of feature PRs unless it's necessary.

[sanducb]

These two branches look like they can be merged into one, WDYT? @rico191013

[rico191013]

their messages are different!

[beniaminmunteanu]

yes, the choice is between having 2 branches or having string interpolation in just one.
I'm fine with both

[sanducb]

@beniaminmunteanu I think that your suggested changes do not decrease readability, but the else if branches do so. I suggested a small change to those branches here, but I think that this can be somehow simplified even more.

[dragosp1011]

why the mocks are changed but there are no tests to check the interval transformation?

[rico191013]

It covers in the list pagination test