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.

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

[ENG-5453] don't update last updated on project's page when log.should_hide=True #10943

Conversation

ihorsokhanexoft
Copy link

@ihorsokhanexoft ihorsokhanexoft commented Jan 23, 2025

Purpose

Update "Last updated" on project's page only when log.should_hide = False

Changes

Added if condition to check if NodeLog requires to update last_logged attribute
Update spam methods to hide their logs

QA Notes

Documentation

Side Effects

Ticket

https://openscience.atlassian.net/jira/software/c/projects/ENG/boards/145?selectedIssue=ENG-5453

@brianjgeiger brianjgeiger changed the base branch from develop to feature/b-and-i-25-01 January 29, 2025 16:36
Copy link
Collaborator

@brianjgeiger brianjgeiger left a comment

Choose a reason for hiding this comment

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

Could we get a test for this?

@ihorsokhanexoft
Copy link
Author

@brianjgeiger I've tried to play around with these actions and run new tests when should_hide=False passed to add_log (hardcoded for tests) and tests failed. After the fix, test were passed successfully

Copy link
Collaborator

@brianjgeiger brianjgeiger left a comment

Choose a reason for hiding this comment

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

Looks good to me. I may have someone more familiar with Spam take a quick look as well.

@brianjgeiger brianjgeiger dismissed their stale review February 4, 2025 20:11

Mistakenly chose the wrong option

Copy link
Collaborator

@brianjgeiger brianjgeiger left a comment

Choose a reason for hiding this comment

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

Looks good to me. I may have someone more familiar with the Spam code take a quick look as well.

@brianjgeiger brianjgeiger changed the title don't update last updated on project's page when log.should_hide=True [ENG-5453] don't update last updated on project's page when log.should_hide=True Feb 4, 2025
Copy link
Contributor

@Johnetordoff Johnetordoff left a comment

Choose a reason for hiding this comment

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

Just some questions about the requirements, but otherwise looks a good improvement.

@@ -1280,6 +1280,7 @@ def set_privacy(self, permissions, auth=None, log=True, save=True, meeting_creat
},
auth=auth,
save=False,
should_hide=True
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure we always want to hide these logs? Seems like it should only be hidden scenarios where we set privacy ourselves.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch. Yeah, let's not hide these logs.

@@ -868,6 +868,7 @@ def set_privacy(self, permissions, auth=None, log=True, save=True, check_addons=
},
auth=auth,
save=False,
should_hide=True
Copy link
Contributor

Choose a reason for hiding this comment

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

Also not sure if we want to hide these logs, it seems like intentional behavior, not sure about the use cases though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's also not hide these logs.

@@ -1280,6 +1280,7 @@ def set_privacy(self, permissions, auth=None, log=True, save=True, meeting_creat
},
auth=auth,
save=False,
should_hide=True
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch. Yeah, let's not hide these logs.

@@ -868,6 +868,7 @@ def set_privacy(self, permissions, auth=None, log=True, save=True, check_addons=
},
auth=auth,
save=False,
should_hide=True
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's also not hide these logs.

@brianjgeiger
Copy link
Collaborator

If the spam functions are calling set privacy, we may have to make the spam calls of set privacy hide their logs, while default behavior for set privacy would be to show the logs.

@ihorsokhanexoft
Copy link
Author

ihorsokhanexoft commented Feb 5, 2025

I also thought that making public/private project should add visible logs but I'm not familiar with spam functionality. Fixed it and conflict

@brianjgeiger brianjgeiger merged commit 98a2534 into CenterForOpenScience:feature/b-and-i-25-01 Feb 5, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants