Conversation
…oller integration
|
|
|
ThinuwanW
left a comment
ThinuwanW
left a comment
There was a problem hiding this comment.
🤖 Claude Code Review
This PR adds a POST /v2/crm/industry endpoint (controller → service interface → impl → Dao), a soft-delete flag on the CrmIndustry entity, name normalization/validation helpers, message constants, and good integration + unit test coverage. The layering, logging, @Transactional, ModuleException + message-constant usage, and role hierarchy (CRM_ADMIN inherits ROLE_CRM_SALES_MANAGER) all match existing module conventions. The main concerns are a whitespace-normalization gap that lets duplicates slip past the uniqueness check, a missing Liquibase changeset for the newly mapped is_deleted column, and the fact that nothing in the codebase can read the industries this endpoint creates.
Found 22 new issue(s): 🔴 4 important, 🟡 12 suggestion(s), 🟣 6 nit(s)
| if (name == null) { | ||
| return null; | ||
| } | ||
| return ValidationConstant.MULTIPLE_WHITESPACE_PATTERN.matcher(name.trim()).replaceAll(" "); |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
normalizeIndustryName trims before collapsing, which leaves non-breaking spaces (U+00A0) at the edges intact. String.trim() only strips chars <= U+0020, so for input "\u00A0Retail" the trim is a no-op and the [\s\u00A0]+ regex then converts the leading NBSP into a normal space, persisting " Retail". The duplicate guard existsByNameIgnoreCaseAndIsDeletedFalse(" Retail") will not match the existing "Retail" row, so a duplicate industry is created — exactly what this PR is meant to prevent. Worse, validateIndustryName uses name.isBlank(), and Character.isWhitespace('\u00A0') is false, so a name consisting only of NBSP passes the "required" check and is stored as a single-space industry name. Fix by collapsing first and trimming after, and by checking blankness on the normalized value: e.g. String normalized = MULTIPLE_WHITESPACE_PATTERN.matcher(name).replaceAll(" ").trim(); in normalizeIndustryName, and in validateIndustryName compute the normalized value once, then throw CRM_ERROR_INDUSTRY_NAME_REQUIRED if it is empty and CRM_ERROR_INDUSTRY_NAME_TOO_LONG if it exceeds the max.
| @Column(name = "name", nullable = false) | ||
| private String name; | ||
|
|
||
| @Column(name = "is_deleted", nullable = false) |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
This PR maps a new is_deleted column on CrmIndustry but ships no Liquibase changeset. The only place the column is defined is common/db/changelog/ddl-script/common-ddl-script-v1-create-tables.sql, which is an already-executed changeset using CREATE TABLE IF NOT EXISTS — it will not add the column to any database where crm_industry already exists, and editing an executed changeset also invalidates its Liquibase checksum (the enterprise multi-tenant path runs this changelog per tenant). The repo convention for this is a dedicated alter script, e.g. people-ddl-script-v1-alter-table-com-work-location-add-is-deleted.sql registered in db.changelog.yml. Please confirm the column predates every deployment; otherwise add a crm-ddl-script-v1-alter-table-crm-industry-add-is-deleted.sql changeset. Separately, note that crm_industry.is_deleted is declared boolean NOT NULL with no DEFAULT FALSE, unlike crm_company and crm_contact — any insert path that omits the column (raw SQL/DML scripts) will fail in strict mode.
| CrmValidations.validateIndustryName(requestDto.getName()); | ||
| String normalizedName = CrmValidations.normalizeIndustryName(requestDto.getName()); | ||
|
|
||
| if (checkIndustryExists(normalizedName)) { |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
The check-then-insert is not atomic and crm_industry.name has no unique index, so two concurrent POSTs with the same name both pass checkIndustryExists and both insert, producing exactly the duplicates this feature exists to prevent. Since this is new organisation-wide reference data, consider adding a unique index (MySQL needs a prefix length or a varchar column, as name is currently text) plus a catch (DataIntegrityViolationException) around the save that rethrows new ModuleException(CrmMessageConstant.CRM_ERROR_INDUSTRY_EXISTS), so the response stays a 400 rather than a 500.
| description = "Creates an organisation-wide industry, rejecting a duplicate name the same way company " | ||
| + "creation does.") | ||
| @PostMapping | ||
| @PreAuthorize("hasAnyRole('ROLE_CRM_SALES_MANAGER')") |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
Industries are organisation-wide master data, but this endpoint is open to ROLE_CRM_SALES_MANAGER (and therefore also every CRM_ADMIN via the hierarchy in AuthorityServiceImpl). The comparable master-data mutations in this module — CrmDealStageController create/update/delete — all require ROLE_CRM_ADMIN. Unless sales managers are deliberately allowed to grow the shared industry list, tighten this to hasAnyRole('ROLE_CRM_ADMIN') and update the integration test that currently asserts a sales manager gets 201.
| @@ -0,0 +1,35 @@ | |||
| package com.skapp.community.crmplanner.controller.v2; | |||
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
📁 File-level observation
Nothing in the codebase can read the rows this endpoint creates. There is no GET/list endpoint for crm_industry, and CrmCompany still persists its industry as the com.skapp.community.crmplanner.type.CrmIndustry enum (@Column(name = "industry")) — the crm_company.industry_id FK that exists in the DDL is not mapped on the entity. So a newly created industry can never be selected on a company, and the only consumer of CrmIndustryDao remains CrmConfigServiceImpl's default seeding. If this is the first slice of a larger change, please say so in the PR description; otherwise the read endpoint and the CrmCompany.industry → industryId migration are needed for this feature to be usable. The model.CrmIndustry / type.CrmIndustry name clash makes this duality especially easy to get wrong — consider renaming the enum (e.g. CrmIndustryType) while the two still coexist.
| } | ||
|
|
||
| CrmIndustry industry = new CrmIndustry(); | ||
| industry.setName(normalizedName); |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
Name format is inconsistent with the seeded data. DefaultCrmIndustryTemplate populates crm_industry from CrmIndustryName.getDisplayName(), which yields enum-style strings like RETAIL and TECHNOLOGY_INFORMATION_AND_MEDIA, whereas this endpoint stores free-form display text like Renewable Energy. Once both exist, any list rendered from this table shows a mix of RETAIL and Renewable Energy. Decide on one representation — either title-case the seeded defaults (via a data migration) or normalise user input to the same convention on write.
| } | ||
| } | ||
|
|
||
| public static String normalizeIndustryName(String name) { |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
The whitespace-collapse logic here duplicates com.skapp.community.common.util.StringUtils.normalizeName (same ValidationConstant.MULTIPLE_WHITESPACE_PATTERN, same replaceAll(" ")), differing only in that it preserves case and diacritics. Rather than a CRM-local copy, extract the shared step into StringUtils (e.g. collapseWhitespace(String)), have normalizeName call it, and have normalizeIndustryName delegate to it — that also keeps the trim/strip semantics fixed in one place. As a smaller point, returning null from a public normalize* helper is a trap for future callers that normalise before validating; returning "" for null/blank input (as StringUtils.normalizeName does) is safer.
| return ValidationConstant.MULTIPLE_WHITESPACE_PATTERN.matcher(name.trim()).replaceAll(" "); | ||
| } | ||
|
|
||
| public static void validateIndustryName(String name) { |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
validateIndustryName enforces only presence and length, while comparable user-supplied names in this module also constrain the character set — validateDealStageName applies DEAL_STAGE_NAME_REGEX, validateContactName applies CONTACT_NAME_REGEX. Because industries are shared, organisation-wide reference data surfaced in dropdowns, consider adding an INDUSTRY_NAME_REGEX to CrmConstants (letters/digits/space plus a small punctuation set, requiring at least one letter) and a matching CRM_ERROR_INDUSTRY_NAME_INVALID_CHARS message constant, so control characters, emoji and markup cannot be persisted.
| .andExpect(jsonPath(RESULTS_0_PATH + "['name']").value("Renewable Energy")) | ||
| .andExpect(jsonPath(RESULTS_0_PATH + "['id']").isNumber()); | ||
|
|
||
| assert crmIndustryDao.existsByNameIgnoreCaseAndIsDeletedFalse("Renewable Energy"); |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
These three checks (lines 88, 109 and 135) use the bare Java assert keyword — the only occurrences in the entire test tree. Java assertions are no-ops unless the JVM is started with -ea, so they pass silently in IDE runs, and when they do fire the AssertionError carries no message explaining what was expected. Replace with the JUnit/AssertJ assertions used elsewhere in this package, e.g. assertTrue(crmIndustryDao.existsByNameIgnoreCaseAndIsDeletedFalse("Renewable Energy")), assertFalse(...), and assertThat(...).hasSize(1) / assertEquals(1, count) for the duplicate check.
| @Setter | ||
| public class CrmIndustryCreateDto { | ||
|
|
||
| private String name; |
There was a problem hiding this comment.
🤖 Claude · 🟣 Nit
No bean-validation annotations on the DTO and no @Valid on the controller's @RequestBody. This matches the CRM module, which validates in the service layer via CrmValidations + message constants, but it diverges from the rest of the codebase (leave, people, common controllers all use @Valid). If you keep service-layer validation, that's defensible — but adding @NotBlank + @Size(max = 100) here without @Valid on the parameter would be worse than either option, so decide deliberately rather than leaving the DTO bare by accident.
| @Getter | ||
| @Setter | ||
| @AllArgsConstructor | ||
| public class CrmIndustryCreateResponseDtoV2 { |
There was a problem hiding this comment.
🤖 Claude · 🟣 Nit
Two small consistency points versus the sibling DTOs in this package: CrmContactLookupResponseDtoV2 carries @NoArgsConstructor alongside @AllArgsConstructor (useful for deserialization in tests/clients), and entity→DTO conversion for v2 responses goes through CrmMapperV2. Since id and name match by name, a one-line CrmIndustryCreateResponseDtoV2 crmIndustryToCrmIndustryCreateResponseDtoV2(CrmIndustry industry) on CrmMapperV2 would replace the hand-rolled constructor call in the service.
|
|
||
| CrmIndustry industry = new CrmIndustry(); | ||
| industry.setName(normalizedName); | ||
| crmIndustryDao.save(industry); |
There was a problem hiding this comment.
🤖 Claude · 🟣 Nit
The return value of crmIndustryDao.save(industry) is discarded and the response is built from the pre-save instance. It happens to work because GenerationType.IDENTITY populates the id on the passed entity, but createCompany in CrmCompanyServiceImpl assigns the result (CrmCompany result = crmCompanyDao.save(newCompany)) and builds the response from it. Do the same here so the response always reflects the persisted state.
| .andExpect(jsonPath(RESULTS_0_PATH + "['name']").value(maxLengthName)); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
🤖 Claude · 🟣 Nit
Two small coverage gaps at the controller level: there is no test that an unauthenticated request returns 401, and no test for a body that omits name entirely ({}) producing CRM_ERROR_INDUSTRY_NAME_REQUIRED. The null-name path is covered in CrmValidationsTest, but the end-to-end mapping from a missing JSON field to a 400 with the right message key is not.
|
|
||
| @Override | ||
| @Transactional | ||
| public ResponseEntityDto createIndustry(CrmIndustryCreateDto requestDto) { |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
This service breaks the module's v2 write pattern and removes the enterprise extension seam. Every other CRM v2 write delegates the domain work to the v1 service (CrmContactServiceImplV2.createContact -> crmContactService.persistNewContact, CrmDealServiceImplV2.createDeal -> crmDealService.persistNewDeal). That matters because the v1 impls (CrmCompanyServiceImpl, CrmContactServiceImpl, CrmDealServiceImpl, CrmDealStageServiceImpl, CrmTaskServiceImpl) each expose a protected validateXCreationLimit() placeholder that the enterprise @Primary subclasses (EpCrmCompanyServiceImpl, EpCrmDealStageServiceImpl, ...) override to call EpCrmLimitValidator.validateCreation(...) and, in the deal-stage case, TenantValidator tier filtering. CrmIndustryServiceImplV2 owns all the logic itself, talks to the DAO directly, has no v1 CrmIndustryService interface/impl and no protected hook — so there is no way for enterprise to bound or tenant-scope industry creation. Combined with the absence of any delete path, an authenticated CRM sales manager can insert unbounded rows into crm_industry (a text-typed, unindexed name column that the duplicate check full-scans on every POST). Fix: add a v1 CrmIndustryService/CrmIndustryServiceImpl holding persistNewIndustry(...) plus a protected void validateIndustryCreationLimit() {} placeholder, have CrmIndustryServiceImplV2 delegate to it, and add the matching enterprise override + count method on EpCrmLimitValidator.
| log.info("createIndustry: execution started"); | ||
|
|
||
| CrmValidations.validateIndustryName(requestDto.getName()); | ||
| String normalizedName = CrmValidations.normalizeIndustryName(requestDto.getName()); |
There was a problem hiding this comment.
🤖 Claude · 🔴 Important
Validation is run against the raw request value but the normalized value is what gets persisted, and the normalized value is never re-checked — so a name that is non-empty before normalization but blank after it slips through. Concretely, POST {"name":"\u00a0"}: String.isBlank() returns false because Character.isWhitespace('\u00A0') is false for the non-breaking space, so the required-check passes; normalizeIndustryName then matches it via MULTIPLE_WHITESPACE_PATTERN ([\s\u00A0]+) and replaces it with a single space, giving " ", which is 1 char so the length check passes; the row is persisted with the name " ". The same ordering also means the regex normalization runs twice per request (once inside validateIndustryName for the length check, once here) and lets the two normalizations drift. Fix: normalize once first, then validate the normalized result — String normalizedName = CrmValidations.normalizeIndustryName(requestDto.getName()); CrmValidations.validateIndustryName(normalizedName); — and have validateIndustryName reject a blank normalized value with CRM_ERROR_INDUSTRY_NAME_REQUIRED.
| crmIndustryDao.save(industry); | ||
|
|
||
| log.info("createIndustry: execution ended"); | ||
| return new ResponseEntityDto(false, new CrmIndustryCreateResponseDtoV2(industry.getId(), industry.getName())); |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
The response DTO is built by hand while every other v2 response in this module goes through the MapStruct CrmMapperV2 (crmContactToCrmContactResponseDtoV2, crmDealToCrmDealResponseDtoV2, crmTaskToCrmTaskResponseDtoV2). The review guidelines explicitly flag manual field-by-field mapping where MapStruct is the established mechanism. Add CrmIndustryCreateResponseDtoV2 crmIndustryToCrmIndustryCreateResponseDtoV2(CrmIndustry industry); to CrmMapperV2, inject the mapper here, and map the entity returned by save(...). That also removes the need for the @AllArgsConstructor-only DTO shape.
|
|
||
| @Getter | ||
| @Setter | ||
| public class CrmIndustryCreateDto { |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
Naming is inconsistent with the request/response pair it belongs to and with the module's conventions. The response is CrmIndustryCreateResponseDtoV2 (v2 package, V2 suffix) while the request is CrmIndustryCreateDto — no RequestDto suffix and no V2 marker, even though it is only reachable from a v2-only endpoint. Sibling create payloads in the same package use *CreateRequestDto (CrmContactCreateRequestDto, CrmDealCreateRequestDto, CrmDealStageCreateRequestDto) and v2-specific request payloads carry the suffix (CrmTaskFilterDtoV2, CrmTaskRelatedFilterDtoV2). Rename to CrmIndustryCreateRequestDtoV2 so the pair reads consistently and the v2 scope is obvious.
| @Column(name = "name", nullable = false) | ||
| private String name; | ||
|
|
||
| @Column(name = "is_deleted", nullable = false) |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
The new isDeleted flag is write-once dead state: nothing in the codebase ever sets it to true — there is no delete endpoint, no service method, and no CrmIndustryService — so the only consumer is the IsDeletedFalse clause of the new derived query, which can never filter anything out. Two consequences worth resolving now rather than later: (1) if the flag is meant to be used, ship the delete path in the same PR so the column is not untestable dead schema; (2) once soft delete exists, this create path will happily insert a second row with a name that already exists on a soft-deleted row, since the duplicate check only looks at IsDeletedFalse and there is no revive/already-deleted handling equivalent to CrmCompany's api.error.crm.company-already-deleted. Decide the semantics (revive the soft-deleted row vs. allow a duplicate name across deleted/active) and encode it here.
| @Repository | ||
| public interface CrmIndustryDao extends JpaRepository<CrmIndustry, Long> { | ||
|
|
||
| boolean existsByNameIgnoreCaseAndIsDeletedFalse(String name); |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
The IsDeletedFalse half of this derived query has no test coverage. The integration tests only ever create rows with the default isDeleted = false, so every assertion passes identically whether or not the AndIsDeletedFalse clause is present — the one behaviour this new query adds over a plain existsByNameIgnoreCase is unverified. Add a test that persists a CrmIndustry with isDeleted = true and asserts that creating the same name succeeds (or is rejected, per whatever semantics you settle on in the entity), and assert isDeleted is false on the entity produced by a successful create.
| private final CrmIndustryServiceV2 industryService; | ||
|
|
||
| @Operation(summary = "Create an industry", | ||
| description = "Creates an organisation-wide industry, rejecting a duplicate name the same way company " |
There was a problem hiding this comment.
🤖 Claude · 🟡 Suggestion
The Swagger description claims duplicates are rejected "the same way company creation does", but the two paths are not equivalent. CrmCompanyServiceImpl.createCompany passes the raw request name straight to existsByNameIgnoreCaseAndIsDeletedFalse with no whitespace normalization, whereas this endpoint collapses and trims first — so "Real Estate" is a duplicate here but would create a second row on the company endpoint. Either apply normalizeIndustryName (or the shared StringUtils.normalizeName) consistently across the CRM name-uniqueness checks, or correct the description so it does not document behaviour the sibling endpoint does not have. API docs that assert a cross-endpoint guarantee tend to be trusted by frontend callers.
| + "creation does.") | ||
| @PostMapping | ||
| @PreAuthorize("hasAnyRole('ROLE_CRM_SALES_MANAGER')") | ||
| public ResponseEntity<ResponseEntityDto> createIndustry(@RequestBody CrmIndustryCreateDto requestDto) { |
There was a problem hiding this comment.
🤖 Claude · 🟣 Nit
A malformed or empty JSON body on this new POST surfaces as a 500 rather than a 400. @RequestBody with required=true raises HttpMessageNotReadableException (e.g. empty body, or {"name": {}} which cannot coerce to String), and GlobalExceptionHandler has no handler for it — the handler list covers BindException, ModuleException, ValidationException, EntityNotFoundException, NoHandlerFoundException, SQLException, DataAccessException, ServletException, IOException, TooManyRequestsException, MissingRequestCookieException and the catch-all Exception, so this falls through to the catch-all at HttpStatus.INTERNAL_SERVER_ERROR. The behaviour is pre-existing across POST endpoints, but this PR adds another one, so it is a good moment to add @ExceptionHandler(HttpMessageNotReadableException.class) returning 400 through the standard ErrorResponse shape.
| @@ -1,5 +1,6 @@ | |||
| package com.skapp.community.crmplanner.util; | |||
|
|
|||
| import com.skapp.community.common.constant.ValidationConstant; | |||
There was a problem hiding this comment.
🤖 Claude · 🟣 Nit
📁 File-level observation
This class now sits on both sides of a type-name collision that this PR makes materially more confusing. com.skapp.community.crmplanner.type.CrmIndustry (the enum still persisted by CrmCompany.industry) and com.skapp.community.crmplanner.model.CrmIndustry (the entity this endpoint writes) share a simple name, and CrmValidations imports the enum while also hosting validateIndustryName, which exists solely to serve the entity. The next person adding an entity-typed industry validation here has to fully qualify one of them or silently validate the wrong concept. Consider renaming the enum to CrmIndustryType (it is already a closed value set distinct from the crm_industry master table) so the two never appear interchangeable.
3 participants
PR checklist
TaskId: (https://github.com/SkappHQ/skapp/issues/[id])
Summary
POST /v2/crm/industryso an organisation-wide industry can be created on thefly, without leaving the company modal. Returns
201with{ id, name }.ROLE_CRM_SALES_MANAGER, matching company creation.CrmIndustryControllerV2,CrmIndustryServiceV2+ impl,CrmIndustryCreateDto,CrmIndustryCreateResponseDtoV2.CrmValidations:validateIndustryName— rejects null/blank, and names longer thanCrmConstants.INDUSTRY_NAME_MAX_LENGTH(100).normalizeIndustryName— trims and collapses runs of internal whitespace, reusingthe existing
ValidationConstant.MULTIPLE_WHITESPACE_PATTERN, so"Real estate "and
"Real estate"are treated as the same name.CrmIndustryDao.existsByNameIgnoreCaseAndIsDeletedFalse, throwingCRM_ERROR_INDUSTRY_EXISTS.CrmIndustrygains anis_deletedcolumn (non-null, defaults tofalse) so deletedindustries don't block name reuse.
industry-name-required,industry-name-length,industry-name-exists.How to test
ROLE_CRM_SALES_MANAGER.POST /v2/crm/industrywith{ "name": "Tourism" }→ expect201and{ id, name: "Tourism" }.industry-name-existserror.{ "name": " tourism " }→ still rejected as a duplicate (normalised,case-insensitive).
{ "name": "" }→industry-name-required.industry-name-length.403.CrmIndustryControllerV2IntegrationTestandCrmValidationsTest.Project Checklist
CrmIndustryControllerV2IntegrationTest(new, 190 lines)CrmValidationsTest(+96 lines covering normalisation and both validation failures)Additional Information
feat/crm-add-industry-via-company-modal-FE, whichconsumes this endpoint. This PR is standalone and safe to merge first.
from board init-data, so no
GETwas needed here.