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

MARXAN-545 MARXAN-644 specification repository #423

Merged
merged 6 commits into from
Aug 11, 2021

Conversation

kgajowy
Copy link
Contributor

@kgajowy kgajowy commented Aug 9, 2021

Substitute this line for a meaningful title for your changes

Overview

Please write a description. If the PR is hard to understand, provide a quick explanation of the code.

Designs

Link to the related design prototypes (if applicable)

Testing instructions

Please explain how to test the PR: ID of a dataset, steps to reach the feature, etc.

Feature relevant tickets

Link to the related task manager tickets


Checklist before submitting

  • Meaningful commits and code rebased on develop.
  • If this PR adds feature that should be tested for regressions when
    deploying to staging/production, please add brief testing instructions
    to the deploy checklist (docs/deployment-checklist.md)
  • Update CHANGELOG file

@vercel
Copy link

vercel bot commented Aug 9, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

marxan – ./app

🔍 Inspect: https://vercel.com/vizzuality1/marxan/7T4EJvfNAxEPyHDx7NnGRUoMZsx2
✅ Preview: https://marxan-git-feat-marxan-545-marxan-644-specif-35051c-vizzuality1.vercel.app

marxan-storybook – ./app

🔍 Inspect: https://vercel.com/vizzuality1/marxan-storybook/EfT9hatCLnqaqW2wAwG2vKj9Ag8W
✅ Preview: https://marxan-storybook-git-feat-marxan-545-marxan-5c29b2-vizzuality1.vercel.app

Base automatically changed from feat/MARXAN-545-MARXAN-657-find-specs-with-given-geofeatures to feat/MARXAN-545-application-pieces August 9, 2021 11:29
@kgajowy kgajowy force-pushed the feat/MARXAN-545-application-pieces branch from 9f5166b to c40aebc Compare August 9, 2021 11:42
Base automatically changed from feat/MARXAN-545-application-pieces to develop August 9, 2021 11:53
@kgajowy kgajowy force-pushed the feat/MARXAN-545-MARXAN-644-specificaiton-repo branch from be34c05 to 899cc73 Compare August 9, 2021 11:55
@kgajowy kgajowy force-pushed the feat/MARXAN-545-MARXAN-644-specificaiton-repo branch from 899cc73 to 3d1e1eb Compare August 9, 2021 11:57
@kgajowy kgajowy force-pushed the feat/MARXAN-545-MARXAN-644-specificaiton-repo branch from 3d1e1eb to eca05dc Compare August 10, 2021 08:07
@kgajowy kgajowy force-pushed the feat/MARXAN-545-MARXAN-644-specificaiton-repo branch from eca05dc to a2578fa Compare August 10, 2021 09:53
@kgajowy kgajowy force-pushed the feat/MARXAN-545-MARXAN-644-specificaiton-repo branch from a2578fa to 4f5b3f6 Compare August 10, 2021 09:54
@kgajowy kgajowy changed the title Feat/marxan 545 marxan 644 specificaiton repo MARXAN-545 MARXAN-644 specification repository Aug 10, 2021
@kgajowy kgajowy marked this pull request as ready for review August 10, 2021 09:59
),
}),
);
return;
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
return;

@@ -69,7 +69,7 @@ export class ApiEvent {
*/
@ApiProperty()
@IsEnum(Object.values(API_EVENT_KINDS))
@Column('enum')
@Column('enum', { enum: API_EVENT_KINDS })
Copy link
Contributor

Choose a reason for hiding this comment

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

not true, it's varchar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be that actual state isn't matched to what we have in entity declaration - still wanted a short route just o keep migration-generation to return results.

Copy link
Contributor

Choose a reason for hiding this comment

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

then it's better to update it to varchar :D

} from 'typeorm';
import { SpecificationFeatureConfigApiEntity } from './specification-feature-config.api.entity';

@Entity(`specification_feature`)
Copy link
Contributor

Choose a reason for hiding this comment

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

the kept convention is to use plurals

Copy link
Member

Choose a reason for hiding this comment

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

Since this is a full entity now, we will also need an order (or something like that) column to keep the ordering of features in a specification consistent with what API clients send (or the frontend to start with may see features jumping around depending on contingent factors such as order of insert/update in db)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hotzevzl sorting on serializer/controller level should be enough, shouldn't it?

if it is needed at all (https://vizzuality.slack.com/archives/C01G7PRQB96/p1628593316134400)

Copy link
Member

Choose a reason for hiding this comment

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

ok, so if we are echoing back (with any augmentation) the specification as received in the request payload then we don't need to keep an order of the children of specification_features as stored in db.

if we are composing the response from the data we have in db, then keeping the same order of geofeatures as provided in the request payload would be preferable for a consistent user experience - we could sort at serializer level, but without other order hints we could only order alphabetically here, unless I am misunderstanding what you suggest.

} from 'typeorm';
import { SpecificationFeatureConfigApiEntity } from './specification-feature-config.api.entity';

@Entity(`specification_feature`)
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a full entity now, we will also need an order (or something like that) column to keep the ordering of features in a specification consistent with what API clients send (or the frontend to start with may see features jumping around depending on contingent factors such as order of insert/update in db)

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