Split Incentive field for studies into two new fields #5

[xavier630]

This pull request solves issue #5. It splits the incentive field of a study into incentive type and incentive detail.

This involves changing the front end when creating, editing and displaying studies, as well as changing what the server and the database expect.

To be honest, I think that it's larger than a small issue as it took many hours and the database side of it got very technical.

[xavier630]

@softeng-701 - could you please review/merge these changes. Thank you!

[softeng-701]

@xavier630 Please squash your commits, thanks!

[xavier630]

@softeng-701 With regards to squashing, I constantly merged master in between my commits to ensure that I stayed up to date with Kelly's master, resolving conflicts as I merged it. As such I get all of the commits from master when trying to squash my commits which makes it undesirable to squash the whole branch. Can we please merge it without squashing so that I don't squash in the commits from master?

This issue has come up numerous times in Visual Git as well:

eg here's a similar issue from Visual Git 3:

I resolved merge conflicts but was unable to squash it as the all the commits on Kelly's master appeared when attempting to squash so was told by Kelly not to squash.

Here's what happens when I go to squash my 6 or so commits ( I have all of the commits from master).

[kblincoe]

You should be able to squash non-sequential commits: https://www.intertech.com/Blog/git-how-to-combine-non-consecutive-multiple-commits-into-one/

If you have trouble, you can skip squashing on this one, but squash on all future pull requests.

[softeng-701]

@xavier630 I am merging this one now! Make sure to squash in the future.

[xavier630]

Thanks! I'll do my best.

[xavier630]

@softeng-701 Do you also agree that this one has a medium worth of content? (See my first comment). It is similar in depth to this medium here: https://github.com/kblincoe/QualOpt_SE701/pull/80

Cheers, Xavier

[TheGuardianWolf]

@softeng-701 @xavier630

Commit 2c6b39b removes the 'incentives' property on study, a test depended on this and haven't been updated.

StudyResourceIntTest.java

I am fixing this on my branch, please review this change below.

https://github.com/kblincoe/QualOpt_SE701/pull/108/commits/cf2ac08a0d52a55bc9f8d9bd90bec6c61e27bdf1

[TheGuardianWolf]

The liquibase configuration is also failing because of clob/blob problems, I'm not sure how to resolve this, could I get some help?

Schema-validation: wrong column type encountered in column [incentive_type] in table [study]; found [clob (Types#CLOB)], but expecting [blob (Types#BLOB)]

When I change it to blob, it says expecting CLOB...

[xavier630]

@TheGuardianWolf Did you have any luck? Is this due to the new Spring Version?

[xavier630]

@TheGuardianWolf Also I've reviewed the new test and it looks good 👍

[TheGuardianWolf]

@xavier630 Have you tried building your branch/master? I'm not the only one getting the error I think.

An annotation needs to be added to the incentive_type property which could solve the problem: @Column(name = “incentive_type”, columnDefinition = “blob”)

The problem comes from the use of enum as a type which cannot be mapped to the database, because there is no 'enum' type in a db by default.

[xavier630]

Yep just gave it a go. Interesting how the build fails - I was able to build/run it fine when I opened the PR.

[TheGuardianWolf]

If I had to guess it's the squashing, I had issues performing my squash which reverted many changes.

Edit: Actually, how did you build it when the DB doesn't have an enum?

[xavier630]

👍 How close are you to getting your changes merged? If you're far away, I can resolve this test myself so we can get master's tests passing.

[xavier630]

Note that if I resolve it on this version of Spring, it'll be on the old version of Spring and conflict when you try to merge your changes.

[TheGuardianWolf]

I'm planning to shove my pull in today, it's been delayed long enough since each time it comes to merge my changes, several pulls are merged before mine and conflict...

[xavier630]

👍 Also - which command did you build it with to get the liquibase error? I'm not able to reproduce it right now on master but I'll keep trying. Were you on master or your own branch?

[TheGuardianWolf]

mvnw clean test

It's my branch, if you can't reproduce then I'll just do the changes on mine since I need it to have the tests running.

[kblincoe]

@TheGuardianWolf please don't merge your own changes. @softeng-701 prioritize this one when they say it is ready.

[softeng-701]

Okay Kelly @kblincoe @TheGuardianWolf @xavier630 Guys, Please let me know when done!

[TheGuardianWolf]

@kblincoe I was meaning to talk to you today about how I can get my pull request merged, each time another pull request gets merged in, I have to adjust the new code to work for SB 2. After that I have to wait a bit for authors to review the changes.

What I was thinking was either merge mine in as the last pull that's done, and give me some time to patch up everyone's code when all the pulls are in, or put mine in after master is fixed this time so people can use the new API.

@softeng-701 The fixes are on another PR #141 rather than this, that one should be priority so I can test my upgrade branch.

[xavier630]

@softeng-701 Still waiting for one teammate to review it. As @TheGuardianWolf said, the fixes are on #141.

[softeng-701]

@xavier630 @TheGuardianWolf #141 merged.. will do this merge after another team member reviews.

[kblincoe]

@TheGuardianWolf the rest of the teams have signed off on the change. I would say to merge this one, and they can update their code accordingly. You should not have to update all code written by others.