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

Save the newest Guest with updated datas #22022

Merged
merged 4 commits into from
Dec 16, 2020

Conversation

prestamodule
Copy link
Contributor

@prestamodule prestamodule commented Nov 19, 2020

Questions Answers
Branch? develop
Description? Right now, with the module statsdata enable, while a guest authenticate, it's call this method. This method will firstly delete the row inside the guest table. After that, it's try to update the same row. Of course, it will never update anything as it's deleted before. The only solution is to add the ObjectModel, with the force_id set to true in order to keep the setted ID.
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no
How to test? Install the statsdata module. Have a guest and authentificate it. See that row inside the guest table is deleted. With this PR, it will be not.

This change is Reviewable

@prestamodule prestamodule requested a review from a team as a code owner November 19, 2020 20:48
@prestonBot
Copy link
Collaborator

Hi, thanks for this contribution!

I found some issues with the Pull Request description:

  • Your pull request does not seem to fix any issue, consider creating one (see note below) and linking it by writing Fixes #1234.

Would you mind having a look at it? This will help us understand how interesting your contribution is, thank you very much!

About linked issues

Please consider opening an issue before submitting a Pull Request:

  • If it's a bug fix, it helps maintainers verify that the bug is effectively due to a defect in the code, and that it hasn't been fixed already.
  • It can help trigger a discussion about the best implementation path before a single line of code is written.
  • It may lead the Core Product team to mark that issue as a priority, further attracting the maintainers' attention.

(Note: this is an automated message, but answering it will reach a real human)

@prestonBot prestonBot added develop Branch Bug Type: Bug labels Nov 19, 2020
classes/Guest.php Outdated Show resolved Hide resolved

// $this is now the old guest but filled with the most up to date values
$this->update();
$this->force_id = true;
return $this->add();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return $this->add();
try {
return $this->add();
} catch (PrestaShopDatabaseException $e) {
return false;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not a PrestaShopException instead ?
Or we need to add the return at the end of the method, to get a return by default (true, or false ?)

Co-authored-by: Arman Hosseini <namra.1377@gmail.com>
@Progi1984 Progi1984 added the Waiting for author Status: action required, waiting for author feedback label Nov 24, 2020
@PrestaEdit
Copy link
Contributor

Hi @Progi1984 ,

Why is it in "Waiting for author" label ?

Progi1984
Progi1984 previously approved these changes Nov 24, 2020
@Progi1984
Copy link
Member

@PrestaEdit Sorry my morning's comment has not be saved 😢

Could you fix PHPCSFixer & Travis, please ?

In order to avoid primary key duplication
@Progi1984 Progi1984 removed the Waiting for author Status: action required, waiting for author feedback label Dec 1, 2020
@sowbiba sowbiba added the Waiting for QA Status: action required, waiting for test feedback label Dec 3, 2020
@SD1982
Copy link
Contributor

SD1982 commented Dec 11, 2020

Hi @prestamodule
Even without the application of your PR I do not have the behavior that you describe in the ps_guest table
Can you please check this on your side?
Can you also please declare an issue by following the provided template which describes the behavior you want to correct with this PR.
Thanks !

@SD1982 SD1982 added the Waiting for author Status: action required, waiting for author feedback label Dec 11, 2020
@PrestaEdit
Copy link
Contributor

Hi,

Please, redo your test. This behavior is since the beginning of PrestaShop and the use of the module "statsdatas".

You need to open your shop (don't be logged, of course) and so have a guest created.
Now, login you. You will have two guest.

@SD1982
Copy link
Contributor

SD1982 commented Dec 15, 2020

Hi @prestamodule
After some additionals tests the PR looks good to me at the beginning I was not testing in the right way because of the "how to test" which was not very explicit (it is also to avoid this kind of misunderstanding that we ask our contributors to write an issue before posting a PR it's really helpfull for us to have a better understanding of the bug) I was trying to "authenticate" the guest from the BO by passing it from "guest" status to "client" status.

Thanks !!

@SD1982 SD1982 added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback Waiting for author Status: action required, waiting for author feedback labels Dec 15, 2020
@prestonBot
Copy link
Collaborator

QA approved, well done! Message to the maintainers: do not forget to milestone it before the merge.

@matthieu-rolland matthieu-rolland merged commit 603fef9 into PrestaShop:develop Dec 16, 2020
@prestonBot
Copy link
Collaborator

Contribution merged, congratulations!

Would you mind answering our quick 1-minute survey? We would love to hear about your experience so far, it will help us improve our process for the community involved, like you. ;-)

@matthieu-rolland
Copy link
Contributor

thank you @prestamodule !

@matthieu-rolland matthieu-rolland added this to the 1.7.8.0 milestone Dec 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Type: Bug develop Branch QA ✔️ Status: check done, code approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants