OBPIH-6112 Add attributes section with inputs (fixes after QA)#4535
OBPIH-6112 Add attributes section with inputs (fixes after QA)#4535awalkowiak merged 7 commits intofeature/product-supplier-list-redesignfrom
Conversation
| query += " join a.entityTypeCodes etc where etc = :entityTypeCode and active = :active" | ||
| argumentsList += [entityTypeCode: entityTypeCode, active: active] |
There was a problem hiding this comment.
I wasn't sure how I should deal with the active property separately from entityTypeCode here, because the syntax of this query forced me to append the active in the where clause
There was a problem hiding this comment.
@alannadolny one minor thing for now, if you have above select a from Attribute a, and you use it in join as join a.entityTypeCodes then please use also a here as and a.active = :active for clarity.
|
I am marking this PR as a draft, to wait for Manon's response to proposed changes from Katarzyna. |
alannadolny
force-pushed
the
OBPIH-6112-2
branch
from
e1b6720 to
1fda47f
Compare
| if (entityTypeCode) { | ||
| query += " join a.entityTypeCodes etc where etc = :entityTypeCode" | ||
| argumentsList += [entityTypeCode: entityTypeCode] | ||
| query += " join a.entityTypeCodes etc where etc = :entityTypeCode and a.active = :active" |
There was a problem hiding this comment.
by doing it like this, if you don't provide entityTypeCode, you would not include the active neither (which is supposed to be true by default).
There was a problem hiding this comment.
if we suppose to have a default value for active we could do it like (pseudocode, check if it works/adjust it)
String query = "select a from Attribute a"
String whereQuery = "where a.active = :active"
Map<String, String> argumentsList = [:]
if (entityTypeCode) {
argumentsList += [entityTypeCode: entityTypeCode]
query += " join a.entityTypeCodes etc"
whereQuery += " and etc = :entityTypeCode"
}
argumentsList.active = active
query += whereQuery
return Attribute.executeQuery(query, argumentsList)
| <div> | ||
| <TextInput | ||
| required | ||
| title={{ id: '', defaultMessage: 'Other' }} |
There was a problem hiding this comment.
should the id be empty?
| <TextInput | ||
| required | ||
| title={{ id: '', defaultMessage: 'Other' }} | ||
| errorMessage={inputValue === '' && translate( |
There was a problem hiding this comment.
what if inputValue would be null? Why don't we want to validate it then?
There was a problem hiding this comment.
If the value is null, we are treating the input as not touched previously
| <Button | ||
| disabled={!inputValue} | ||
| defaultLabel="Confirm" | ||
| label="" |
There was a problem hiding this comment.
should it be empty here and in the button above?
| if (entityTypeCode) { | ||
| query += " join a.entityTypeCodes etc where etc = :entityTypeCode" | ||
| argumentsList += [entityTypeCode: entityTypeCode] | ||
| query += " join a.entityTypeCodes etc where etc = :entityTypeCode and a.active = :active" |
There was a problem hiding this comment.
I think we can add a form with validation schema on this modal and
setValue(`attributes.${selectedAttribute?.id}`, { id: attribute?.id, value, label: value });
on form submit.
This will require a little more code but is probably the right approach
cc @kchelstowski
There was a problem hiding this comment.
Yeah, it is probably the right approach, but I think it's a little overkill to use a new hook and new validation schema to handle the one input with a text value, with validation only for the presence/absence of the entered value.
| const setAttributeValue = (attribute, value) => () => { | ||
| setValue(`attributes.${selectedAttribute?.id}`, { id: attribute?.id, value, label: value }); | ||
| setInputValue(null); | ||
| close(); | ||
| }; |
There was a problem hiding this comment.
I think this doesn't have to be a double callback
| const setAttributeValue = (attribute, value) => () => { | |
| setValue(`attributes.${selectedAttribute?.id}`, { id: attribute?.id, value, label: value }); | |
| setInputValue(null); | |
| close(); | |
| }; | |
| const setAttributeValue = () => { | |
| setValue(`attributes.${selectedAttribute?.id}`, { id: selectedAttribute?.id, value, label: inputValue }); | |
| setInputValue(null); | |
| close(); | |
| }; |
am I wrong?
alannadolny
force-pushed
the
OBPIH-6112-2
branch
from
b88d5bf to
8fcf136
Compare
* OBPIH-6112 Add validation for empty strings and add filtering for active attributes * OBPIH-6112 Add new translations * OBPIH-6112 Add new props and create variables for new modal * OBPIH-6112 Add new modal * OBPIH-6112 Handle attribute without options and allow Other * OBPIH-6112 Change query in AttributeService * OBPIH-6112 Changes after review
4 participants
No description provided.