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

Rewrite score connecter #200

Merged
merged 10 commits into from
Sep 7, 2023
Merged

Rewrite score connecter #200

merged 10 commits into from
Sep 7, 2023

Conversation

James-Lu-none
Copy link
Collaborator

@James-Lu-none James-Lu-none commented Sep 4, 2023

Description

A user has a dropout record in the QryRank/QryScore inquiry result, which is not expected in the present data extraction algorithm, so I changed the algorithm and tried to fix this error.

How to Verify?

  • Test the score connector with the data provided by the user.
  • Test with accounts having the same situation and issue.
  • Test with normal accounts to validate the correctness of the algorithm.

Screenshots/GIF/Test Results (Optional)

Copy link
Member

@Xanonymous-GitHub Xanonymous-GitHub left a comment

Choose a reason for hiding this comment

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

From what I can see, there are many areas for improvement in this file. Although our changes are small, we can try to do better with each modification.

Moreover, many steps and variables in this PR are named after HTML elements, making it difficult to understand why these steps appear in the data conversion process. For example, what is h3Node? What table does tableNodes refer to? And so on.

If possible, I'd suggest adding some comments, giving variables more descriptive names, or documenting in external files to clarify what these steps are actually doing. Em... I know it's hard...

lib/src/connector/score_connector.dart Outdated Show resolved Hide resolved
lib/src/connector/score_connector.dart Outdated Show resolved Hide resolved
lib/src/connector/score_connector.dart Outdated Show resolved Hide resolved
lib/src/connector/score_connector.dart Outdated Show resolved Hide resolved
lib/src/connector/score_connector.dart Outdated Show resolved Hide resolved
@Xanonymous-GitHub
Copy link
Member

Regarding the description of the PR, I have a few suggestions:

Firstly, in the 'Description' section, you only wrote 'Problem', and the 'Solution' part may be not detailed enough. You mentioned that you changed the algorithm, but what we should be referring to is 'the process of data transformation'. We may add more details about what exactly has been changed, which key steps are involved, and what is the root of the problem, etc.

Additionally, for the 'How to Verify' section, I recommend writing from a QA perspective. For instance, the first point, "Test the score connector with the data provided by the user," would be hard to understand for the verifier if they are not familiar with what a 'score connector' is.

Lastly, I'm not quite sure what DATA.zip is, but I guess it's the faulty response? I would recommend transferring this in dc only, and consider placing some images or videos that 'show key differences before and after this PR'.

Well... above are just some personal viewpoints by me, You may feel free to change anything :)

@James-Lu-none
Copy link
Collaborator Author

James-Lu-none commented Sep 6, 2023

Firstly, in the 'Description' section, you only wrote 'Problem', and the 'Solution' part may be not detailed enough. You mentioned that you changed the algorithm, but what we should be referring to is 'the process of data transformation'. We may add more details about what exactly has been changed, which key steps are involved, and what is the root of the problem, etc.

  1. The significant difference between his/her data and the normal one is there's a dropout record in the data, which creates a blank row and corrupts the data converting process.

image

So I dropped the invalid row by checking the number of columns in that row since it will push the valid pending data in the list, making the converting process in the for loop produce unexpected results.

  1. Same as the rank query, the overall score query will also create an unexpected element.

image

As I just did, I dropped the invalid semester by checking if there was a score table right next to the semester h3 element.

  • The solutions provided above don't offer a permanent resolution, perhaps applying validating classes will be better for handling such a conversion process.

Lastly, I'm not quite sure what DATA.zip is, but I guess it's the faulty response? I would recommend transferring this in dc only, and consider placing some images or videos that 'show key differences before and after this PR'.

  • The test files in the DATA.zip are provided by the user encountering the issue which can not retrieve results using either the user or the user's friend's mobile devices.

Additionally, for the 'How to Verify' section, I recommend writing from a QA perspective. For instance, the first point, "Test the score connector with the data provided by the user," would be hard to understand for the verifier if they are not familiar with what a 'score connector' is.

  • To verify the modified connector, one can directly paste the HTML text as a string and replace the actual result from the connector.

  • example:

      ...

      result = await Connector.getDataByGet(parameter);
      result='''<html>... </html>''';//<-HTML text String here
      tagNode = parse(result);

      ...

Copy link
Member

@Xanonymous-GitHub Xanonymous-GitHub left a comment

Choose a reason for hiding this comment

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

LGTM.

Remind that we should ensure this pr can really solve the problem before merging.

Copy link
Member

@Xanonymous-GitHub Xanonymous-GitHub left a comment

Choose a reason for hiding this comment

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

LGTM.

@ntut-xuan ntut-xuan assigned ntut-xuan and unassigned ntut-xuan Sep 7, 2023
@ntut-xuan ntut-xuan merged commit e0f53c0 into master Sep 7, 2023
4 checks passed
@ntut-xuan ntut-xuan deleted the rewrite-score-connecter branch September 7, 2023 17:23
@ntut-xuan ntut-xuan added this to the 1.4.5 milestone Oct 21, 2023
@ntut-xuan ntut-xuan mentioned this pull request Oct 23, 2023
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants