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

Remove event rule session storage #170

Open
wants to merge 14 commits into
base: event-rule-config-enhancement
Choose a base branch
from
Open

Remove event rule session storage #170

wants to merge 14 commits into from

Conversation

raviks789
Copy link
Contributor

@raviks789 raviks789 commented Apr 5, 2024 •
edited
Loading

resolves #173

Blocked by #159

@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Apr 5, 2024
@raviks789 raviks789 changed the base branch from main to event-rule-config-enhancement April 5, 2024 15:04
@raviks789 raviks789 force-pushed the event-rule-remove-session-storage branch 3 times, most recently from 577a616 to 128624e Compare April 8, 2024 16:25
@raviks789 raviks789 force-pushed the event-rule-config-enhancement branch from 38298bb to bfb1542 Compare April 9, 2024 08:32
@raviks789 raviks789 force-pushed the event-rule-remove-session-storage branch 6 times, most recently from dd2c1a5 to 9f051ac Compare April 10, 2024 10:55
@raviks789 raviks789 changed the title WIP: Event rule remove session storage Remove event rule session storage Apr 10, 2024
@raviks789 raviks789 marked this pull request as ready for review April 10, 2024 11:00
@raviks789 raviks789 requested a review from nilmerg April 10, 2024 11:00
@raviks789 raviks789 force-pushed the event-rule-config-enhancement branch from bfb1542 to ce523f6 Compare April 10, 2024 11:01
@raviks789 raviks789 force-pushed the event-rule-remove-session-storage branch from 9f051ac to a139259 Compare April 10, 2024 11:01
@raviks789 raviks789 self-assigned this Apr 11, 2024
@raviks789 raviks789 force-pushed the event-rule-config-enhancement branch 2 times, most recently from 473161f to d35dd96 Compare April 11, 2024 15:28
@raviks789 raviks789 force-pushed the event-rule-remove-session-storage branch from a139259 to 913e324 Compare April 23, 2024 11:23
@sukhwinder33445 sukhwinder33445 force-pushed the event-rule-remove-session-storage branch 2 times, most recently from cdddd29 to e08b25b Compare April 23, 2024 13:59
sukhwinder33445
sukhwinder33445 requested changes Apr 23, 2024
View reviewed changes
Copy link
Contributor

@sukhwinder33445 sukhwinder33445 left a comment

Choose a reason for hiding this comment

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

When I add a new event rule and refresh the form before submitting, the name disappears and I can no longer save the form.

@raviks789 raviks789 force-pushed the event-rule-remove-session-storage branch from e08b25b to 8e62fee Compare April 23, 2024 15:12
@raviks789 raviks789 force-pushed the event-rule-config-enhancement branch 3 times, most recently from 04e9fb6 to 18bbc0a Compare April 24, 2024 11:29
@raviks789 raviks789 mentioned this pull request Apr 24, 2024
Draft
@raviks789 raviks789 force-pushed the event-rule-config-enhancement branch from fd35468 to c77091c Compare June 4, 2024 08:25
@raviks789 raviks789 force-pushed the event-rule-remove-session-storage branch from 319d6cd to 258191c Compare June 4, 2024 09:49
sukhwinder33445
sukhwinder33445 reviewed Jun 7, 2024
View reviewed changes
Copy link
Contributor

@sukhwinder33445 sukhwinder33445 left a comment

Choose a reason for hiding this comment

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

I cannot remove a recently added escalation.

@raviks789
Copy link
Contributor Author

I cannot remove a recently added escalation.

I think this is related to the EventRuleConfigForm::getChanges() method you have implemented. The method is returning an empty array instead of the changes. This is causing the form to not save the changes.

@sukhwinder33445
Copy link
Contributor

No the method works fine, the checked values are equal.

@raviks789
Copy link
Contributor Author

No the method works fine, the checked values are equal.

I just checked. The checked values are different. I mean $values and $dbValuesToCompare are different. And this only happens when the last escalation is being deleted.

@sukhwinder33445
Copy link
Contributor

You're right, either it worked before or we simply overlooked the bug. This must be fixed.

@sukhwinder33445
Copy link
Contributor

EventRuleConfigForm::getChanges() should work now, please test.

sukhwinder33445
sukhwinder33445 reviewed Jun 7, 2024
View reviewed changes
application/forms/EventRuleConfigForm.php
$db->beginTransaction();
$db->update('rule', ['object_filter' => $this->objectFilter], ['id = ?' => $id]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please only update object_filter if it has been changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there are no changes, the transaction does not even happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed offline, this is also triggered if we only add a new escalation and the filter remains the same.

@sukhwinder33445 sukhwinder33445 force-pushed the event-rule-remove-session-storage branch from f92b668 to 258191c Compare June 7, 2024 14:03
@raviks789 raviks789 force-pushed the event-rule-remove-session-storage branch from 258191c to d1b418d Compare June 7, 2024 15:11
@raviks789 raviks789 force-pushed the event-rule-config-enhancement branch from c77091c to 437291d Compare June 20, 2024 10:17
@raviks789 raviks789 force-pushed the event-rule-remove-session-storage branch from d1b418d to d24b072 Compare June 20, 2024 10:18
@raviks789 raviks789 force-pushed the event-rule-config-enhancement branch from 437291d to c00a2f1 Compare June 20, 2024 13:10
raviks789 and others added 14 commits June 20, 2024 15:11
@raviks789
Remove event rule session storage
cf49a7c
@raviks789
EventRulesController: Move addAction to EventRuleController
a2022e6
@raviks789
PHPStan: Update baseline
f65f877
@sukhwinder33445 @raviks789
EventRuleConfigForm::ON_SUCCESS: Only save data if changes have bee…
4366b65 
…n made
@raviks789
Update column 1 after successful changes to event rule configuration
1acaf25
@raviks789
EventRuleConfigForm::getChanges: Do not check for changes in config f…
2cd3a08 
…ilter
@raviks789
Improve event rule configuration layout
b9e9519
@raviks789
Move creation of form submit buttons to EventRuleConfigForm
64187b6
@raviks789
EventRuleForm: Save new event rule to the database on submission
6e871e4
@raviks789
Make event rule delete action CSRF protected
62cd26f
@raviks789
EventRulesController: Make getFilter method private
a5b1d1a
@raviks789
Fix: Remove false positive due to not checking object_filter changes
e8c6546
@raviks789
Fix: Review comments
6f5a77a
@raviks789
EventRuleConfigForm: Consider escalation deletion while checking for …
6d24f32 
…changes
@raviks789 raviks789 force-pushed the event-rule-remove-session-storage branch from d24b072 to 6d24f32 Compare June 20, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sukhwinder33445 sukhwinder33445 sukhwinder33445 requested changes

@nilmerg nilmerg Awaiting requested review from nilmerg

Assignees

@raviks789 raviks789

Labels
cla/signed CLA is signed by all contributors of a PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@raviks789 @sukhwinder33445 @nilmerg