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

15632 remotecontrol cpd import participants update #1361

Conversation

mbischof
Copy link
Contributor

@mbischof mbischof commented Jan 9, 2020

Fixed issue # : https://bugs.limesurvey.org/view.php?id=15632
New feature # :
Changed feature # : cpd_importParticipants allows updating records
Dev:
Dev:

// save participant to database
Participant::model()->insertParticipantCSV($aData);
$scenario = $model->getScenario(); // insert or update
if ($scenario == 'update' && $update === false) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Strange to use getScenario :)

Why not , maybe more simple to understand

if (!$model) {
    // Create
} elseif (!$update) {
    continue;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still think i prefer the elseif method. What it's your opinion @olleharstedt ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No strong opinion. So will merge.

@olleharstedt
Copy link
Collaborator

Looks like lots of tests fail for some reason. I will run them locally and check.

@olleharstedt
Copy link
Collaborator

The test fails because there are too many failed login attempts.

@olleharstedt
Copy link
Collaborator

Nope, wrong. The test fails because the default user is overwritten from "admin" to "remotecontroladmin".

@mbischof
Copy link
Contributor Author

mbischof commented Jan 9, 2020

@Shnoulle
Copy link
Collaborator

Scrutinizer is more happy now :).

Else : can you review the website and the email ?


public static function setUpBeforeClass()
{
Yii::import('application.helpers.common_helper', true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

These lines are duplicated - they are already here: https://github.com/LimeSurvey/LimeSurvey/blob/master/tests/TestHelper.php#L23

@olleharstedt
Copy link
Collaborator

I'm guessing you want this merged into new branch 3.x-LTS, not master (which is now LS4)?

@mbischof
Copy link
Contributor Author

thats correct

@mbischof mbischof changed the base branch from master to 3.x-LTS January 17, 2020 00:21
@maziminke
Copy link
Collaborator

We also need this fix to be available at Limesurvey 4 later.

@olleharstedt
Copy link
Collaborator

We also need this fix to be available at Limesurvey 4 later.

No problem.

@olleharstedt
Copy link
Collaborator

thats correct

👍

Please note that I won't merge until the issues are resolved (code duplication, in this case).

Copy link
Collaborator

@Shnoulle Shnoulle left a comment

Choose a reason for hiding this comment

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

No external unknow website please

tests/helpers/remotecontrol/CPDImportParticpantsTest.php Outdated Show resolved Hide resolved
tests/helpers/remotecontrol/CPDImportParticpantsTest.php Outdated Show resolved Hide resolved
@Shnoulle
Copy link
Collaborator

https://tools.ietf.org/html/rfc2606 or limesurvey.org for example . limesurvey.example can be good too :)

@olleharstedt
Copy link
Collaborator

olleharstedt commented Jan 21, 2020

Can you fix the things commented by Denis, please?

@Shnoulle Shnoulle self-requested a review January 22, 2020 08:44
Copy link
Collaborator

@Shnoulle Shnoulle left a comment

Choose a reason for hiding this comment

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

OK, just the elseif part. Buit more an opinion ;)

// save participant to database
Participant::model()->insertParticipantCSV($aData);
$scenario = $model->getScenario(); // insert or update
if ($scenario == 'update' && $update === false) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still think i prefer the elseif method. What it's your opinion @olleharstedt ?

@olleharstedt olleharstedt merged commit b8e9df5 into LimeSurvey:3.x-LTS Jan 22, 2020
@olleharstedt
Copy link
Collaborator

FYI, this PR has been cherry-picked into the master branch.

olleharstedt pushed a commit that referenced this pull request Jan 22, 2020
Dev: fixed return array on checkSessionKey
Dev: tests now use admin user with uid = 1
Dev: Remotecontrol BaseTest extends TestHelper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants