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

Fix #3080: Activity Feed: Blank feed on my-data page for updating user's role or team #3131

Merged
merged 2 commits into from
Mar 4, 2022
Merged

Fix #3080: Activity Feed: Blank feed on my-data page for updating user's role or team #3131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d7b5ec6
Fix #3080: Activity Feed: Blank feed on my-data page for updating use…
vivekratnavel Mar 4, 2022
f9b80ed
Address review comments
vivekratnavel Mar 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions catalog-rest-service/src/main/java/org/openmetadata/catalog/Entity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,11 @@ public final class Entity {
public static final String AIRFLOW_PIPELINE = "airflowPipeline";
public static final String WEBHOOK = "webhook";

//
// List of entities whose changes should not be published to the Activity Feed
//
public static final List<String> ACTIVITY_FEED_EXCLUDED_ENTITIES = List.of(USER, TEAM, ROLE, POLICY, BOTS);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor: this could be a Set/HashSet


private Entity() {}

public static <T> void registerEntity(
Expand Down Expand Up @@ -185,6 +190,16 @@ public static boolean shouldHaveOwner(@NonNull String entityType) {
return !entityType.equals(TEAM);
}

/**
* Returns true if the change events of the given entity type should be published to the activity feed.
*
* @param entityType Type of the entity.
* @return true if change events of the entity should be published to activity feed, false otherwise
*/
public static boolean shouldDisplayEntityChangeOnFeed(@NonNull String entityType) {
return !ACTIVITY_FEED_EXCLUDED_ENTITIES.contains(entityType);
}

public static <T> EntityInterface<T> getEntityInterface(T entity) {
if (entity == null) {
return null;
Expand Down
21 changes: 13 additions & 8 deletions catalog-rest-service/src/main/java/org/openmetadata/catalog/events/ChangeEventHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,15 @@ public Void process(ContainerRequestContext requestContext, ContainerResponseCon

// Add a new thread to the entity for every change event
// for the event to appear in activity feeds
List<Thread> threads = getThreads(responseContext, changeEvent);
if (threads != null) {
for (var thread : threads) {
feedDao.create(thread);
if (Entity.shouldDisplayEntityChangeOnFeed(changeEvent.getEntityType())) {
List<Thread> threads = getThreads(responseContext);
if (threads != null) {
for (var thread : threads) {
// Don't create a thread if there is no message
if (!thread.getMessage().isEmpty()) {
feedDao.create(thread);
}
}
}
}
}
Expand Down Expand Up @@ -171,7 +176,7 @@ private static ChangeEvent copyChangeEvent(ChangeEvent changeEvent) {
.withCurrentVersion(changeEvent.getCurrentVersion());
}

private List<Thread> getThreads(ContainerResponseContext responseContext, ChangeEvent changeEvent) {
private List<Thread> getThreads(ContainerResponseContext responseContext) {
Object entity = responseContext.getEntity();
if (entity == null) {
return null; // Response has no entity to produce change event from
Expand All @@ -184,14 +189,14 @@ private List<Thread> getThreads(ContainerResponseContext responseContext, Change
return null;
}

return getThreads(entity, entityInterface.getChangeDescription(), changeEvent);
return getThreads(entity, entityInterface.getChangeDescription());
}

private List<Thread> getThreads(Object entity, ChangeDescription changeDescription, ChangeEvent changeEvent) {
private List<Thread> getThreads(Object entity, ChangeDescription changeDescription) {
List<Thread> threads = new ArrayList<>();
var entityInterface = Entity.getEntityInterface(entity);

Map<EntityLink, String> messages = ChangeEventParser.getFormattedMessages(changeDescription, entity, changeEvent);
Map<EntityLink, String> messages = ChangeEventParser.getFormattedMessages(changeDescription, entity);

// Create an automated thread
for (var link : messages.keySet()) {
Expand Down
6 changes: 2 additions & 4 deletions catalog-rest-service/src/main/java/org/openmetadata/catalog/util/ChangeEventParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import org.openmetadata.catalog.Entity;
import org.openmetadata.catalog.resources.feeds.MessageParser.EntityLink;
import org.openmetadata.catalog.type.ChangeDescription;
import org.openmetadata.catalog.type.ChangeEvent;
import org.openmetadata.catalog.type.EntityReference;
import org.openmetadata.catalog.type.FieldChange;

Expand All @@ -46,8 +45,7 @@ private enum CHANGE_TYPE {
DELETE
}

public static Map<EntityLink, String> getFormattedMessages(
ChangeDescription changeDescription, Object entity, ChangeEvent changeEvent) {
public static Map<EntityLink, String> getFormattedMessages(ChangeDescription changeDescription, Object entity) {
// Store a map of entityLink -> message
Map<EntityLink, String> messages;

Expand Down Expand Up @@ -82,7 +80,7 @@ private static Map<EntityLink, String> getFormattedMessages(
}

private static String getFieldValue(Object fieldValue) {
if (fieldValue != null) {
if (fieldValue != null && !fieldValue.toString().isEmpty()) {
try {
// Check if field value is a json string
JsonValue json = JsonUtils.readJson(fieldValue.toString());
Expand Down
31 changes: 8 additions & 23 deletions catalog-rest-service/src/test/java/org/openmetadata/catalog/util/ChangeEventParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
import org.openmetadata.catalog.resources.databases.TableResourceTest;
import org.openmetadata.catalog.resources.feeds.MessageParser.EntityLink;
import org.openmetadata.catalog.type.ChangeDescription;
import org.openmetadata.catalog.type.ChangeEvent;
import org.openmetadata.catalog.type.EntityReference;
import org.openmetadata.catalog.type.FieldChange;
import org.openmetadata.catalog.type.TagLabel;
Expand All @@ -57,16 +56,14 @@ public void setup(TestInfo test) throws IOException, URISyntaxException {
@Test
void testFormattedMessages() throws JsonProcessingException {
ChangeDescription changeDescription = new ChangeDescription();
ChangeEvent changeEvent = new ChangeEvent();
// Simulate updating tags of an entity from tag1 -> tag2
FieldChange addTag = new FieldChange();
addTag.withName("tags").withNewValue("tag2");
FieldChange deleteTag = new FieldChange();
deleteTag.withName("tags").withOldValue("tag1");
changeDescription.withFieldsAdded(List.of(addTag)).withFieldsDeleted(List.of(deleteTag)).withPreviousVersion(1.0);
changeEvent.withChangeDescription(changeDescription).withPreviousVersion(1.0).withCurrentVersion(1.1);

Map<EntityLink, String> messages = ChangeEventParser.getFormattedMessages(changeDescription, TABLE, changeEvent);
Map<EntityLink, String> messages = ChangeEventParser.getFormattedMessages(changeDescription, TABLE);
assertEquals(1, messages.size());

TagLabel tag1 = new TagLabel();
Expand All @@ -78,8 +75,7 @@ void testFormattedMessages() throws JsonProcessingException {
addTag.withNewValue(JsonUtils.pojoToJson(List.of(tag2)));
deleteTag.withOldValue(JsonUtils.pojoToJson(List.of(tag1)));

Map<EntityLink, String> jsonMessages =
ChangeEventParser.getFormattedMessages(changeDescription, TABLE, changeEvent);
Map<EntityLink, String> jsonMessages = ChangeEventParser.getFormattedMessages(changeDescription, TABLE);
assertEquals(1, jsonMessages.size());

// The entity links and values of both the messages should be the same
Expand All @@ -89,17 +85,15 @@ void testFormattedMessages() throws JsonProcessingException {
@Test
void testEntityReferenceFormat() throws JsonProcessingException {
ChangeDescription changeDescription = new ChangeDescription();
ChangeEvent changeEvent = new ChangeEvent();
// Simulate adding owner to a table
EntityReference entityReference = new EntityReference();
entityReference.withId(UUID.randomUUID()).withName("user1").withDisplayName("User One");
FieldChange addOwner = new FieldChange();
addOwner.withName("owner").withNewValue(JsonUtils.pojoToJson(entityReference));

changeDescription.withFieldsAdded(List.of(addOwner)).withPreviousVersion(1.0);
changeEvent.withChangeDescription(changeDescription).withPreviousVersion(1.0).withCurrentVersion(1.1);

Map<EntityLink, String> messages = ChangeEventParser.getFormattedMessages(changeDescription, TABLE, changeEvent);
Map<EntityLink, String> messages = ChangeEventParser.getFormattedMessages(changeDescription, TABLE);
assertEquals(1, messages.size());

assertEquals("Added **owner**: `User One`", messages.values().iterator().next());
Expand All @@ -108,15 +102,13 @@ void testEntityReferenceFormat() throws JsonProcessingException {
@Test
void testUpdateOfString() {
ChangeDescription changeDescription = new ChangeDescription();
ChangeEvent changeEvent = new ChangeEvent();
// Simulate a change of description in table
FieldChange updateDescription = new FieldChange();
updateDescription.withName("description").withNewValue("new description").withOldValue("old description");

changeDescription.withFieldsUpdated(List.of(updateDescription)).withPreviousVersion(1.0);
changeEvent.withChangeDescription(changeDescription).withPreviousVersion(0.1).withCurrentVersion(1.1);

Map<EntityLink, String> messages = ChangeEventParser.getFormattedMessages(changeDescription, TABLE, changeEvent);
Map<EntityLink, String> messages = ChangeEventParser.getFormattedMessages(changeDescription, TABLE);
assertEquals(1, messages.size());

assertEquals(
Expand All @@ -135,11 +127,8 @@ void testUpdateOfString() {
.withFieldsDeleted(List.of(deleteDescription))
.withPreviousVersion(1.0);

changeEvent.withChangeDescription(changeDescription).withPreviousVersion(0.1).withCurrentVersion(1.1);

// now test if both the type of updates give the same message
Map<EntityLink, String> updatedMessages =
ChangeEventParser.getFormattedMessages(changeDescription, TABLE, changeEvent);
Map<EntityLink, String> updatedMessages = ChangeEventParser.getFormattedMessages(changeDescription, TABLE);
assertEquals(1, updatedMessages.size());

assertEquals(messages.keySet().iterator().next(), updatedMessages.keySet().iterator().next());
Expand All @@ -149,7 +138,6 @@ void testUpdateOfString() {
@Test
void testMajorSchemaChange() {
ChangeDescription changeDescription = new ChangeDescription();
ChangeEvent changeEvent = new ChangeEvent();
// Simulate a change of column name in table
FieldChange addColumn = new FieldChange();
addColumn
Expand All @@ -167,9 +155,8 @@ void testMajorSchemaChange() {
.withFieldsAdded(List.of(addColumn))
.withFieldsDeleted(List.of(deleteColumn))
.withPreviousVersion(1.3);
changeEvent.withChangeDescription(changeDescription).withPreviousVersion(0.1).withCurrentVersion(2.3);

Map<EntityLink, String> messages = ChangeEventParser.getFormattedMessages(changeDescription, TABLE, changeEvent);
Map<EntityLink, String> messages = ChangeEventParser.getFormattedMessages(changeDescription, TABLE);
assertEquals(1, messages.size());

assertEquals(
Expand All @@ -186,9 +173,8 @@ void testMajorSchemaChange() {
.withFieldsAdded(List.of(addColumn))
.withFieldsDeleted(List.of(deleteColumn))
.withPreviousVersion(1.3);
changeEvent.withChangeDescription(changeDescription).withPreviousVersion(0.1).withCurrentVersion(2.3);

messages = ChangeEventParser.getFormattedMessages(changeDescription, TABLE, changeEvent);
messages = ChangeEventParser.getFormattedMessages(changeDescription, TABLE);
assertEquals(1, messages.size());

assertEquals(
Expand All @@ -205,9 +191,8 @@ void testMajorSchemaChange() {
.withFieldsAdded(List.of(addColumn))
.withFieldsDeleted(List.of(deleteColumn))
.withPreviousVersion(1.4);
changeEvent.withChangeDescription(changeDescription).withPreviousVersion(0.1).withCurrentVersion(2.4);

messages = ChangeEventParser.getFormattedMessages(changeDescription, TABLE, changeEvent);
messages = ChangeEventParser.getFormattedMessages(changeDescription, TABLE);
assertEquals(1, messages.size());

assertEquals(
Expand Down