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

[Conflict Resolver] Reactify + DataFramework #5140

Closed
wants to merge 65 commits into from

Conversation

xlecours
Copy link
Contributor

@xlecours xlecours commented Aug 26, 2019

The frontend and the backend follows the same structure; There is a Conflict_resolver React app that renders a <FilterableDatatable> for unresolved or resolved conflict according to the selected tab.
This React app is provided by the conflict_resolver.class.inc php file along with all the CSS and other JS. The rendered <FilterableDatatable> will the make a fetch (GET) call to either conflict_resolver/unresolved or conflict_resolver/resolved that correspond to 2 RequestHandlers with the same name. Each handler will then process the request and return the data and the filterOptions.

The conflict_resolver/unresolved endpoint also support POST request with conflict_id and correct_answer to resolve a conflict. As long as the page is not refreshed, it is possible to fix a conflict multiple time; each time changing the values in the "wrong" instrument, recalculate score, inserting/replacing the row in conflicts_resolved and removing the row in conflicts_unresolved if not already removed.

A UNIQUE constraint is added on ConflictID in conflicts_resolved table to allow REPLACE sql statement to work when a conflict is resolved then corrected.

Still missing:

  • Feedback to user when a conflict is fixed. "Saved" tooltip or label which fades after 1 second or something like that.
  • Integration test needs to be adapted

Bonus:
This provide a new database provisioner, src/Data/Provisioners/DBObjectProvisioner.php, that returns a Traversable of objects of a given class by using PDOStatement::setFetchMode( PDO::FETCH_CLASS). Basically, each rows are transformed from an associative array to an object with column names as properties. (see unresolveddto.class.inc and resolveddto.class.inc)

This also adds a open-api spec schema.yml file in the static directory of the module to view the endpoitns in swagger-ui.

@xlecours xlecours added the State: Needs work PR awaiting additional work by the author to proceed label Aug 26, 2019
@xlecours xlecours added Cleanup PR or issue introducing/requiring at least one clean-up operation Language: SQL PR or issue that update SQL code and removed State: Needs work PR awaiting additional work by the author to proceed labels Aug 28, 2019
@xlecours xlecours added the State: Needs work PR awaiting additional work by the author to proceed label Aug 28, 2019
@xlecours xlecours removed the State: Needs work PR awaiting additional work by the author to proceed label Sep 6, 2019
@xlecours xlecours requested a review from maltheism September 9, 2019 15:36
@maltheism
Copy link
Member

Hi @xlecours I'm manually testing right now and I'm confused on how to save the Correct Answer in the Unresolved Conflicts panel. As an example I changed one of the values for the Correct Answer and I see the green checkbox but I don't see how to save it. Then after I visit the Resolved panel and I'm not seeing it when filtering for most recent Timestamp.

@xlecours
Copy link
Contributor Author

xlecours commented Sep 9, 2019

@maltheism The save happen onChange automatically. The "Resolved" tab should update when you change tabs so the newly resolved conflict should appear right away.
Can you see the new entry in the conflicts_resolved table? Is it removed from the conflicts_unresolved?

@xlecours xlecours requested a review from driusan February 4, 2020 17:19
Copy link
Collaborator

@ridz1208 ridz1208 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • in modules/conflict_resolver/php/conflict_resolver.class.inc line 404: PhanUndeclaredStaticMethod Static call to undeclared method \NDB_Page::toArray

After fixing the requested changes locally, the module seemed to load and function normally.

A UI improvement could be done for the autosaved select elements, they show up like this on my chrome
Screenshot from 2020-02-12 13-08-38

modules/conflict_resolver/test/conflict_resolverTest.php Outdated Show resolved Hide resolved
public function getCenterID(): int
{
return (int) $this->centerid;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get projectId function should be added

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To filter out other projects data, just like centerid

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ridz1208 Done

@ridz1208 ridz1208 self-assigned this Feb 29, 2020
@xlecours
Copy link
Contributor Author

@driusan This is passing Travis now. Ready for review

public function getCenterID(): int
{
return (int) $this->centerid;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To filter out other projects data, just like centerid

@xlecours
Copy link
Contributor Author

@ridz1208 Done. Please re-review

*
* @param {Event} e - The onChange event.
*/
fix(e) {
Copy link
Collaborator

@HenriRabalais HenriRabalais Mar 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest having this function alter the DOM via a state change, rather than altering the element directly. That way you can also use the LORIS SelectElement rather than a custom element.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you care to propose something?

Copy link
Contributor Author

@xlecours xlecours Mar 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You suggestion must:

  1. send a post request to /conflict_resolver/unresolved with {conflictid: conflictid, correctanswer: correctanswer}
  2. display feedback to the user for each resolved conflict.
  3. remove the empty option in the select options when a conflict is resolved because it can be re-resolved with an other answer but not bring back as unresolved.

Copy link
Collaborator

@HenriRabalais HenriRabalais Mar 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

  1. This wouldn't have to change. The fix function could still post the request the same way you are currently doing it.
  2. The fix function would call a function that will have been passed by the parent via props and pass an id that indicates which row of the datatable it corresponds to. This callback function would then trigger a state change in the parent indexed by the row id which would display the appropriate feedback to the user for that given datatable row in the formatColumn function of unresolved_filterabledatatable.js.
  3. You could use the same state from (2) to conditionally remove the empty option when a conflict has been resolved. There's a prop you can pass to SelectElements called emptyOption that determines whether there is an empty option or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HenriRabalais Would you care to propose something? (I mean code)

Copy link
Collaborator

@HenriRabalais HenriRabalais Apr 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want me to commit code to your PR? I don't think pasting code here would do much good since it's out of the context of the PR, and my previous response are instructions for the code I'm proposing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sending a PR to my branch would do the trick.

@ridz1208 ridz1208 removed their assignment Apr 27, 2020
@johnsaigle johnsaigle added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label May 26, 2020
@ridz1208
Copy link
Collaborator

@HenriRabalais @xlecours do we know how close this is to being mergeable ? other than conflicts and travis ?

@xlecours
Copy link
Contributor Author

@ridz1208 I completely agree with @HenriRabalais but I don't have time at the moment to look into it. The current state is working but it is not following React best practices.

I will fix the conflicts

If @HenriRabalais can send a PR to my branch with his requested changes implemented, I will gladly merge it.

@ridz1208
Copy link
Collaborator

This PR was 90% there, a new PR has been issued. May it RIP

@ridz1208 ridz1208 closed this Jul 23, 2020
@xlecours
Copy link
Contributor Author

xlecours commented Aug 11, 2020

I sent HenriRabalais#7 to fix the thing. There is still an issue with the selected option after the fix.

driusan pushed a commit that referenced this pull request Sep 24, 2021
implements the auto-saving of conflict resolution
Add 2 endpoints (/conflict_resolver/unresolved and /conflict_resolver/resolved)
Adds a schema.yml file

PR #6862
PR #5140
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup PR or issue introducing/requiring at least one clean-up operation Language: SQL PR or issue that update SQL code State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants