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

feat: operation logs for enrollment requests #17

Merged
merged 36 commits into from
Aug 3, 2023
Merged

Conversation

julianramirez2
Copy link
Contributor

Description

A logging system has been implemented to enable traceability of operations performed on Enrollment Requests. This system will describe the action focusing on the questions: "Who?" "When?" and "What?"

For each Enrollment Request accessed, a panel will appear on the right-hand side, displaying the history of actions carried out on the post.

Testing instructions

To test the entire functionality, the idea is to access the Enrollments Manager and create a new Enrollment Request. Then, make changes to it and compare the actions taken with the generated logs.

Checklist for Merge

  • Tested in a remote environment
  • Updated documentation
  • Rebased master/main
  • Squashed commits

@MaferMazu
Copy link
Contributor

image
Looks good, but I have issues with the enrollment_request_type attribute because before, I had "Enroll," but the log said 1.
Another thing that is weird for me, but probably you have more experience with, is the order of the log. For some reason, I think the recent log should be at least not at the top, but I'm not sure what is the most intuitive form for WordPress operators. What do you think?

@julianramirez2
Copy link
Contributor Author

image Looks good, but I have issues with the enrollment_request_type attribute because before, I had "Enroll," but the log said 1. Another thing that is weird for me, but probably you have more experience with, is the order of the log. For some reason, I think the recent log should be at least not at the top, but I'm not sure what is the most intuitive form for WordPress operators. What do you think?

Initially, the idea of displaying the logs in this manner was based on how WooCommerce shows order notes, so it was most appropriate to follow that convention. However, considering the best UX practices, it is better to have the latest log displayed as the most recent one. It is more common and intuitive for users to read logs in this manner. Therefore, the correction was made, and now the logs are shown with the latest entry at the top and the oldest at the bottom.

@MaferMazu
Copy link
Contributor

MaferMazu commented Jul 31, 2023

Thanks for taking that comment into account. And I think you mean the newer log at the bottom and the oldest at the top. Btw thanks for removing the general info field.

Copy link
Contributor

@MaferMazu MaferMazu left a comment

Choose a reason for hiding this comment

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

I left you some comments. Please let me know what you think.

Copy link
Contributor

@MaferMazu MaferMazu left a comment

Choose a reason for hiding this comment

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

@julianramirez2 remember to add the end line in includes/model/Openedx_Woocommerce_Plugin_Logs.php.

I test this in Stage and looks good. I have one comment: if we create an Enrollment Request always, the action is "Object Created," but if we create the object with another action, for example, "Process request," we lose that information. Maybe it's a good idea only use the "Object Created" if we don't have action and I created that object. What do you think?

The rest looks good to me ✨

I wonder if @jvegalo has some time to do a quick review of this.

@julianramirez2
Copy link
Contributor Author

julianramirez2 commented Aug 2, 2023

@julianramirez2 remember to add the end line in includes/model/Openedx_Woocommerce_Plugin_Logs.php.

I test this in Stage and looks good. I have one comment: if we create an Enrollment Request always, the action is "Object Created," but if we create the object with another action, for example, "Process request," we lose that information. Maybe it's a good idea only use the "Object Created" if we don't have action and I created that object. What do you think?

The rest looks good to me ✨

I wonder if @jvegalo has some time to do a quick review of this.

I just added a new field called "object_status" to clarify this. So, the request type will remain in the information, and we'll also have this new field indicating if an object is new with the message "Object Created" or if it has been modified with the message "Object Modified."

image

MaferMazu
MaferMazu previously approved these changes Aug 2, 2023
Copy link
Contributor

@MaferMazu MaferMazu left a comment

Choose a reason for hiding this comment

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

I would like to know the opinion of @arangos since he knows more about good practices and quality in PHP than me. But for me looks good.

@arangos, do you have a chance to review this?

@arangos
Copy link

arangos commented Aug 2, 2023

I would like to know the opinion of @arangos since he knows more about good practices and quality in PHP than me. But for me looks good.

@arangos, do you have a chance to review this?

Done

@MaferMazu
Copy link
Contributor

Thanks for your contribution, @arangos.

@arangos
Copy link

arangos commented Aug 3, 2023

@MaferMazu Ready for merge

MaferMazu
MaferMazu previously approved these changes Aug 3, 2023
Copy link
Contributor

@MaferMazu MaferMazu 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. Thanks for taking all into account @julianramirez2. Please add an end line in the CSS file and you can squash and merge.

@julianramirez2 julianramirez2 merged commit 9d05522 into main Aug 3, 2023
@felipemontoya felipemontoya deleted the operation-logs branch March 21, 2024 17:31
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