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

feat: TPW item upsert support #376

Merged
merged 9 commits into from
Jan 6, 2022
Merged

Conversation

LautaroPetaccio
Copy link
Contributor

This PR adds TPW item upsert support.

@@ -19,6 +19,7 @@ export type ItemAttributes = {
id: string // uuid
/**
* The urn_suffix field holds the item part of the URN in third party items.
* TODO: what about this? The urn_suffix will remain null until the user sets it.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we allow third party items to be created with a null urn_suffix?
These are the two alternatives:

  • We could consider that any TWP item upsert MUST have an urn where we will get the suffix from. This implies that the form to create the new items must contain a new field to set the URN.
  • We could allow any TPW item upsert to have its URN as null, and let the user set it later.

Copy link
Contributor

Choose a reason for hiding this comment

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

We talked about this a bit, but I think I'd go with option 1. Items MUST have an urn_suffix. Maybe we could just pre-fill it with the uuid to avoid having to add a new input to the Modal in the front-end

@coveralls
Copy link

coveralls commented Dec 29, 2021

Pull Request Test Coverage Report for Build 1663689801

  • 93 of 96 (96.88%) changed or added relevant lines in 11 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.2%) to 66.058%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/Item/utils.ts 6 7 85.71%
spec/utils.ts 9 11 81.82%
Totals Coverage Status
Change from base Build 1655400458: 1.2%
Covered Lines: 1913
Relevant Lines: 2765

💛 - Coveralls

Copy link
Contributor

@nicosantangelo nicosantangelo left a comment

Choose a reason for hiding this comment

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

Great work

const hasURN =
itemAttributes.urn_suffix &&
dbCollection &&
dbCollection.urn_suffix &&
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use isTPCollection here. Is not necessary, but with the change on the return type here you can remove the ! below later

Comment on lines 135 to 138
private buildItemWithDates(
dbItem: ItemAttributes | undefined,
item: FullItem
): FullItem {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you think it's a bit weird to receive both types of items here? It's not clear why one it's being used for the spread and the other for the created_at itself.

Given that the method is just being used in one place, what do you think of simplifying it a bit. For example:

updateModelDates(createdAt?: Date) {
  const currentDate = new Date()
  return {
      created_at: created_at || currentDate,
      updated_at: currentDate,
  }
}

item = { ...item, ...this.updateModelDates(dbItem?.created_at) }

with this implementation (just one of many) the method could be useful for other Models aswell

dbCollection: CollectionAttributes,
eth_address: string
): Promise<FullItem> {
const dbCollectionURN = getThirdPartyCollectionURN(
Copy link
Contributor

Choose a reason for hiding this comment

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

After we merge this we could add a check for isTPCollection here and avoid having to use ! everywhere later on

} {
const matches = tpwItemURNRegex.exec(itemURN)
if (matches === null) {
throw new Error('The given item URN is not TWP compliant')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
throw new Error('The given item URN is not TWP compliant')
throw new Error('The given item URN is not TPW compliant')

(or just TP)

Comment on lines 55 to 56
dbCollection.urn_suffix &&
dbCollection.third_party_id
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be using isTPCollection right?

Comment on lines 312 to 314
// if (item.urn === null) {
// throw new InvalidItemURNError()
// }
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 left overs? same on line 334

@LautaroPetaccio LautaroPetaccio merged commit 425bdec into master Jan 6, 2022
@LautaroPetaccio LautaroPetaccio deleted the feat/tpw-item-upsert-support branch January 6, 2022 15:45
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.

None yet

3 participants