chore: Add admin email to config DB #36596
Conversation
WalkthroughThe changes involve the introduction of new classes and methods for handling admin email configurations, along with modifications to existing services and migration scripts. Key updates include the deprecation of an existing method in Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/InstanceAdminMetaDTO.java (1)
9-10: Let's add some explanations to our fields!Class, while your fields are correctly defined, it's always good practice to include JavaDoc comments. This helps other students understand the purpose of each field. Let's add some comments like this:
/** * The email address of the instance admin. */ String email; /** * A hash of the email domain for security purposes. */ String emailDomainHash;Remember, good documentation is key to helping others understand your code!
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java (2)
159-167: Ensure secure management of environment variablesAccessing environment variables directly is acceptable, but it's crucial to ensure that sensitive data such as API keys are managed securely and not exposed inadvertently.
If you need guidance on securely managing environment variables or implementing a configuration service, please let me know.
138-172: Add unit tests forgetTenantConfigurationmethodTo ensure the robustness of the new logic, it's advisable to add unit tests covering scenarios like missing configurations, null values, and malformed data. This will help in catching potential issues early.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
- app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonConfig.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/InstanceAdminMetaDTO.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/HashUtils.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration063AddInstanceAdminDetailsToDB.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/TenantServiceImpl.java (0 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCEImpl.java (4 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PingScheduledTaskCEImpl.java (4 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserSignupCEImpl.java (3 hunks)
💤 Files not reviewed due to no reviewable changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/TenantServiceImpl.java
🧰 Additional context used
📓 Learnings (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCEImpl.java (1)
Learnt from: abhvsn PR: appsmithorg/appsmith#33894 File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCEImpl.java:256-258 Timestamp: 2024-05-31T13:06:01.938Z Learning: Different values for `EMAIL_DOMAIN_HASH` and `ADMIN_EMAIL_DOMAIN_HASH` are used in events to facilitate granular filtering on third-party services.
🔇 Additional comments (16)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/HashUtils.java (3)
1-5: Well done, class! The package declaration and imports are spot on!The package
com.appsmith.server.helpersis a perfect fit for our utility class. And you've chosen just the right tools for the job with Apache Commons Codec and Lang. It's like bringing the right textbooks to class!
6-7: A+ for the class declaration, students!The
HashUtilsclass is like a well-organized toolbox. It's public, so everyone can use it, and it doesn't need a constructor because all its tools (methods) are static. Just remember, with great power comes great responsibility!
8-10: Excellent work on thehashmethod, class!This method is like a well-oiled machine. It checks if the input is empty (always good to mind your manners), and if not, it applies the SHA-256 algorithm faster than you can say "cryptography"! It's short, sweet, and to the point - just like a good answer in class.
app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/InstanceAdminMetaDTO.java (1)
1-8: Class, pay attention to the structure!Well done, class! The package declaration, imports, and class structure are all in order. You've correctly used Lombok's @DaTa annotation, which will save you from writing boilerplate code. Keep up the good work!
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration063AddInstanceAdminDetailsToDB.java (2)
1-22: Class, today we'll be looking at a well-organized migration file.I'm pleased to see that you've properly declared the package and imported all the necessary classes. This shows good attention to detail and understanding of the tools you'll be using in your migration. Keep up the good work!
23-26: Excellent use of annotations, class!You've done a splendid job with the class declaration and annotations. Let's break it down:
- @requiredargsconstructor: This will create a constructor with required arguments, saving you time and reducing boilerplate code.
- @slf4j: This gives you access to a logger, which is crucial for monitoring and debugging your migration.
- @ChangeUnit: This is the star of the show! It tells our migration framework that this class is a migration unit. The 'order' and 'id' parameters are particularly important:
- 'order' ensures this migration runs in the correct sequence.
- 'id' provides a unique identifier for this migration.
Remember, class, in the world of database migrations, order and identification are key to maintaining data integrity!
app/server/appsmith-server/src/main/java/com/appsmith/server/configurations/CommonConfig.java (1)
151-151: Class, pay attention to this important change!Now, children, we've added a new annotation to our
getAdminEmailDomainHash()method. Can anyone tell me what this annotation does? That's right, it marks the method as deprecated! And what does "deprecated" mean? It means we're planning to remove this method in the future, just like how we remove old, worn-out books from our library.Let's discuss why this is important:
- It's like putting a "going out of business" sign on a shop. It warns other developers that they shouldn't use this method in new code.
- The
forRemoval = trueparameter is like telling everyone, "This shop is definitely closing, not just thinking about it!"Now, for your homework:
- Search the entire codebase for uses of this method. We need to know who's still relying on it!
- Think about what alternatives we can provide. Remember, good developers always offer a solution when they take something away!
Here's a little script to help you with your homework:
Class dismissed! Don't forget to run these searches and report back with your findings!
app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java (1)
207-207: Excellent addition, class! Let's discuss this new constant.Well done on adding the
INSTANCE_ADMIN_CONFIGconstant to ourFieldNameCEclass. This addition follows our established naming conventions and coding standards perfectly.Let's take a moment to understand why this is important:
Consistency: By using a constant, we ensure that the string "instanceAdminConfig" is used consistently throughout our codebase. This reduces the chance of typos and makes our code more maintainable.
Readability: The name
INSTANCE_ADMIN_CONFIGclearly communicates its purpose to other developers who might use this constant in the future.Refactoring: If we ever need to change the value of "instanceAdminConfig", we can do so in one place, and it will be updated everywhere the constant is used.
Remember, class, using constants for string literals is a best practice in Java. It's part of the DRY (Don't Repeat Yourself) principle we've discussed in our lessons.
Keep up the good work!
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PingScheduledTaskCEImpl.java (4)
8-9: Excellent addition of necessary importsBy importing
ConfigandInstanceAdminMetaDTO, we ensure that our code has access to these classes, which are essential for handling the instance admin configurations. This is a good practice to include all the required dependencies.
38-38: Good use of static import for constantsIncluding the static import of
INSTANCE_ADMIN_CONFIGallows us to reference this constant directly without qualifying it with the class name. This improves code readability and maintains consistency throughout our codebase.
175-176: Verify the tuple ordering inMono.zipWhen adding
instanceAdminEmailDomainMonoto theMono.zipmethod, it's crucial to ensure that the ordering of Monos corresponds correctly to how we access them later usingstatsData.getT1(),statsData.getT2(), etc.Please review all instances where
statsData.getT*()is called to confirm that they match the expected data after adding the new Mono. This helps prevent any mismatches or bugs due to incorrect tuple indexing.
194-195: Clarify assignment of email domain hash valuesIn this section, both
ADMIN_EMAIL_DOMAIN_HASHandEMAIL_DOMAIN_HASHare assigned the value ofstatsData.getT5(), which represents the admin's email domain hash. It's important to verify whether this is intentional.Please confirm if
EMAIL_DOMAIN_HASHshould also represent the admin's email domain hash. IfEMAIL_DOMAIN_HASHis intended to capture general user email domain hashes, we might need to adjust the assignment to reflect the correct data.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java (2)
138-147: Excellent retrieval of admin email domain hashYou've implemented a thoughtful approach to retrieve the admin email domain hash, including null checks and default values. This ensures the application remains robust even when configuration data is missing.
151-154: Effective use ofzipWithfor data aggregationYour use of
zipWithto combine the instance ID and admin email domain hash is commendable. It efficiently aggregates data from multiple sources in a reactive manner.app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserSignupCEImpl.java (2)
Line range hint
479-492: Properly Chain Asynchronous OperationsThe way you've chained
configMonowith the analytics event ensures that the analytics event is sent only after the configuration is handled. This is a good practice to maintain data consistency. Ensure that any delays in the configuration retrieval or saving won't adversely affect the user experience, especially if this method is part of a critical path.
Line range hint
6-61: Unused Imports DetectedThere are import statements added for
Config,InstanceAdminMetaDTO, andINSTANCE_ADMIN_CONFIG. Ensure that all imported classes and constants are used in the codebase to maintain code cleanliness. If they are necessary for the changes introduced, this is acceptable.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/HashUtils.java
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/InstanceAdminMetaDTO.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/InstanceAdminMetaDTO.java
Outdated
Show resolved
Hide resolved
.../main/java/com/appsmith/server/migrations/db/ce/Migration063AddInstanceAdminDetailsToDB.java
Outdated
Show resolved
Hide resolved
.../main/java/com/appsmith/server/migrations/db/ce/Migration063AddInstanceAdminDetailsToDB.java
Outdated
Show resolved
Hide resolved
.../appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PingScheduledTaskCEImpl.java
Outdated
Show resolved
Hide resolved
...erver/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java
Show resolved
Hide resolved
...er/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCEImpl.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserSignupCEImpl.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserSignupCEImpl.java
Show resolved
Hide resolved
…add-admin-email-to-db
…add-admin-email-to-db
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PingScheduledTaskCEImpl.java (1)
195-196: Let's examine these new additions to our properties map.Excellent work incorporating the admin email domain hash into our stats! You've correctly added both
ADMIN_EMAIL_DOMAIN_HASHandEMAIL_DOMAIN_HASHto the properties map.However, I noticed that both entries are using the same value (
statsData.getT5()). While this might be intentional, it could lead to confusion for future readers of the code. If these are indeed meant to be the same, consider adding a comment explaining why. If they're supposed to be different, we might need to revisit our data collection logic.Here's a suggestion to improve clarity:
String emailDomainHash = defaultIfEmpty(statsData.getT5(), ""); // Both ADMIN_EMAIL_DOMAIN_HASH and EMAIL_DOMAIN_HASH are set to the same value // as they represent the same data in this context entry(ADMIN_EMAIL_DOMAIN_HASH, emailDomainHash), entry(EMAIL_DOMAIN_HASH, emailDomainHash)Remember, clear code is like a well-organized lesson plan - it helps everyone understand the intent and reduces confusion!
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCEImpl.java (1)
Line range hint
227-273: Great improvements to the sendEvent method, but let's discuss a potential issue, class.You've done an excellent job updating the
sendEventmethod to include the admin email domain hash in the analytics data. The caching mechanism using the static variable is a smart approach to improve performance.However, I'd like to draw your attention to a potential issue:
adminEmailDomainHash = InstanceAdminMetaDTO.fromJsonObject(config.getConfig()) .getEmailDomainHash();This line could throw an exception if the
config.getConfig()returns invalid JSON or ifgetEmailDomainHash()returns null. Let's add some error handling to make our code more robust.Here's a suggested improvement:
try { adminEmailDomainHash = InstanceAdminMetaDTO.fromJsonObject(config.getConfig()) .getEmailDomainHash(); if (adminEmailDomainHash == null) { adminEmailDomainHash = ""; } } catch (Exception e) { log.error("Failed to parse admin email domain hash", e); adminEmailDomainHash = ""; }This change will ensure that we handle any potential exceptions and always have a valid (even if empty) value for
adminEmailDomainHash.Keep up the good work, and remember: proper error handling is crucial for maintaining a stable application!
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration064AddInstanceAdminDetailsToDB.java (1)
78-81: Ensure that configuration is saved regardless of email availability.Currently, the configuration is only saved if
adminEmailhas length. However, if no admin email is found, it might be beneficial to save the configuration with a placeholder or note that no admin email is set. This ensures the config object reflects the state of the system.Consider modifying the logic:
if (StringUtils.hasLength(adminEmail)) { config.setConfig(InstanceAdminMetaDTO.toJsonObject(adminEmail)); mongoTemplate.save(config); + } else { + // Save config with a note or placeholder indicating no admin email is set + config.setConfig(InstanceAdminMetaDTO.toJsonObject("No admin email set")); + mongoTemplate.save(config); }This approach ensures that the configuration is always present, which can simplify downstream processes that rely on this config.
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java (1)
521-524: Handle the Scenario When No Valid Admin Email Is ProvidedIf no valid admin email is found, the code does not set up the
INSTANCE_ADMIN_CONFIG. It's important to handle this case explicitly. Consider adding a warning or an error log to inform administrators that the instance admin configuration could not be set due to the absence of a valid admin email.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java (4 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration064AddInstanceAdminDetailsToDB.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCEImpl.java (5 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PingScheduledTaskCEImpl.java (4 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserSignupCEImpl.java (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/TenantServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserSignupCEImpl.java
🧰 Additional context used
📓 Learnings (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCEImpl.java (1)
Learnt from: abhvsn PR: appsmithorg/appsmith#33894 File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCEImpl.java:256-258 Timestamp: 2024-05-31T13:06:01.938Z Learning: Different values for `EMAIL_DOMAIN_HASH` and `ADMIN_EMAIL_DOMAIN_HASH` are used in events to facilitate granular filtering on third-party services.
🔇 Additional comments (6)
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PingScheduledTaskCEImpl.java (1)
8-9: Class, let's review the new imports.Good job adding the necessary imports for
Config,InstanceAdminMetaDTO, andINSTANCE_ADMIN_CONFIG. These additions are crucial for the new functionality we're implementing. Remember, keeping our imports organized is like maintaining a tidy classroom - it helps everyone find what they need quickly!Also applies to: 38-38
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCEImpl.java (3)
11-11: Well done on adding the necessary imports, class!You've correctly imported the
ConfigandInstanceAdminMetaDTOclasses. These new imports will be essential for working with the instance admin configuration. Keep up the good work!Also applies to: 15-15
45-46: Excellent use of static imports, students!You've made a smart choice by statically importing the
hashmethod fromHashUtils. This will make your code cleaner and more readable when using thehashfunction. Well done!
63-64: Good job introducing a static variable, class!You've added a static variable
adminEmailDomainHashto cache the admin email domain hash. This is an excellent approach to improve performance by avoiding repeated calculations. Remember, caching is a powerful tool when used correctly!app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (1)
176-176: Well done, class! This change improves consistency.Now, children, let's examine what we've learned here. By adding
.as(this::toResponseDTO), we're ensuring that the tenant configuration is wrapped in aResponseDTO, just like its classmates. This is an excellent example of maintaining consistency in our code, which is very important for readability and maintainability.Can anyone tell me why consistency is important in coding? That's right, it makes our code easier to understand and reduces the chance of errors. Good job on this improvement!
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration064AddInstanceAdminDetailsToDB.java (1)
93-93: Consistency is key in log messages.Similarly, on line 93, the log message indicates "Skipping migration 64". Since this is appropriate for this migration, please ensure that all related log messages consistently use the correct migration number.
.../appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PingScheduledTaskCEImpl.java
Show resolved
Hide resolved
.../appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PingScheduledTaskCEImpl.java
Show resolved
Hide resolved
.../main/java/com/appsmith/server/migrations/db/ce/Migration064AddInstanceAdminDetailsToDB.java
Outdated
Show resolved
Hide resolved
.../main/java/com/appsmith/server/migrations/db/ce/Migration064AddInstanceAdminDetailsToDB.java
Outdated
Show resolved
Hide resolved
.../main/java/com/appsmith/server/migrations/db/ce/Migration064AddInstanceAdminDetailsToDB.java
Outdated
Show resolved
Hide resolved
.../main/java/com/appsmith/server/migrations/db/ce/Migration064AddInstanceAdminDetailsToDB.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PingScheduledTaskCEImpl.java (2)
158-168: Well done on implementing theinstanceAdminEmailDomainMono, students!Your implementation shows a good understanding of reactive programming and error handling. Let's break it down:
- You've correctly used
configService.getByName(INSTANCE_ADMIN_CONFIG)to retrieve the configuration.- The
onErrorResumeandswitchIfEmptyoperators ensure graceful handling of errors and missing configurations.- You've properly handled the case where
config.getConfig()might be null.However, I have a small suggestion to enhance our error handling:
.onErrorResume(error -> { log.debug("Error fetching instance admin config", error); return Mono.empty(); })By adding this debug log, we can keep track of any issues without cluttering our logs with expected scenarios. Remember, good error handling is like having a well-prepared lesson plan - it helps us navigate unexpected situations smoothly!
176-177: Excellent integration of the new data, class!You've successfully incorporated the
instanceAdminEmailDomainMonointo ourpingStatsmethod. Your approach of adding it to theMono.zipcall is spot on!To make our code even more readable, let's consider using named variables when unpacking the tuple. Here's a small improvement we could make:
.flatMap(tuple -> { final String instanceId = tuple.getT1(); final String ipAddress = tuple.getT2(); Tuple7<Long, Long, Long, Long, Long, Long, Map<String, Long>> counts = tuple.getT3(); Long publicAppsCount = tuple.getT4(); String adminEmailDomainHash = tuple.getT5(); // Use these named variables in the subsequent code // ... })This way, anyone reading our code can immediately understand what each part of the tuple represents. It's like labeling our classroom supplies - it makes everything easier to find and understand!
Also applies to: 178-184
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/InstanceAdminMetaDTO.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/constants/FieldName.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration064AddInstanceAdminDetailsToDB.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PingScheduledTaskCEImpl.java (3 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/constants/FieldName.java
🚧 Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/InstanceAdminMetaDTO.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration064AddInstanceAdminDetailsToDB.java
🧰 Additional context used
📓 Learnings (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PingScheduledTaskCEImpl.java (1)
Learnt from: abhvsn PR: appsmithorg/appsmith#36596 File: app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PingScheduledTaskCEImpl.java:158-168 Timestamp: 2024-10-04T05:57:16.990Z Learning: In `PingScheduledTaskCEImpl.java`, when retrieving the instance admin config using `configService.getByName(INSTANCE_ADMIN_CONFIG)`, the absence of the resource is expected in some scenarios, and logging the error is not desired.
🔇 Additional comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/PingScheduledTaskCEImpl.java (2)
8-9: Excellent job with the new imports, class!You've correctly added the necessary imports for
ConfigandInstanceAdminMetaDTO, as well as theINSTANCE_ADMIN_CONFIGconstant. These additions are crucial for the new functionality we're implementing. Well done on maintaining proper import organization!Also applies to: 38-38
201-202: Great job updating our properties map, students!You've successfully added the new entries for
ADMIN_EMAIL_DOMAIN_HASHandEMAIL_DOMAIN_HASH. Your use ofdefaultIfEmptyensures we always have a value, even ifadminEmailDomainHashis null. That's excellent defensive programming!I do have a question for you:
Could you explain why we're using the same
adminEmailDomainHashvalue for bothADMIN_EMAIL_DOMAIN_HASHandEMAIL_DOMAIN_HASH? Is this intentional, or should these be different values?Remember, in our code, just like in our classroom, clarity is key. If this is intentional, a small comment explaining the reason would be helpful for future readers of our code.
Also applies to: 204-204
github-actions
bot
added
Admin Settings Product
| } | ||
|
|
||
| return hash(email.contains("@") ? email.split("@", 2)[1] : ""); | ||
| } |
There was a problem hiding this comment.
Moving these methods from AnalyticsService to helper class to be reusable.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
Description
PR to move instance admin email to DB so that analytics events will always rely on the first user that get's created. In the current implementation we are dependent on env variable
APPSMITH_ADMIN_EMAILSwhich is error prone as the list can be updated manually and if the email domain is different for the first user it changes theadminEmailDomainHashfor the subsequent events.Fixes #35415
/test Sanity
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11175096350
Commit: 5233067
Cypress dashboard.
Tags: @tag.Sanity
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/Templates/CreateNewAppFromTemplates_spec.ts
- cypress/e2e/Sanity/Datasources/DatasourceForm_spec.js
List of identified flaky tests.Fri, 04 Oct 2024 07:03:15 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor