SeedCompany / cord-field

CORD UI
MIT License
7 stars 3 forks source link

Engagement's initial_end date occurring before stage_begin date. #43

Closed kjones140 closed 4 years ago

kjones140 commented 5 years ago

Issue: How is the initial_end date derived in prodCord.Engagments date array? We are encountering invalid dates in Cord where a language engagement is specified as ending (initial_end date) before the language even starts (stage_begin date)

Painpoint: Incorrect data where an engagement is expected to end on a date before the starting date will cause confusion for our users, data integrity of Domo questioned, manual work for BI team to correct the data and unable to accurately forecast engagements that will be ending soon.

Priority: High

megbacino commented 4 years ago

Is there any progess on this?

candleshine commented 4 years ago

@megbacino What should the default initial_end_date be? a year from the start date?

candleshine commented 4 years ago

Unless directed otherwise, This is the plan of attack: Option A: On save: if end date is < start date throw error and do not save.

or

Option B: On save: if end date is < start date, set the end date to a year after start date.

megbacino commented 4 years ago

Daniel, Let me check with Sue tomorrow. I’m pretty sure we don’t want an error because there is no one to monitor that. Meg

On Oct 17, 2019, at 5:04 PM, Daniel notifications@github.com wrote:



Unless directed otherwise, This is the plan of attack: Option A: On save: if end date is < start date throw error and do not save.

or

Option B: On save: if end date is < start date, set the end date to a year after start date.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/SeedCompany/cord-field/issues/43?email_source=notifications&email_token=AIVA23U5QT7JX3F753CT33LQPDONLA5CNFSM4IXC5BUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBRVKWI#issuecomment-543380825, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AIVA23XZ7DDXPHF34ULN5RDQPDONLANCNFSM4IXC5BUA.

megbacino commented 4 years ago

Daniel, A couple of questions because I am not real familiar with CORD Field.

Does the user (FC) input the end date and start dates or are those derived by automation?

If the user inputs them then I am good with an error showing up, if the dates are derived from an automation then we need to fix that part.

Ken deals with this more than me so I would want him to speak into this.

Thanks,

[Seed Company] Meg Bacino Interim BI Manager t: 817-855-6673 m: 469-628-2276 seedcompany.com A Wycliffe Bible Translators Affiliate

From: Daniel notifications@github.com Reply-To: SeedCompany/cord-field reply@reply.github.com Date: Thursday, October 17, 2019 at 5:04 PM To: SeedCompany/cord-field cord-field@noreply.github.com Cc: Meg Bacino Meg_Bacino@tsco.org, Mention mention@noreply.github.com Subject: Re: [SeedCompany/cord-field] Engagement's initial_end date occurring before stage_begin date. (#43)

Unless directed otherwise, This is the plan of attack: Option A: On save: if end date is < start date throw error and do not save.

or

Option B: On save: if end date is < start date, set the end date to a year after start date.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/SeedCompany/cord-field/issues/43?email_source=notifications&email_token=AIVA23U5QT7JX3F753CT33LQPDONLA5CNFSM4IXC5BUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBRVKWI#issuecomment-543380825, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AIVA23XZ7DDXPHF34ULN5RDQPDONLANCNFSM4IXC5BUA.

gowthamrodda commented 4 years ago

@megbacino @candleshine stage being and initial end dates are not inputs from users. they are assigned based on the status changes of project. https://github.com/SeedCompany/cord-api-plo/pull/14 This PR might fix data incorrectness by workflow. However the condition mentioned by @candleshine is not implemented yet.

With the above PR: StageBegin Date will be projects MOUStart Date and Initial End is MOUENd date when project is made active. We are restricting MOUStartDate to be never greater than MOUEnd Date in the Front end itself.

gowthamrodda commented 4 years ago

image

megbacino commented 4 years ago

I do like that workflow but who would receive that error message? Again, I apologize I am not familiar with the processes.

If we implement the error what will happen to the ones that are wrong now?

[Seed Company] Meg Bacino Interim BI Manager t: 817-855-6673 m: 469-628-2276 seedcompany.com A Wycliffe Bible Translators Affiliate

From: Gowtham notifications@github.com Reply-To: SeedCompany/cord-field reply@reply.github.com Date: Friday, October 18, 2019 at 10:38 AM To: SeedCompany/cord-field cord-field@noreply.github.com Cc: Meg Bacino Meg_Bacino@tsco.org, Mention mention@noreply.github.com Subject: Re: [SeedCompany/cord-field] Engagement's initial_end date occurring before stage_begin date. (#43)

[image]https://user-images.githubusercontent.com/13014470/67108101-68d66380-f1eb-11e9-8420-1a71598f939a.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/SeedCompany/cord-field/issues/43?email_source=notifications&email_token=AIVA23T5UXUJKKKVBUZP7SLQPHJ7RA5CNFSM4IXC5BUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBU4C3A#issuecomment-543801708, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AIVA23W4YSCHX6ANTNB67ELQPHJ7RANCNFSM4IXC5BUA.

gowthamrodda commented 4 years ago

Im not sure how wrong data got into database. We have to fix this regardless of this PR. I ll fix by adding more conditions in the backend to ensure this wont happen further.

megbacino commented 4 years ago

Gotham,

Seth should probably be the advisor on how this should be implemented since we are unsure of the processes involved in retrieving the dates.

Although we do like the validation for the end dates, BI should not be making this decision. We were just bringing up the issue.

To go along with this issue, the Begin and End dates are not populating into Prod.DOMO either.

I have attached a list of Projects with no dates in DOMO. Not sure if these are related issues or separate.

[Seed Company] Meg Bacino Interim BI Manager t: 817-855-6673 m: 469-628-2276 seedcompany.com A Wycliffe Bible Translators Affiliate

From: Gowtham notifications@github.com Reply-To: SeedCompany/cord-field reply@reply.github.com Date: Monday, October 21, 2019 at 6:08 AM To: SeedCompany/cord-field cord-field@noreply.github.com Cc: Meg Bacino Meg_Bacino@tsco.org, Mention mention@noreply.github.com Subject: Re: [SeedCompany/cord-field] Engagement's initial_end date occurring before stage_begin date. (#43)

Im not sure how wrong data got into database. We have to fix this regardless of this PR. I ll fix by adding more conditions in the backend to ensure this wont happen further.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/SeedCompany/cord-field/issues/43?email_source=notifications&email_token=AIVA23QUUOSRS7KWLSQEJ7TQPWESXA5CNFSM4IXC5BUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBZ6HWI#issuecomment-544465881, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AIVA23QY5I3HDNU6C3GVBFLQPWESXANCNFSM4IXC5BUA.

kjones140 commented 4 years ago

Audit.OriginalEndFY2.PROD.xlsx I have just attached a list of records found with an initial_end date (LangOriginalEndFY) before a stgbegindate (LangBeginFY).

candleshine commented 4 years ago

Based on what @kjones140 posted, it looks like MOUStartDate and langBeginFY are disconnected. The work we did on MOUStartDate does not fix this issue.
@gowthamrodda

megbacino commented 4 years ago

Any idea of a timeline to fix this? We currently have over 400 languages without dates.

Thanks!

[Seed Company] Meg Bacino Interim BI Manager t: 817-855-6673 m: 469-628-2276 seedcompany.com A Wycliffe Bible Translators Affiliate

From: Daniel notifications@github.com Reply-To: SeedCompany/cord-field reply@reply.github.com Date: Monday, October 21, 2019 at 3:52 PM To: SeedCompany/cord-field cord-field@noreply.github.com Cc: Meg Bacino Meg_Bacino@tsco.org, Mention mention@noreply.github.com Subject: Re: [SeedCompany/cord-field] Engagement's initial_end date occurring before stage_begin date. (#43)

Based on what @kjones140https://github.com/kjones140 posted, it looks like MOUStartDate and langBeginFY are disconnected. The work we did on MOUStartDate does not fix this issue. @gowthamroddahttps://github.com/gowthamrodda

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/SeedCompany/cord-field/issues/43?email_source=notifications&email_token=AIVA23U7YQ4OIANRDWSVMATQPYJBLA5CNFSM4IXC5BUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEB3YDCQ#issuecomment-544702858, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AIVA23XAW2CWAIZNEDJ56TLQPYJBLANCNFSM4IXC5BUA.

candleshine commented 4 years ago

I have been working on this from the domo-etl side of things while Gowtham approaches it from the cord side of things. We've been attacking it as a problem when the numbers are in the wrong order. If they are blank, should we initialize them to the same as the MOU dates? @sethmcknight

candleshine commented 4 years ago

@gowthamrodda New plan When a project or internship is in Development Status, On Save, project.mouStart should be saved in engagements.dates.stage_begin as well On Save, internship.mouStart should be saved in internshipEngagements.dates.stage_begin as well On Save, project.mouEnd should be saved in engagements.dates.initial_end as well On Save, internship.mouEnd should be saved in internshipEngagements.dates.initial_end as well

Once this is applied, we will need to correct the data.

kjones140 commented 4 years ago

@candleshine Where did this logic come from? Did Seth approve? Also when you are correcting the data, will you ONLY be changing the records that have end years which are blank?

candleshine commented 4 years ago

Yeah seth and I sat together to come up with it. We are going to update the logic first, then we'll go and save a project or two. See if that updates the data. THen we can batch fix some data or we can do it through the UI. That hasn't been decided yet.

On Tue, Oct 29, 2019 at 3:09 PM kjones140 notifications@github.com wrote:

@candleshine https://github.com/candleshine Where did this logic come from? Did Seth approve? Also when you are correcting the data, will you ONLY be changing the records that have end years which are blank?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/SeedCompany/cord-field/issues/43?email_source=notifications&email_token=ABYMOPAU5KXZTTZUSKPUTPTQRCJ77A5CNFSM4IXC5BUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECR5FPY#issuecomment-547607231, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABYMOPFINP4VR2U3YZXZBFLQRCJ77ANCNFSM4IXC5BUA .

kjones140 commented 4 years ago

@candleshine Please do not mass update data based on this logic. Only fix records which have a missing initial_end date with this. Do you agree that this will be the process used?

CarsonF commented 4 years ago

I want to clarify the logic in code right now.

The engagement's initial end date:

The engagement's stage begin date:

This logic feels weird and wrong in multiple places. It seems like these engagement dates need to generally match the project but we also want to be able to change them individually. I think that makes sense, but we need to have better logic here. For example, if we have overridden an engagement date this gets blindly replaced with the project date in some cases. I don't believe we show these dates to the user or allow them to change them. Confusion increases since this data is hidden. Our "magic sync" logic, I don't think, can solve every use case and we need to expose this to users.

@sethmcknight @kjones140 What do you think of the logic above?

kjones140 commented 4 years ago

@CarsonF Thanks so much for providing this logic. I need to read this closely and think thru this, but at first glance...I agree this does not seem entirely accurate. I will respond with more detail later, at this point it will probably won't be until Monday.

candleshine commented 4 years ago

On Thu, Dec 12, 2019 at 1:29 PM Carson Full notifications@github.com wrote:

Just to clarify, the logic in code right now.

The engagement's initial end date:

  • is set to project's mou end date when:
    • the project status changes from a development status to another development status (What??)
    • the project status changes to active for the first time

There should have been logic that checks to see if it is empty before it is changed. No?

-

  • is set to now when:
    • the project's mou end date changes and the engagement is in development

I don't think this should ever happen. 'now' makes no sense in this context.

The engagement's stage begin date:

  • is set to match the project's mou start date when:

    • the engagement is created
    • the project status changes from a development status to another development status (What??)
    • the project status changes to active and the engagement is in development (it becomes active as well)

This came straight from Seth.

This logic feels weird and wrong in multiple places.

It seems like these engagement dates need to generally match the project but we also want to be able to change them individually. I think that makes sense, but we need to have better logic here. For example, if we have overridden an engagement date this gets blindly replaced with the project date in some cases. I don't believe we show these dates to the user or allow them to change them. Confusion increases since this data is hidden. Our "magic sync" logic, I don't think, can solve every use case and we need to expose this to users.

They surface in Domo.

@sethmcknight https://github.com/sethmcknight @kjones140

https://github.com/kjones140 What do you think of the logic above?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/SeedCompany/cord-field/issues/43?email_source=notifications&email_token=ABYMOPEZGL5CQJT6SCQAP7TQYKGI7A5CNFSM4IXC5BUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGXYDVY#issuecomment-565150167, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABYMOPG3DJVJUYL6SI75TE3QYKGI7ANCNFSM4IXC5BUA .

michaelmarshall commented 4 years ago

@CarsonF @candleshine what's the status on this? does the estimate or column need to be updated? Is it ready for promotion?

kjones140 commented 4 years ago

@CarsonF After thinking about this and reading Daniel's comments...here are mine.


Regarding the engagement's initial end date... I completely agree with the following (abbreviated) @candleshine comments on this field

  • is set to project's mou end date when:
    • the project status changes from a development status to active for the first time There should have been logic that checks to see if it is empty before it is changed.

And I agree with this as well

  • is set to now when:
    • the project's mou end date changes and the engagement is in development I don't think this should ever happen.

Regarding the Engagement's stage begin date, I agree with the following...

  • is set to match the project's mou start date when:
    • the project status changes to active and the engagement is in development (it becomes active as well)

I agree with you Carson that

I don't believe we show these dates to the user or allow them to change them. and and we need to expose this to users.

It makes sense to me that users should be able to see and modify these date. (as well as the revised_ end date).

IMO though the product owner @sethmcknight should weigh in on what the field wants to happen here.

CarsonF commented 4 years ago

@michaelmarshall Based on the past few comments we are still trying to figure out what changes are needed.


There should have been logic that checks to see if it is empty before it is changed. No?

@candleshine no that hasn't happened yet. There was a PR up for that but it also included "timezone changes". I'll create a different PR for that.

kjones140 commented 4 years ago

@CarsonF , @candleshine PLEASE READ- I ran some of these comments by Elizabeth P to get her perspective on this.

She corrected me on my following comment...

It makes sense to me that users should be able to see and modify these date. (as well as the revised_ end date). We should NOT allow the user to modify the Stage Begin Date with the UI.

Also she caught a situation that has not been addressed here... How will the stage begin and initial end dates be set for languages that are added to an ALREADY ACTIVE project? It seems from your comments that these dates are initialized only when the engagement is in development and the project becomes active. In this situation the stage begin date should be set from the MOUStartDate, and the initial end date should be set from the MOUEndDate when the language is added to an already active project.

reneepe commented 4 years ago

@kjones140 I compared the engagements' information to the projects' information and the two did not match. I tried to search the engagements' project ids and could not find them on the project page. See below for an example.

image.png

image.png

image.png

sethmcknight commented 4 years ago

@reneepe this is still a high-priority bug, but it appears that further clarification on the logic is needed. Somehow I missed the conversation on this in the past. I'll review and touch base with @candleshine to pass on how the logic should be functioning.

reneepe commented 4 years ago

@candleshine @sethmcknight Checking in on the progress with this one. Any updates?

candleshine commented 4 years ago

@kjones140 I think this is ready to close as well. Please confirm

kjones140 commented 4 years ago

@candleshine Yes, lets close this too. Thanks.