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

Add column created_date to action_schedule #19068

Merged
merged 2 commits into from
Dec 14, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Nov 28, 2020

Overview

The additional field part of #18298

Before

No created date column

After

Column created

Technical Details

@seamuslee001 @monishdeb @JoeMurray I decided to try to peel off the field creation part of #18298 as the PR seems to be fully review ready but has failed to attract review & now is stale so getting the field merged seemed like it would help.

However, in the process I started to question whether the field is correct. The field is a created_date & has a now timestamp default - but it is also exposed, unlike any other create_date field. It allows manipulation in order to allow people to treat it as an effective date.

My feeling is that it should either be

  1. not exposed or
  2. renamed to something like effective_start_date or process_from_date and while we might have a form default it would be optional at the form layer and not have a DB default in this case.

Looking at the larger PR I suspect that it might be easier to get through review with 2 as the risk of unintended consequences is lower I would imagine

Comments

I think this will fail on tests but per technical details I'm unsure that it is the correct field as stands

@civibot
Copy link

civibot bot commented Nov 28, 2020

(Standard links)

@civibot civibot bot added the master label Nov 28, 2020
@seamuslee001
Copy link
Contributor

well it passed tests @eileenmcnaughton and I'm not really sure as Monish was the one working on this task rather than I

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 yeah - I just don't know if it should be merged as per above I'm not sure the column being added is right

@JoeMurray
Copy link
Contributor

So functionally, my sense is that we want the field to be defaulted to now() when record is created. I don't see any use case for editing this field. There is a weak case for needing to see the value in case of errors, but really it is only needed by developers during debugging. So I would say 1) do not expose the field.

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 as discussed I've updated this to add the effective_start_date and effective_end_date to set the valid range.

However, in the end I didn't remove created_date but rather added modified_date. My thinking was simply that we are seeing increasing requests for these fields on various tables & should probably treat them as a good thing to add while we are at it

@seamuslee001
Copy link
Contributor

@seamuslee001
Copy link
Contributor

This is as it was agreed in the product maintenance meeting

@eileenmcnaughton eileenmcnaughton force-pushed the actsched branch 2 times, most recently from 0cee9c1 to 6e8a980 Compare December 11, 2020 20:13
@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton
Copy link
Contributor Author

PHP Fatal error: Allowed memory size of 2147483648 bytes exhausted (tried to allocate 276574758 bytes) in phar:///home/jenkins/bknix-dfl/extern/phpunit7

@eileenmcnaughton
Copy link
Contributor Author

test this please

@seamuslee001
Copy link
Contributor

Jenkins re test this please

Since we expect these to be managed by mysql to some extent they seem best ignored
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants