-
Notifications
You must be signed in to change notification settings - Fork 15
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
Executing Test: Tester View: Test Conflict Resolution, Pt 1 #94
Conversation
9c33bc3
to
6ef1c2e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the next thing I need to work on is identifying conflicts, in which case, I'm inclined to just merge this PR and work on top of it. My main issues are with the workflow and that I think your stump'ed function calls don't need to exist, so I can just remove them in my work.
</Button> | ||
</Modal.Footer> | ||
</Modal> | ||
<> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
omg, what is this? replacement for fragment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
* @param {Number} cycle_id The id of the cycle to retrieve conflicts | ||
* @return {Array} An array of conflict objects. | ||
*/ | ||
export function getConflictsByCycleId(cycle_id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually don't need this. All the results for a cycle are already in the front end -- so we can do the conflict calculation on the front end.
As it is right now, having all test results for all test for a cycle when you contribute to a cycle -- might seems a little heavy handed.. but for now it is not too much data. we might need to later serve things in smaller pieces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might be thinking of something different here. The conflicts/diffs that @s3ththompson and I planned out are based on a serialization of the actual form itself within the iframe. That would also be used to repopulate those forms the next time you open the test to run it. The current data that I saw in the result objects does not have the details necessary to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think the issue is that there are 2 concepts here with similar terminology. here's my conception of them, hopefully it matches yours?
- [1] the results conflict diff, which Matt sketched out, which will go at the bottom of the GitHub issue, which Val points out can be generated on the frontend.
- [2] the (basically) vdom state that will need to be serialized from the iframe form, saved to the db, and then retrieved to re-hydrate the form during editing.
@rwaldron it sounds like this stub is for [2]? if so, is it appropriately named? don't we need to save the serialized data for all results, whether it's conflicted or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@s3ththompson yes, you're correct. @spectranaut and I had a call to discuss this and I described our idea and she's got it now.
client/actions/cycles.js
Outdated
* @param {Number} cycle_id The id of the cycle to retrieve results | ||
* @return {Array} An array of result objects. | ||
*/ | ||
export function getResultsByCycleId(cycle_id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, we have all this data.
e5c5845
to
747a61e
Compare
- Executing Test: Tester View: Status Bar - API request new GitHub issue on backend, save ID, and redirect - Query GitHub Issue ID for status on view test
747a61e
to
e1a3281
Compare
issue_number: { | ||
type: Sequelize.INTEGER, | ||
allowNull: false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you may be missing the open
boolean column?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just got an error about this @s3ththompson , I'll push a commit...
server/models/TestIssue.js
Outdated
}, | ||
open: { | ||
type: BOOLEAN, | ||
allowNull: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe boolean false is not null, so allowNull
can be false for consistency / safety.
yarn db-migrate:dev
yarn
yarn dev
Test Plans"
" prompt, and shown a list that has your issue in it.
" prompt.