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

Return version logging to prices DB table #980

Merged
merged 3 commits into from
Sep 14, 2020

Conversation

artur-intech
Copy link
Contributor

PaperTrail's Version model itself has been removed in #475, so those
columns are now useless

@vohmar
Copy link
Contributor

vohmar commented Sep 19, 2018

merge conflict with master

@vohmar vohmar assigned artur-intech and unassigned vohmar Sep 19, 2018
@artur-intech artur-intech force-pushed the remove-paper-trail-columns-from-prices branch from 00973e8 to 55d1258 Compare September 19, 2018 17:02
@artur-intech artur-intech assigned vohmar and unassigned artur-intech Sep 19, 2018
@vohmar
Copy link
Contributor

vohmar commented Sep 20, 2018

how does the registry currently log who made a change in pricelists? is the data collected and saved in the columns creator_str and updator_str preserved somewhere?

@vohmar vohmar assigned artur-intech and unassigned vohmar Sep 20, 2018
@artur-intech
Copy link
Contributor Author

Changes are not logged. I guess "Update price" button should be taken away http://prntscr.com/l1vmtr. Otherwise I don't see a point of having "Expire" button.

@artur-intech artur-intech assigned vohmar and unassigned artur-intech Oct 8, 2018
@vohmar
Copy link
Contributor

vohmar commented Oct 17, 2018

there is a reason for expire - price promotions. The use-case example is

  • you have a regular price beginning from 1.01.2018 without end date
  • now you want to have special temporary price promotion - one might add new price list record with starting and end date or if the end date is not set leave end date blank and expire the record once the moment arrives. So the expire button is basically quick action for updating the current price list record's end date to the moment of the button press.
  • update price offers more freedom - to change the start date, the price but basically it should be used only if there was some error, In regular operations when one wants to set a new price new record should be created and the last one expired.

It might be worth to discuss if there should be only one button like expire, but either way any changes made to any record incl price list entries should be logged and thus I do not see how could we drop papertrail columns for price list without replacing these with some alternative way of tracking changes.

@artur-intech
Copy link
Contributor Author

artur-intech commented Oct 23, 2018

Just to be sure you understand: currently, there is no audit log for prices at all. Presence of columns in question has no effect on that. They are useless.

Based on your last comment, we probably need to restore this feature. But in this case I wonder why have we removed it in the first place.

@teadur
Copy link
Contributor

teadur commented Oct 23, 2018

Just to be sure you understand: currently, there is no audit log for prices at all. Presence of columns in question has no effect on that. They are useless.

Based on your last comment, we probably need to restore this feature. But in this case I wonder why have we removed it in the first place.

Actually the columns in question are filled for data generated before deploying #475 and as of no new prices have been defined after that currently we haven't "lost" any data.

But thats good question why paper trail was removed from this table, sadly #475 doesn't have any reasoning, but sounds like data what should be audited for sure.

@vohmar vohmar added the blocked label Dec 3, 2018
@vohmar
Copy link
Contributor

vohmar commented Dec 3, 2018

the problem is not with the columns but with removed logging / version handling.

@artur-intech
Copy link
Contributor Author

@vohmar If I remember correctly, you said it is not needed. Should I close this PR and make a new ticket to restore versioning for Price model?

@artur-intech
Copy link
Contributor Author

@vohmar Reminder.

@vohmar
Copy link
Contributor

vohmar commented Oct 30, 2019

Yes we need to track changes with prices as with anything else in the registry - need to restore change tracking. It does not have to be handled by papertrail.

@teadur
Copy link
Contributor

teadur commented Aug 18, 2020

possibly will be fixed by #1563

@teadur teadur marked this pull request as draft August 18, 2020 19:14
@yulgolem yulgolem self-assigned this Sep 10, 2020
@yulgolem yulgolem force-pushed the remove-paper-trail-columns-from-prices branch 3 times, most recently from cbb9cf6 to 4c755b6 Compare September 10, 2020 11:30
@yulgolem yulgolem force-pushed the remove-paper-trail-columns-from-prices branch from 4c755b6 to 55ddd61 Compare September 10, 2020 11:50
@yulgolem yulgolem changed the title Remove PaperTrail columns from prices DB table Return version logging to prices DB table Sep 11, 2020
@yulgolem yulgolem force-pushed the remove-paper-trail-columns-from-prices branch from 55ddd61 to 8f1a4aa Compare September 11, 2020 08:11
@yulgolem yulgolem marked this pull request as ready for review September 11, 2020 08:23
Artur Beljajev and others added 3 commits September 14, 2020 12:45
PaperTrail's `Version` model itself has been removed in #475, so those
columns are now useless
@yulgolem yulgolem force-pushed the remove-paper-trail-columns-from-prices branch from 8f1a4aa to 6d908f6 Compare September 14, 2020 07:47
@yulgolem yulgolem assigned vohmar and unassigned yulgolem Sep 14, 2020
@vohmar vohmar added refactor Refactoring of existing feature and removed blocked labels Sep 14, 2020
@vohmar vohmar merged commit e14b0c9 into master Sep 14, 2020
@vohmar vohmar deleted the remove-paper-trail-columns-from-prices branch September 15, 2020 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactoring of existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants