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.

Sign up for GitHub

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

Jump to bottom

IBX-3814: Added new endpoint to get field definition by identifier #99

Merged
merged 8 commits into from
Dec 14, 2022
Merged

IBX-3814: Added new endpoint to get field definition by identifier #99

merged 8 commits into from
Dec 14, 2022

Conversation

mateuszdebinski
Copy link
Contributor

@mateuszdebinski mateuszdebinski commented Dec 1, 2022 •
edited

Question Answer
JIRA issue IBX-3814
Type bug
Target version V3.3+
BC breaks no
Tests pass yes
Doc needed no

For the purpose of the multi-upload functionality in BO, an endpoint for getting field def has been added by the identifier (in admin-UI we use the identifier of the field to define which field will be used to upload the file).

TODO:

  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@mateuszdebinski mateuszdebinski self-assigned this Dec 1, 2022
@mateuszdebinski mateuszdebinski requested a review from a team December 1, 2022 14:57
@mateuszdebinski mateuszdebinski mentioned this pull request Dec 1, 2022
Closed
3 tasks
konradoboza
konradoboza requested changes Dec 1, 2022
View reviewed changes
src/lib/Server/Controller/ContentType.php Outdated Show resolved Hide resolved
src/lib/Server/Controller/ContentType.php Outdated Show resolved Hide resolved
src/lib/Server/Controller/ContentType.php Outdated Show resolved Hide resolved
src/lib/Server/Output/ValueObjectVisitor/RestFieldDefinition.php Outdated Show resolved Hide resolved
src/lib/Server/Values/RestFieldDefinition.php Outdated Show resolved Hide resolved
tests/lib/Server/Output/ValueObjectVisitor/RestFieldDefinitionTest.php Outdated Show resolved Hide resolved
tests/lib/Server/Output/ValueObjectVisitor/RestFieldDefinitionTest.php Outdated Show resolved Hide resolved
@konradoboza konradoboza requested a review from a team December 1, 2022 15:15
@mateuszdebinski mateuszdebinski requested review from konradoboza and a team December 2, 2022 09:53
@ViniTou ViniTou requested a review from a team December 2, 2022 10:08
@konradoboza konradoboza requested a review from a team December 2, 2022 10:46
Steveb-p
Steveb-p reviewed Dec 2, 2022
View reviewed changes
Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

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

I think that a functional test (integration actually) is missing.

See tests in tests/bundle/Functional. There should be at least one well-known field definition that we can query against an actual instance of Ibexa DXP and check the result.

Steveb-p
Steveb-p approved these changes Dec 2, 2022
View reviewed changes
Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

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

LGTM.

You can take a look at ibexa/rest#40 regarding how I envision we will write tests in the newer packages. I'm talking specifically about REST response snapshots.

@mateuszdebinski mateuszdebinski mentioned this pull request Dec 5, 2022
Merged
2 tasks
@sonarcloud
Copy link

sonarcloud bot commented Dec 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

adamwojs
adamwojs approved these changes Dec 9, 2022
View reviewed changes
Copy link
Member

@adamwojs adamwojs left a comment

Choose a reason for hiding this comment

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

+1 but please update REST reference as well

@bogusez bogusez self-assigned this Dec 14, 2022
This was referenced Dec 14, 2022
Closed
Closed
Closed
@bogusez
Copy link

bogusez commented Dec 14, 2022

Regression tests passed:
ibexa/experience#118
ibexa/commerce#175
ibexa/content#65

@ViniTou ViniTou merged commit 28eb4cb into 1.3 Dec 14, 2022
@ViniTou ViniTou deleted the IBX-3814_add_load_field_def_by_identifier branch December 14, 2022 13:51
@mateuszdebinski mateuszdebinski mentioned this pull request Dec 22, 2022
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamwojs adamwojs adamwojs approved these changes

@Steveb-p Steveb-p Steveb-p approved these changes

@konradoboza konradoboza konradoboza approved these changes

@bogusez bogusez bogusez approved these changes

@ViniTou ViniTou Awaiting requested review from ViniTou

Assignees

@Steveb-p Steveb-p

@alongosz alongosz

@ViniTou ViniTou

@mateuszdebinski mateuszdebinski

@bogusez bogusez

Labels
Bug QA approved Ready for MERGE
Milestone
No milestone
7 participants
@mateuszdebinski @bogusez @adamwojs @Steveb-p @ViniTou @konradoboza @alongosz